Implement the new wallpaper manager launcher window.

Implement the new wallpaper manager launcher window.


BUG=118684
TEST=
1. Goto about:flags, enable "Experimental Wallpaper UI" flag. (chromeos only)
2. Relaunch browser
3. Open some browser windows.
4. Click "Set Wallpaper..." from background context menu or from
"chrome://chrome/settings/"

Expected: all open browser windows will minimized, the new Wallpaper Manager window
should launch.

5. close the launched Wallpaper Manager window

Expected: all previous open browser windows should restore to its state before wallpaper
manager launch.(full screen, normal, minimized)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146608

[bshe, 2012-07-04 20:51:14]

Hi Rob.

Could you take a look at this CL? I started to split CLs from one large CL and
this is the first of them.

Basically, this CL implements the wallpaper manager container. When the
container launched, all existing windows will minimize. And when the container
closed, those existing windows then restore to it's states before wallpaper
manager launch. 

I also include harness and mock files. Those only used for development purpose.

[bshe, 2012-07-04 20:54:51]

+kaznacheev

Could you please take a look at this CL? Or could you please add appropriate
reviewers for this CL? I am implementing a wallpaper manager component
extension. And I use file manager extension as example to implement it. So it
could be very helpful to have someone with lots of file manager experience as my
reviewer. Thanks!

spec and mocks for wallpaper manager are here:
https://docs.google.com/a/google.com/document/d/1iBxnEswdOKPJqoKNLgGfUDSNSnvR...

Thanks!

[flackr, 2012-07-04 22:20:56]

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.cc (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:28: const char
kWallpaperManagerDomain[] = WALLPAPERMANAGER_EXTENSON_ID;
Why not just assign the extension ID directly?

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:32: // Hide behind
flag
Does this comment belong by the if statement?

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:18: // to add some
wallpaper manager specific api functions.
While it only has 1 function, it is not really a placeholder anymore. You can
say this is the wallpaper manager API.

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/harness.html (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/harness.html:34: <script
src="js/mock_chrome.js"></script>
You could probably conditionally include mock_chrome.js from main.html file in
the <if expr="0">. This way it would use mock_chrome when run outside of chrome
and the script would not get included within chrome.

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/background.js
(right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/background.js:48: var
windowStateManager = new WindowStateManager();
So does this background page run all the time? If so, can we tell it to shut
down after it finishes restoring window states?

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/main.js (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/main.js:8: var
wallpaperManager;
Initialized elsewhere?

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/main.html (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/main.html:14: Keep the list
in sync with js/main_scripts.js. -->
Do you know why file manager was written this way? I still think you should just
include all your javascript here and then things would be simpler.

[bshe, 2012-07-05 13:54:49]

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.cc (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:28: const char
kWallpaperManagerDomain[] = WALLPAPERMANAGER_EXTENSON_ID;
On 2012/07/04 22:20:57, flackr wrote:
> Why not just assign the extension ID directly?

Done.

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:32: // Hide behind
flag
On 2012/07/04 22:20:57, flackr wrote:
> Does this comment belong by the if statement?

Done.

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:18: // to add some
wallpaper manager specific api functions.
On 2012/07/04 22:20:57, flackr wrote:
> While it only has 1 function, it is not really a placeholder anymore. You can
> say this is the wallpaper manager API.

Done.

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/harness.html (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/harness.html:34: <script
src="js/mock_chrome.js"></script>
I was just hoping to make the mock looks as real as possible. So I add a grey
title bar here to mock the the real title bar in the extension window. Also, I
want to mock the set wallpaper function. Using harness, I can set the width and
height of the wallpaper picker ui(iframe) and mock set wallpaper function by set
the body background image of harness.html. But if I directly access main.html, I
can't set a specific width/height for the picker ui(it's the same as the width
and height of body) and it is no easy to mock the set wallpaper function.

On 2012/07/04 22:20:57, flackr wrote:
> You could probably conditionally include mock_chrome.js from main.html file in
> the <if expr="0">. This way it would use mock_chrome when run outside of
chrome
> and the script would not get included within chrome.

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/background.js
(right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/background.js:48: var
windowStateManager = new WindowStateManager();
According to this:
http://code.google.com/chrome/extensions/background_pages.html

background page has the same lifetime af of the extension. So it should not run
all the time.

On 2012/07/04 22:20:57, flackr wrote:
> So does this background page run all the time? If so, can we tell it to shut
> down after it finishes restoring window states?

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/main.js (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/main.js:8: var
wallpaperManager;
Aha. Sorry. The initiazlier is in a script file which I dont intend to include
in this CL. So I will just remove the global variable for now.

On 2012/07/04 22:20:57, flackr wrote:
> Initialized elsewhere?

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/main.html (right):

https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/main.html:14: Keep the list
in sync with js/main_scripts.js. -->
The main_scripts was introduced in this CL:
http://codereview.chromium.org/10068007/
It seems to relate to file manager's content security policy at that time.
However, after a quick test with wallpaper manager. I found that even with
"default-src:none" setting (I am guessing it means inline scripts are
disallowed), just using <script> tag for script files also work. These scripts
are not inlined to html after flatten.

Another perspective is using main_scripts can avoid to keep the list sync with
chrome/browser/resources/component_extension_resources.grd file. So in the grd
file, only main_scripts.js needed to be added. So we either keep the list sync
with grd file or with main_scripts file. But using main_scripts may have a
faster loading time since we only have to load one file?

On 2012/07/04 22:20:57, flackr wrote:
> Do you know why file manager was written this way? I still think you should
just
> include all your javascript here and then things would be simpler.

[Vladislav Kaznacheev, 2012-07-05 17:01:36]

I will take a look tomorrow.
On 2012/07/05 13:54:49, bshe wrote:
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
> File chrome/browser/chromeos/extensions/wallpaper_manager_api.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
> chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:28: const char
> kWallpaperManagerDomain[] = WALLPAPERMANAGER_EXTENSON_ID;
> On 2012/07/04 22:20:57, flackr wrote:
> > Why not just assign the extension ID directly?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
> chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:32: // Hide behind
> flag
> On 2012/07/04 22:20:57, flackr wrote:
> > Does this comment belong by the if statement?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
> File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/chro...
> chrome/browser/chromeos/extensions/wallpaper_manager_api.h:18: // to add some
> wallpaper manager specific api functions.
> On 2012/07/04 22:20:57, flackr wrote:
> > While it only has 1 function, it is not really a placeholder anymore. You
can
> > say this is the wallpaper manager API.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
> File chrome/browser/resources/chromeos/wallpaper_manager/harness.html (right):
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
> chrome/browser/resources/chromeos/wallpaper_manager/harness.html:34: <script
> src="js/mock_chrome.js"></script>
> I was just hoping to make the mock looks as real as possible. So I add a grey
> title bar here to mock the the real title bar in the extension window. Also, I
> want to mock the set wallpaper function. Using harness, I can set the width
and
> height of the wallpaper picker ui(iframe) and mock set wallpaper function by
set
> the body background image of harness.html. But if I directly access main.html,
I
> can't set a specific width/height for the picker ui(it's the same as the width
> and height of body) and it is no easy to mock the set wallpaper function.
> 
> On 2012/07/04 22:20:57, flackr wrote:
> > You could probably conditionally include mock_chrome.js from main.html file
in
> > the <if expr="0">. This way it would use mock_chrome when run outside of
> chrome
> > and the script would not get included within chrome.
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
> File chrome/browser/resources/chromeos/wallpaper_manager/js/background.js
> (right):
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
> chrome/browser/resources/chromeos/wallpaper_manager/js/background.js:48: var
> windowStateManager = new WindowStateManager();
> According to this:
> http://code.google.com/chrome/extensions/background_pages.html
> 
> background page has the same lifetime af of the extension. So it should not
run
> all the time.
> 
> On 2012/07/04 22:20:57, flackr wrote:
> > So does this background page run all the time? If so, can we tell it to shut
> > down after it finishes restoring window states?
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
> File chrome/browser/resources/chromeos/wallpaper_manager/js/main.js (right):
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
> chrome/browser/resources/chromeos/wallpaper_manager/js/main.js:8: var
> wallpaperManager;
> Aha. Sorry. The initiazlier is in a script file which I dont intend to include
> in this CL. So I will just remove the global variable for now.
> 
> On 2012/07/04 22:20:57, flackr wrote:
> > Initialized elsewhere?
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
> File chrome/browser/resources/chromeos/wallpaper_manager/main.html (right):
> 
>
https://chromiumcodereview.appspot.com/10698099/diff/1017/chrome/browser/reso...
> chrome/browser/resources/chromeos/wallpaper_manager/main.html:14: Keep the
list
> in sync with js/main_scripts.js. -->
> The main_scripts was introduced in this CL:
> http://codereview.chromium.org/10068007/
> It seems to relate to file manager's content security policy at that time.
> However, after a quick test with wallpaper manager. I found that even with
> "default-src:none" setting (I am guessing it means inline scripts are
> disallowed), just using <script> tag for script files also work. These scripts
> are not inlined to html after flatten.
> 
> Another perspective is using main_scripts can avoid to keep the list sync with
> chrome/browser/resources/component_extension_resources.grd file. So in the grd
> file, only main_scripts.js needed to be added. So we either keep the list sync
> with grd file or with main_scripts file. But using main_scripts may have a
> faster loading time since we only have to load one file?
> 
> On 2012/07/04 22:20:57, flackr wrote:
> > Do you know why file manager was written this way? I still think you should
> just
> > include all your javascript here and then things would be simpler.

[Vladislav Kaznacheev, 2012-07-06 10:57:12]

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/main.js (right):

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/main.js:7:
chrome.extension.getBackgroundPage().windowStateManager;
Why do you need to keep the state in the background page? Do you expect to have
more than one wallpaper manager window open?

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/manifest.json (right):

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/manifest.json:12: "scripts":
["js/background.js"]
Having a background page is expensive because it occupies some memory (~20M last
time I checked) even if you do not have you main.html loaded.
I suggest getting rid of it if possible

[flackr, 2012-07-06 12:28:52]

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/background.js
(right):

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/background.js:37: for
(var j in self.savedWindow) {
You can probably avoid the need for a background page if you just send window
state updates for every window you remembered rather than wait for the
asynchronous window list to come back. I think it wouldn't matter if you sent an
update for a window id which no longer existed, and then the entire function
would be synchronous and could run in the page rather than a background page.

[bshe, 2012-07-06 18:58:05]

done. Thanks!

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/background.js
(right):

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/background.js:37: for
(var j in self.savedWindow) {
Done. Also added a focus event listener. So when the wallpaper manager get
focused, all opened windows should minimize.

On 2012/07/06 12:28:53, flackr wrote:
> You can probably avoid the need for a background page if you just send window
> state updates for every window you remembered rather than wait for the
> asynchronous window list to come back. I think it wouldn't matter if you sent
an
> update for a window id which no longer existed, and then the entire function
> would be synchronous and could run in the page rather than a background page.

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/main.js (right):

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/main.js:7:
chrome.extension.getBackgroundPage().windowStateManager;
When wallpaper manager window opens, it hides all existing browser windows are
minimized so user can see their choice of wallpaper more clearly. But after
window manager closed, we want to restore the state of all existing windows. So
I keep the state in the background page. I would not expect to have more than
one wallpaper manager open but I haven't implement the check yet.

On 2012/07/06 10:57:13, Vladislav Kaznacheev wrote:
> Why do you need to keep the state in the background page? Do you expect to
have
> more than one wallpaper manager window open?

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/manifest.json (right):

https://chromiumcodereview.appspot.com/10698099/diff/5003/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/manifest.json:12: "scripts":
["js/background.js"]
Removed.

On 2012/07/06 10:57:13, Vladislav Kaznacheev wrote:
> Having a background page is expensive because it occupies some memory (~20M
last
> time I checked) even if you do not have you main.html loaded.
> I suggest getting rid of it if possible

[flackr, 2012-07-09 21:29:58]

https://chromiumcodereview.appspot.com/10698099/diff/15002/chrome/browser/chr...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.cc (right):

https://chromiumcodereview.appspot.com/10698099/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:54: }  //namespace
wallpaper_manager_util
Space after //.

https://chromiumcodereview.appspot.com/10698099/diff/15002/chrome/browser/chr...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

https://chromiumcodereview.appspot.com/10698099/diff/15002/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:17: // Wallpaper
manager API functions. Following up CL will add implementationis
s/Following up/Followup
s/implementationis/implementations

This comment describes the namespace, not the const char. It should probably be
moved down.

https://chromiumcodereview.appspot.com/10698099/diff/15002/chrome/browser/res...
File chrome/browser/resources/chromeos/wallpaper_manager/js/mock_chrome.js
(right):

https://chromiumcodereview.appspot.com/10698099/diff/15002/chrome/browser/res...
chrome/browser/resources/chromeos/wallpaper_manager/js/mock_chrome.js:9:
getBackgroundPage: function() {
You no longer use a background page right?

https://chromiumcodereview.appspot.com/10698099/diff/15002/chrome/browser/res...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/window_state_manager.js
(right):

https://chromiumcodereview.appspot.com/10698099/diff/15002/chrome/browser/res...
chrome/browser/resources/chromeos/wallpaper_manager/js/window_state_manager.js:20:
if (windows[i].state != 'minimized')
You should probably add this to the following condition and both update and save
the window to the list.

[bshe, 2012-07-10 01:39:22]

Done. Thanks!

http://codereview.chromium.org/10698099/diff/15002/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.cc (right):

http://codereview.chromium.org/10698099/diff/15002/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:54: }  //namespace
wallpaper_manager_util
On 2012/07/09 21:29:58, flackr wrote:
> Space after //.

Done.

http://codereview.chromium.org/10698099/diff/15002/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

http://codereview.chromium.org/10698099/diff/15002/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:17: // Wallpaper
manager API functions. Following up CL will add implementationis
On 2012/07/09 21:29:58, flackr wrote:
> s/Following up/Followup
> s/implementationis/implementations
> 
> This comment describes the namespace, not the const char. It should probably
be
> moved down.

Done. Moved the comment below namespace. It will be removed in my next CL.

http://codereview.chromium.org/10698099/diff/15002/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/mock_chrome.js
(right):

http://codereview.chromium.org/10698099/diff/15002/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/mock_chrome.js:9:
getBackgroundPage: function() {
Aha. Right. I should remove it. Sorry about it.

On 2012/07/09 21:29:58, flackr wrote:
> You no longer use a background page right?

http://codereview.chromium.org/10698099/diff/15002/chrome/browser/resources/c...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/window_state_manager.js
(right):

http://codereview.chromium.org/10698099/diff/15002/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/window_state_manager.js:20:
if (windows[i].state != 'minimized')
On 2012/07/09 21:29:58, flackr wrote:
> You should probably add this to the following condition and both update and
save
> the window to the list.

Done.

[bshe, 2012-07-11 16:51:17]

On 2012/07/10 01:39:22, bshe wrote:
> Done. Thanks!
> 
>
http://codereview.chromium.org/10698099/diff/15002/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/wallpaper_manager_api.cc (right):
> 
>
http://codereview.chromium.org/10698099/diff/15002/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:54: }  //namespace
> wallpaper_manager_util
> On 2012/07/09 21:29:58, flackr wrote:
> > Space after //.
> 
> Done.
> 
>
http://codereview.chromium.org/10698099/diff/15002/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):
> 
>
http://codereview.chromium.org/10698099/diff/15002/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/wallpaper_manager_api.h:17: // Wallpaper
> manager API functions. Following up CL will add implementationis
> On 2012/07/09 21:29:58, flackr wrote:
> > s/Following up/Followup
> > s/implementationis/implementations
> > 
> > This comment describes the namespace, not the const char. It should probably
> be
> > moved down.
> 
> Done. Moved the comment below namespace. It will be removed in my next CL.
> 
>
http://codereview.chromium.org/10698099/diff/15002/chrome/browser/resources/c...
> File chrome/browser/resources/chromeos/wallpaper_manager/js/mock_chrome.js
> (right):
> 
>
http://codereview.chromium.org/10698099/diff/15002/chrome/browser/resources/c...
> chrome/browser/resources/chromeos/wallpaper_manager/js/mock_chrome.js:9:
> getBackgroundPage: function() {
> Aha. Right. I should remove it. Sorry about it.
> 
> On 2012/07/09 21:29:58, flackr wrote:
> > You no longer use a background page right?
> 
>
http://codereview.chromium.org/10698099/diff/15002/chrome/browser/resources/c...
> File
> chrome/browser/resources/chromeos/wallpaper_manager/js/window_state_manager.js
> (right):
> 
>
http://codereview.chromium.org/10698099/diff/15002/chrome/browser/resources/c...
>
chrome/browser/resources/chromeos/wallpaper_manager/js/window_state_manager.js:20:
> if (windows[i].state != 'minimized')
> On 2012/07/09 21:29:58, flackr wrote:
> > You should probably add this to the following condition and both update and
> save
> > the window to the list.
> 
> Done.

Friendly ping?

[flackr, 2012-07-11 16:57:35]

LGTM with nit.

http://codereview.chromium.org/10698099/diff/21001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

http://codereview.chromium.org/10698099/diff/21001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:28: // some API
functions.
Sorry I meant put this after the extern const char, but before the namespace
wallpaper_manager_util, i.e. before line 19.

[bshe, 2012-07-11 19:52:21]

Sorry for my misunderstanding. Done!

http://codereview.chromium.org/10698099/diff/21001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

http://codereview.chromium.org/10698099/diff/21001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:28: // some API
functions.
On 2012/07/11 16:57:35, flackr wrote:
> Sorry I meant put this after the extern const char, but before the namespace
> wallpaper_manager_util, i.e. before line 19.

Done.

[Vladislav Kaznacheev, 2012-07-13 14:54:48]

lgtm

[commit-bot: I haz the power, 2012-07-13 18:31:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/10698099/1041

[commit-bot: I haz the power, 2012-07-13 18:31:25]

Presubmit check for 10698099-1041 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

See the JavaScript style guide at
http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript
and if you have any feedback about the JavaScript PRESUBMIT check, contact
tbreisacher@chromium.org

** Presubmit Warnings **
Found JavaScript style violations in
chrome/browser/resources/chromeos/wallpaper_manager/js/window_state_manager.js:
  line 44: E0210: Missing docs for parameter: "windowId"
WindowStateManager.prototype.removeSavedState = function(windowId) {
                                                                 ^^

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui/options2
    chrome/browser/resources/chromeos
    chrome/browser/resources/options2
    chrome/browser/resources

Banned functions were used.
    chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:51:
      This function is deprecated and we're working on removing it. Pass
      more context to get a Browser*, like a WebContents, window, or session
      id. Talk to ben@ or jam@ for more information.

Do not use #pragma once in header files.
See http://www.chromium.org/developers/coding-style#TOC-File-headers
  chrome/browser/chromeos/extensions/wallpaper_manager_api.h

Presubmit checks took 1.9s to calculate.

[bshe, 2012-07-13 18:34:42]

+jhawkins for owners

    chrome/browser/ui/webui/options2
    chrome/browser/resources/*

Thanks!

[James Hawkins, 2012-07-13 18:46:55]

LGTM