Enable user change background image in settings page in Aura build.

Enable user change background image in settings page in Aura build.

Only support change between default background images currently.
Need more refactor work on code.

BUG=105508
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126319

[bshe, 2012-03-02 21:49:19]

Hi Rob.

Could you please take a look at this CL? It's the 1 iteration of implementing
Aura desktop background change feature.

Could you please take a look at the ash/shell parts first, particularly the
controller and observer. I am not familiar with controller/observer in chrome.
Hopefully I did it right. Thanks!

Please ignore the webui part for now, it needs more polish work. Since it's kind
of big CL, I think rather than blocking on these polish works, I just submit it
as it is and have you take a look at the ash/shell part first.

[bshe, 2012-03-05 14:51:47]

Hi Rob.

I had this newer version. Could you please take a look?

Thanks,

Biao

[flackr, 2012-03-05 16:16:01]

https://chromiumcodereview.appspot.com/9580023/diff/6001/ash/desktop_backgrou...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/ash/desktop_backgrou...
ash/desktop_background/desktop_background_view.cc:6: #include
"ash/desktop_background/desktop_background_controller.h"
This include goes below with the others. desktop_background_view.h goes up here
because that is the header for this .cc file.

https://chromiumcodereview.appspot.com/9580023/diff/6001/ash/desktop_backgrou...
ash/desktop_background/desktop_background_view.cc:22: 
nit: no newline here.

https://chromiumcodereview.appspot.com/9580023/diff/6001/ash/desktop_backgrou...
ash/desktop_background/desktop_background_view.cc:34:
IDR_AURA_WALLPAPER_1).ToSkBitmap();
If the user has a custom background set this will have to load the default
wallpaper and then change it. I think that you should pass the wallpaper to use
in the constructor for DesktopBackgroundView and then ash::Shell or the
DesktopBackgroundController can create this view with the appropriate wallpaper.

https://chromiumcodereview.appspot.com/9580023/diff/6001/ash/desktop_backgrou...
File ash/desktop_background/desktop_background_view.h (right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/ash/desktop_backgrou...
ash/desktop_background/desktop_background_view.h:15: 
nit: no newline here.

https://chromiumcodereview.appspot.com/9580023/diff/6001/ash/desktop_backgrou...
ash/desktop_background/desktop_background_view.h:26: friend struct
DefaultSingletonTraits<DesktopBackgroundView>;
I worry about the implications of making this class a Singleton. This class
doesn't have to exist if BackgroundMode is BACKGROUND_SOLID_COLOR and things may
get trickier if eventually there are more than one RootWindow. I think you
should be modifying Shell::SetDesktopBackgroundMode or making a new function in
ash::Shell for setting the desktop background. Since ash::Shell creates the
desktop backgrounds it can remember the DesktopBackgroundView or just create a
new one when you set a new wallpaper.

https://chromiumcodereview.appspot.com/9580023/diff/6001/ash/shell.h
File ash/shell.h (right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/ash/shell.h#newcode52
ash/shell.h:52: class DesktopBackgroundController;
nit: Sort.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/chrom...
File chrome/browser/chromeos/login/user.h (right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/chrom...
chrome/browser/chromeos/login/user.h:35: static const int kDefaultWallpaperIndex
= 0;
Comment.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/chrom...
chrome/browser/chromeos/login/user.h:62: int wallpaper_index() const {return
wallpaper_index_;}
Comment

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/chrom...
chrome/browser/chromeos/login/user.h:82: void SetWallpaper(int wallpaper_index);
Comment.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/chrom...
chrome/browser/chromeos/login/user.h:108: int wallpaper_index_;
Comment.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/chrom...
File chrome/browser/chromeos/options2/desktop_background_observer.cc (right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/chrom...
chrome/browser/chromeos/options2/desktop_background_observer.cc:53: NOTREACHED()
<< "Unexpected notification " << type;
Remove text message, reason for hitting the NOTREACHED is obvious from code and
static text messages increase our binary size.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/chrom...
File chrome/browser/chromeos/options2/desktop_background_observer.h (right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/chrom...
chrome/browser/chromeos/options2/desktop_background_observer.h:15: // the Aura
shell's DesktopBackgroundController class.
This class seems misplaced / unnecessary. Aura shell DesktopBackgroundController
could directly listen for background change events.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/resou...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.js:114: if (cr.isChromeOS) {
Does cr.isChromeOS == if_def('use_aura')? Seems like it would be safer to check
for the existence of the button.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/resou...
File chrome/browser/resources/options2/options.js (right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/resou...
chrome/browser/resources/options2/options.js:126: [$('set-wallpaper')]);
This also concerns me, I'm pretty sure that options will be broken on ChromeOS
non-aura (chromeos_gtk=1) and this won't be registered if we support an aura
non-chromeos build.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:29:
#if defined(USE_AURA)
This is only included if USE_AURA is defined, so this check should be
unnecessary.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:30:
#include "ash/desktop_background/desktop_background_controller.h"
And I don't think you actually use this class in here.

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

https://chromiumcodereview.appspot.com/9580023/diff/6001/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:72: #include
"chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h"
This seems inconsistent with everywhere else where the wallpaper code is
included on USE_AURA.

[bshe, 2012-03-06 01:33:27]

Done except for files related to user or user_manager

There is a problem with these code now. After user logout, the index value is
not saved. It seems I need to use pref_service and save it into local_state. I
will submit this part of code in a new delta.

Thanks!

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
File ash/desktop_background/desktop_background_view.cc (right):

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_view.cc:6: #include
"ash/desktop_background/desktop_background_controller.h"
On 2012/03/05 16:16:03, flackr wrote:
> This include goes below with the others. desktop_background_view.h goes up
here
> because that is the header for this .cc file.

Done.

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_view.cc:22: 
On 2012/03/05 16:16:03, flackr wrote:
> nit: no newline here.

Done.

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_view.cc:34:
IDR_AURA_WALLPAPER_1).ToSkBitmap();
On 2012/03/05 16:16:03, flackr wrote:
> If the user has a custom background set this will have to load the default
> wallpaper and then change it. I think that you should pass the wallpaper to
use
> in the constructor for DesktopBackgroundView and then ash::Shell or the
> DesktopBackgroundController can create this view with the appropriate
wallpaper.

It seems this is constructed before NOTIFICATION_LOGIN_USER_CHANGED
notification. I assume the custom background will only available after
NOTIFICATION_LOGIN_USER_CHANGED notification(I could probably be wrong). If this
is the case, unless we delay the construction of this class after
NOTIFICATION_LOGIN_USER_CHANGED, we can't initialize it with user custom
background. I have tested delay the creation of DesktopBackgroundView after we
observer the notification. But there seems to be a check failed in debug build.
(FATAL:login_performer.cc(226)] Check failed: false.) 
I am thinking we can probably do nothing in the constructor at first and set the
wallpaper after user login as a walkaround.

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
File ash/desktop_background/desktop_background_view.h (right):

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_view.h:15: 
On 2012/03/05 16:16:03, flackr wrote:
> nit: no newline here.

Done.

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_view.h:26: friend struct
DefaultSingletonTraits<DesktopBackgroundView>;
On 2012/03/05 16:16:03, flackr wrote:
> I worry about the implications of making this class a Singleton. This class
> doesn't have to exist if BackgroundMode is BACKGROUND_SOLID_COLOR and things
may
> get trickier if eventually there are more than one RootWindow. I think you
> should be modifying Shell::SetDesktopBackgroundMode or making a new function
in
> ash::Shell for setting the desktop background. Since ash::Shell creates the
> desktop backgrounds it can remember the DesktopBackgroundView or just create a
> new one when you set a new wallpaper.

I created a new function called SetDesktopBackgroundWallpaper in shell.

http://codereview.chromium.org/9580023/diff/6001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/9580023/diff/6001/ash/shell.h#newcode52
ash/shell.h:52: class DesktopBackgroundController;
On 2012/03/05 16:16:03, flackr wrote:
> nit: Sort.

Done.

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user.h (right):

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user.h:62: int wallpaper_index() const {return
wallpaper_index_;}
On 2012/03/05 16:16:03, flackr wrote:
> Comment

The function doesn't seem to work after user logout. Will add comments after I
identify the problem

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/chromeos/opti...
File chrome/browser/chromeos/options2/desktop_background_observer.cc (right):

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/chromeos/opti...
chrome/browser/chromeos/options2/desktop_background_observer.cc:53: NOTREACHED()
<< "Unexpected notification " << type;
On 2012/03/05 16:16:03, flackr wrote:
> Remove text message, reason for hitting the NOTREACHED is obvious from code
and
> static text messages increase our binary size.

Done.

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/chromeos/opti...
File chrome/browser/chromeos/options2/desktop_background_observer.h (right):

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/chromeos/opti...
chrome/browser/chromeos/options2/desktop_background_observer.h:15: // the Aura
shell's DesktopBackgroundController class.
On 2012/03/05 16:16:03, flackr wrote:
> This class seems misplaced / unnecessary. Aura shell
DesktopBackgroundController
> could directly listen for background change events.

Done.

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options2/browser_options.js:114: if (cr.isChromeOS) {
On 2012/03/05 16:16:03, flackr wrote:
> Does cr.isChromeOS == if_def('use_aura')? Seems like it would be safer to
check
> for the existence of the button.

Done.

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options2/options.js (right):

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options2/options.js:126: [$('set-wallpaper')]);
On 2012/03/05 16:16:03, flackr wrote:
> This also concerns me, I'm pretty sure that options will be broken on ChromeOS
> non-aura (chromeos_gtk=1) and this won't be registered if we support an aura
> non-chromeos build.

Done
check the existence of SetWallpaperOptions here to be safer.

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:29:
#if defined(USE_AURA)
On 2012/03/05 16:16:03, flackr wrote:
> This is only included if USE_AURA is defined, so this check should be
> unnecessary.

Done.

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:30:
#include "ash/desktop_background/desktop_background_controller.h"
On 2012/03/05 16:16:03, flackr wrote:
> And I don't think you actually use this class in here.

The ash constant and functions, such as ash::kDefaultWallpaperCount, require
this header file. I moved these const and util functions in
ash/desktop_background/desktop_background_resources.h/cc files.

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

http://codereview.chromium.org/9580023/diff/6001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/options_ui2.cc:72: #include
"chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h"
On 2012/03/05 16:16:03, flackr wrote:
> This seems inconsistent with everywhere else where the wallpaper code is
> included on USE_AURA.

Done. Put everything behind ash.

[bshe, 2012-03-06 16:39:17]

Hi Rob.

Delta 5 is my implementation to save the index of wallpaper to local state. I
have tested it on linux desktop. It works fine. The user specific wallpaper will
draw when user login.

I am building an image now to verify it on stumpy.

Thanks!

[flackr, 2012-03-06 17:16:49]

Is custom image background support coming in a future iteration?

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
File ash/desktop_background/desktop_background_view.cc (right):

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_view.cc:34:
IDR_AURA_WALLPAPER_1).ToSkBitmap();
On 2012/03/06 01:33:27, bshe wrote:
> On 2012/03/05 16:16:03, flackr wrote:
> > If the user has a custom background set this will have to load the default
> > wallpaper and then change it. I think that you should pass the wallpaper to
> use
> > in the constructor for DesktopBackgroundView and then ash::Shell or the
> > DesktopBackgroundController can create this view with the appropriate
> wallpaper.
> 
> It seems this is constructed before NOTIFICATION_LOGIN_USER_CHANGED
> notification. I assume the custom background will only available after
> NOTIFICATION_LOGIN_USER_CHANGED notification(I could probably be wrong). If
this
> is the case, unless we delay the construction of this class after
> NOTIFICATION_LOGIN_USER_CHANGED, we can't initialize it with user custom
> background. I have tested delay the creation of DesktopBackgroundView after we
> observer the notification. But there seems to be a check failed in debug
build.
> (FATAL:login_performer.cc(226)] Check failed: false.) 
> I am thinking we can probably do nothing in the constructor at first and set
the
> wallpaper after user login as a walkaround.

It makes sense that this is constructed before the shell launches, the default
background it refers to should be standardized rather than having one default
listed here, and another in the user manager. If you make the other changes I'm
suggesting, you can refer to UserManager's default background to set the
background view.

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:14: #include
"grit/ui_resources.h"
nit: Sort

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:53:
Shell::GetInstance()->desktop_background_controller()
Shell::GetInstance()->desktop_background_controller() should be the same as
"this". Just directly call OnDesktopBackgroundChange(index);

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:18: class ASH_EXPORT
DesktopBackgroundController :
Comment the class and all non-override functions.

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:19: public
aura::RootWindowObserver,
This doesn't override any functions from RootWindowObserver, remove this.

http://codereview.chromium.org/9580023/diff/18001/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/9580023/diff/18001/ash/shell.cc#newcode555
ash/shell.cc:555: void Shell::SetDesktopBackgroundMode(BackgroundMode mode) {
I think this function should become SetDesktopBackgroundSolidColor() and
SetDesktopBackgroundWallpaper should change the desktop_background_mode_ if
necessary and set the background. This way if you currently have a solid color
set and select a background it will switch modes and set it. Also from a coding
perspective you can pass the background to set to the DesktopBackgroundView
constructor.

http://codereview.chromium.org/9580023/diff/18001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/9580023/diff/18001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager.h:108: // Stores index to the user
choosed defautl wallpaper in local state.
Sets the user wallpaper to the default wallpaper with index |wallpaper_index|,
sends WALLPAPER_CHANGED notification and updates Local State.

Of course replace WALLPAPER_CHANGED with the real notification name :-)

http://codereview.chromium.org/9580023/diff/18001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager.h:110: int wallpaper_index);
As with SaveUserDefaultImageIndex below, call this SaveWallpaperDefaultIndex,
you can mention local state in comment.

[bshe, 2012-03-06 19:56:15]

Done and inlined. Thanks!

Is custom image background support coming in a future iteration?
I believe so but not sure about the time line for it.

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
File ash/desktop_background/desktop_background_view.cc (right):

http://codereview.chromium.org/9580023/diff/6001/ash/desktop_background/deskt...
ash/desktop_background/desktop_background_view.cc:34:
IDR_AURA_WALLPAPER_1).ToSkBitmap();
On 2012/03/06 17:16:49, flackr wrote:
> On 2012/03/06 01:33:27, bshe wrote:
> > On 2012/03/05 16:16:03, flackr wrote:
> > > If the user has a custom background set this will have to load the default
> > > wallpaper and then change it. I think that you should pass the wallpaper
to
> > use
> > > in the constructor for DesktopBackgroundView and then ash::Shell or the
> > > DesktopBackgroundController can create this view with the appropriate
> > wallpaper.
> > 
> > It seems this is constructed before NOTIFICATION_LOGIN_USER_CHANGED
> > notification. I assume the custom background will only available after
> > NOTIFICATION_LOGIN_USER_CHANGED notification(I could probably be wrong). If
> this
> > is the case, unless we delay the construction of this class after
> > NOTIFICATION_LOGIN_USER_CHANGED, we can't initialize it with user custom
> > background. I have tested delay the creation of DesktopBackgroundView after
we
> > observer the notification. But there seems to be a check failed in debug
> build.
> > (FATAL:login_performer.cc(226)] Check failed: false.) 
> > I am thinking we can probably do nothing in the constructor at first and set
> the
> > wallpaper after user login as a walkaround.
> 
> It makes sense that this is constructed before the shell launches, the default
> background it refers to should be standardized rather than having one default
> listed here, and another in the user manager. If you make the other changes
I'm
> suggesting, you can refer to UserManager's default background to set the
> background view.

I am wondering is it also fine to dont set a specific wallpaper here(empty
constructor). And delay it after LOGIN_USER_CHANGED notification, and set the
wallpaper_ to user specific wallpaper through SetWallpaper function. This way,
there is only one standardized image as well and user dont have to load the
default wallpaper and then change it. 

I am not sure if wallpaper_ is used anywhere before the LOGIN_USER_CHANGED
notification though. On linux desktop build, it seems work fine for me.

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:14: #include
"grit/ui_resources.h"
On 2012/03/06 17:16:49, flackr wrote:
> nit: Sort

Done.

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:53:
Shell::GetInstance()->desktop_background_controller()
On 2012/03/06 17:16:49, flackr wrote:
> Shell::GetInstance()->desktop_background_controller() should be the same as
> "this". Just directly call OnDesktopBackgroundChange(index);

Done. The desktop_background_controller function in Shell will probably be used
in the future for customized wallpaper. Should I keep it in shell for future use
or just remove it for sanity?

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:18: class ASH_EXPORT
DesktopBackgroundController :
On 2012/03/06 17:16:49, flackr wrote:
> Comment the class and all non-override functions.

Done.

http://codereview.chromium.org/9580023/diff/18001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:19: public
aura::RootWindowObserver,
On 2012/03/06 17:16:49, flackr wrote:
> This doesn't override any functions from RootWindowObserver, remove this.

Done.

http://codereview.chromium.org/9580023/diff/18001/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/9580023/diff/18001/ash/shell.cc#newcode555
ash/shell.cc:555: void Shell::SetDesktopBackgroundMode(BackgroundMode mode) {
On 2012/03/06 17:16:49, flackr wrote:
> I think this function should become SetDesktopBackgroundSolidColor() and
> SetDesktopBackgroundWallpaper should change the desktop_background_mode_ if
> necessary and set the background. This way if you currently have a solid color
> set and select a background it will switch modes and set it. Also from a
coding
> perspective you can pass the background to set to the DesktopBackgroundView
> constructor.

Just to confirm, your intention here is to enable the ability to switch between
solid color mode and image mode?

My understanding is that the solid color mode is only used in compact window
mode now. Unless we want to switch from compact mode to managed mode
dynamically, we dont need switch between solid color to image. 

To set a solid color background in managed mode, the current plan is to create
some solid color resources/images and draw them on screen using
DesktopBackgroundView.

I agree for solid color background, we should use the solid color mode for
memory efficiency. Do you want push this into this CL or should I just add a
TODO for now?

http://codereview.chromium.org/9580023/diff/18001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/9580023/diff/18001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager.h:108: // Stores index to the user
choosed defautl wallpaper in local state.
On 2012/03/06 17:16:49, flackr wrote:
> Sets the user wallpaper to the default wallpaper with index |wallpaper_index|,
> sends WALLPAPER_CHANGED notification and updates Local State.
> 
> Of course replace WALLPAPER_CHANGED with the real notification name :-)

Done.

http://codereview.chromium.org/9580023/diff/18001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager.h:110: int wallpaper_index);
On 2012/03/06 17:16:49, flackr wrote:
> As with SaveUserDefaultImageIndex below, call this SaveWallpaperDefaultIndex,
> you can mention local state in comment.

Done.

[flackr, 2012-03-06 20:25:28]

I'm not sure what's happening with compact mode, but I realize this will show up
and will appear broken in that context. If we will still have compact mode in
R19 (which I think we will), you should remove wallpaper selection if the user
is running in compact mode.

http://codereview.chromium.org/9580023/diff/18001/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/9580023/diff/18001/ash/shell.cc#newcode555
ash/shell.cc:555: void Shell::SetDesktopBackgroundMode(BackgroundMode mode) {
On 2012/03/06 19:56:16, bshe wrote:
> On 2012/03/06 17:16:49, flackr wrote:
> > I think this function should become SetDesktopBackgroundSolidColor() and
> > SetDesktopBackgroundWallpaper should change the desktop_background_mode_ if
> > necessary and set the background. This way if you currently have a solid
color
> > set and select a background it will switch modes and set it. Also from a
> coding
> > perspective you can pass the background to set to the DesktopBackgroundView
> > constructor.
> 
> Just to confirm, your intention here is to enable the ability to switch
between
> solid color mode and image mode?
> 
> My understanding is that the solid color mode is only used in compact window
> mode now. Unless we want to switch from compact mode to managed mode
> dynamically, we dont need switch between solid color to image. 
> 
> To set a solid color background in managed mode, the current plan is to create
> some solid color resources/images and draw them on screen using
> DesktopBackgroundView.
> 
> I agree for solid color background, we should use the solid color mode for
> memory efficiency. Do you want push this into this CL or should I just add a
> TODO for now?

This is probably important, I think if we start supporting switching background
modes that this will break wallpaper switching (i.e. when you switch to solid
color mode selecting wallpapers won't do anything and when you switch back to
wallpaper mode it won't have a background until you select another wallpaper).
There is an accelerator in accelerator_controller.cc which changes background
modes on ctrl+alt+b if you compile with NDEBUG. You could test to see what
happens. I imagine this is okay as a TODO for now given that switching is not
allowed on release builds.

http://codereview.chromium.org/9580023/diff/20010/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/20010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:13: #include
"ui/aura/root_window_observer.h"
Remove this include as well.

[bshe, 2012-03-07 19:50:44]

Done and inlined. 
Thanks

http://codereview.chromium.org/9580023/diff/18001/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/9580023/diff/18001/ash/shell.cc#newcode555
ash/shell.cc:555: void Shell::SetDesktopBackgroundMode(BackgroundMode mode) {
On 2012/03/06 20:25:28, flackr wrote:
> On 2012/03/06 19:56:16, bshe wrote:
> > On 2012/03/06 17:16:49, flackr wrote:
> > > I think this function should become SetDesktopBackgroundSolidColor() and
> > > SetDesktopBackgroundWallpaper should change the desktop_background_mode_
if
> > > necessary and set the background. This way if you currently have a solid
> color
> > > set and select a background it will switch modes and set it. Also from a
> > coding
> > > perspective you can pass the background to set to the
DesktopBackgroundView
> > > constructor.
> > 
> > Just to confirm, your intention here is to enable the ability to switch
> between
> > solid color mode and image mode?
> > 
> > My understanding is that the solid color mode is only used in compact window
> > mode now. Unless we want to switch from compact mode to managed mode
> > dynamically, we dont need switch between solid color to image. 
> > 
> > To set a solid color background in managed mode, the current plan is to
create
> > some solid color resources/images and draw them on screen using
> > DesktopBackgroundView.
> > 
> > I agree for solid color background, we should use the solid color mode for
> > memory efficiency. Do you want push this into this CL or should I just add a
> > TODO for now?
> 
> This is probably important, I think if we start supporting switching
background
> modes that this will break wallpaper switching (i.e. when you switch to solid
> color mode selecting wallpapers won't do anything and when you switch back to
> wallpaper mode it won't have a background until you select another wallpaper).
> There is an accelerator in accelerator_controller.cc which changes background
> modes on ctrl+alt+b if you compile with NDEBUG. You could test to see what
> happens. I imagine this is okay as a TODO for now given that switching is not
> allowed on release builds.

It seems the ToT build can't switch the color mode back and forth. I will create
a bug to track this. At the same time, I have modified my code according to your
suggestion so when user switch color mode, the user selected image will draw.

http://codereview.chromium.org/9580023/diff/20010/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/20010/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:13: #include
"ui/aura/root_window_observer.h"
On 2012/03/06 20:25:28, flackr wrote:
> Remove this include as well.

Done.

[flackr, 2012-03-07 20:50:28]

This is looking pretty good. It's probably time to get a real reviewer to look
at it =)

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:36: wallpaper);
Does this fit on the previous line?

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.h:14: // webui handler.
Remove comment or change to TODO. Given that you have accessors for the
WallPaper, WallpaperThumbnail, you could probably add one for WallpaperCount and
remove these from being global variables.

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_view.h (right):

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_view.h:20: void SetWallpaper(const
SkBitmap& wallpaper);
This currently isn't used anymore, right?

http://codereview.chromium.org/9580023/diff/28001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/9580023/diff/28001/ash/shell.h#newcode156
ash/shell.h:156: void SetDesktopBackgrondImage(const SkBitmap& wallpaper);
s/Backgrond/Background

http://codereview.chromium.org/9580023/diff/28001/ash/shell.h#newcode158
ash/shell.h:158: // Sets teh desktop background (solid color mode).
s/teh/the

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:316: #if
defined(USE_AURA)
USE_ASH? For consistency.

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:379:
ash::Shell::GetInstance()->IsWindowModeCompact());
This (and the include of ash above) has to be in a #if defined(USE_ASH)

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:95:
if (image_index == previous_index_)
We should probably guard against this in WebUI and not bother here.

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:42:
void HandleSelectImage(const base::ListValue* args);
Comment.

[bshe, 2012-03-07 23:48:35]

Hi Rob.

Could you please take another look?

Also add real reviewers as you suggested :)

+ben for ash and ui part: ash/*
I used the pattern you suggested, use observer/controller to make communication
between ash and webui.

+james for webui part: */option2/*

Thanks!

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:36: wallpaper);
On 2012/03/07 20:50:28, flackr wrote:
> Does this fit on the previous line?

Done.

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.h:14: // webui handler.
On 2012/03/07 20:50:28, flackr wrote:
> Remove comment or change to TODO. Given that you have accessors for the
> WallPaper, WallpaperThumbnail, you could probably add one for WallpaperCount
and
> remove these from being global variables.

Put them behind internal namespace. Not sure if that's what you mean though...

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_view.h (right):

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_view.h:20: void SetWallpaper(const
SkBitmap& wallpaper);
On 2012/03/07 20:50:28, flackr wrote:
> This currently isn't used anymore, right?

I still use it for changing the wallpaper. It seems if I remove the previous one
and use the constructor to change the wallpaper. The background stop paint. This
is essentially the same bug why toggle color mode does not work. After switch
back to image mode, the background somehow also stop paint and give an illusion
that switch to image mode failed. I guess once the bug got fixed, I can remove
the function. So I should probably add a todo here. What do you think?

http://codereview.chromium.org/9580023/diff/28001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/9580023/diff/28001/ash/shell.h#newcode156
ash/shell.h:156: void SetDesktopBackgrondImage(const SkBitmap& wallpaper);
On 2012/03/07 20:50:28, flackr wrote:
> s/Backgrond/Background

Done.

http://codereview.chromium.org/9580023/diff/28001/ash/shell.h#newcode158
ash/shell.h:158: // Sets teh desktop background (solid color mode).
On 2012/03/07 20:50:28, flackr wrote:
> s/teh/the

Done.

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:316: #if
defined(USE_AURA)
On 2012/03/07 20:50:28, flackr wrote:
> USE_ASH? For consistency.

Done.

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:379:
ash::Shell::GetInstance()->IsWindowModeCompact());
On 2012/03/07 20:50:28, flackr wrote:
> This (and the include of ash above) has to be in a #if defined(USE_ASH)

Done.

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:95:
if (image_index == previous_index_)
On 2012/03/07 20:50:28, flackr wrote:
> We should probably guard against this in WebUI and not bother here.

It seems it's already done in webui. It listens to change event. So only when
index changed, this function will be called.

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

http://codereview.chromium.org/9580023/diff/28001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:42:
void HandleSelectImage(const base::ListValue* args);
On 2012/03/07 20:50:28, flackr wrote:
> Comment.

Done.

[flackr, 2012-03-08 01:19:58]

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.h:14: // webui handler.
On 2012/03/07 23:48:35, bshe wrote:
> On 2012/03/07 20:50:28, flackr wrote:
> > Remove comment or change to TODO. Given that you have accessors for the
> > WallPaper, WallpaperThumbnail, you could probably add one for WallpaperCount
> and
> > remove these from being global variables.
> 
> Put them behind internal namespace. Not sure if that's what you mean though...

Nope. What I meant was that the functions give you all of these constants so
they can be removed from the header file and only defined in the .cc file in an
anonymous namespace (see comment in .cc file).

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_view.h (right):

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_view.h:20: void SetWallpaper(const
SkBitmap& wallpaper);
On 2012/03/07 23:48:35, bshe wrote:
> On 2012/03/07 20:50:28, flackr wrote:
> > This currently isn't used anymore, right?
> 
> I still use it for changing the wallpaper. It seems if I remove the previous
one
> and use the constructor to change the wallpaper. The background stop paint.
This
> is essentially the same bug why toggle color mode does not work. After switch
> back to image mode, the background somehow also stop paint and give an
illusion
> that switch to image mode failed. I guess once the bug got fixed, I can remove
> the function. So I should probably add a todo here. What do you think?

Sounds good to me.

http://codereview.chromium.org/9580023/diff/27022/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/9580023/diff/27022/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:205:
shell->SetDesktopBackgroundImageMode(shell->
This would be a good place for just indenting 4 spaces on the next line. Might
save a line and looks neater.

http://codereview.chromium.org/9580023/diff/27022/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.cc (right):

http://codereview.chromium.org/9580023/diff/27022/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.cc:15: namespace internal {
You can remove name from namespace:
namespace {

Also, move this namespace block before the namespace ash {.

http://codereview.chromium.org/9580023/diff/27022/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.cc:43: }
}  // namespace

http://codereview.chromium.org/9580023/diff/27022/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/9580023/diff/27022/ash/shell.cc#newcode682
ash/shell.cc:682: GetPresetWallpaper());
Indent 4 more spaces as this isn't another argument to
SetDesktopBackgroundImageMode but a continuation of previous line.

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:9: #include
"ash/shell.h"
This too ^

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:91:
*Value::CreateIntegerValue(index));
This is a memory leak. You can use the FundamentalValue(int) constructor to make
an integer value.

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:106:
int image_index = atoi(image_index_string.c_str());
Use StringToInt from string_number_conversions.h, add this conversion to above
check for error conditions so that it will hit NOTREACHED and return if it can't
convert the string.

[bshe, 2012-03-08 03:19:23]

Done. Thanks!

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/9580023/diff/28001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.h:14: // webui handler.
On 2012/03/08 01:19:58, flackr wrote:
> On 2012/03/07 23:48:35, bshe wrote:
> > On 2012/03/07 20:50:28, flackr wrote:
> > > Remove comment or change to TODO. Given that you have accessors for the
> > > WallPaper, WallpaperThumbnail, you could probably add one for
WallpaperCount
> > and
> > > remove these from being global variables.
> > 
> > Put them behind internal namespace. Not sure if that's what you mean
though...
> 
> Nope. What I meant was that the functions give you all of these constants so
> they can be removed from the header file and only defined in the .cc file in
an
> anonymous namespace (see comment in .cc file).

Done.

http://codereview.chromium.org/9580023/diff/27022/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/9580023/diff/27022/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:205:
shell->SetDesktopBackgroundImageMode(shell->
On 2012/03/08 01:19:59, flackr wrote:
> This would be a good place for just indenting 4 spaces on the next line. Might
> save a line and looks neater.

Done.

http://codereview.chromium.org/9580023/diff/27022/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.cc (right):

http://codereview.chromium.org/9580023/diff/27022/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.cc:15: namespace internal {
On 2012/03/08 01:19:59, flackr wrote:
> You can remove name from namespace:
> namespace {
> 
> Also, move this namespace block before the namespace ash {.

Done.

http://codereview.chromium.org/9580023/diff/27022/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.cc:43: }
On 2012/03/08 01:19:59, flackr wrote:
> }  // namespace

Done.

http://codereview.chromium.org/9580023/diff/27022/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/9580023/diff/27022/ash/shell.cc#newcode682
ash/shell.cc:682: GetPresetWallpaper());
On 2012/03/08 01:19:59, flackr wrote:
> Indent 4 more spaces as this isn't another argument to
> SetDesktopBackgroundImageMode but a continuation of previous line.

Done.

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:9: #include
"ash/shell.h"
On 2012/03/08 01:19:59, flackr wrote:
> This too ^

Done.

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:91:
*Value::CreateIntegerValue(index));
On 2012/03/08 01:19:59, flackr wrote:
> This is a memory leak. You can use the FundamentalValue(int) constructor to
make
> an integer value.

Done.

http://codereview.chromium.org/9580023/diff/27022/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:106:
int image_index = atoi(image_index_string.c_str());
On 2012/03/08 01:19:59, flackr wrote:
> Use StringToInt from string_number_conversions.h, add this conversion to above
> check for error conditions so that it will hit NOTREACHED and return if it
can't
> convert the string.

Done.

[flackr, 2012-03-08 03:31:58]

LGTM

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.h:15: const SkBitmap&
GetDefaultWallpaperThumbnail(int index);
Only thing I might suggest is putting this in an inner namespace (like
ash::backgrounds) to avoid having 4 new names in the ash namespace. I'm not sure
if this is common practice though.

[James Hawkins, 2012-03-08 14:30:39]

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.js
(right):

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:42:
this.handleImageSelected_.bind(this));
Indentation is off.

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:73:
closePage_: function() {
Why do you need this method?  Just use OptionsPage.closeOverlay().

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:126: for
(var i = 0, url; url = images[i]; i++) {
nit: No braces.

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:104:
return;
NOTREACHED() should not be handled (by returning).

[Ben Goodger (Google), 2012-03-08 16:35:01]

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:11: #include
"content/public/browser/notification_observer.h"
Ash can't depend on content.

[bshe, 2012-03-08 16:36:40]

Done and inlined. 

Thanks!

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.h:15: const SkBitmap&
GetDefaultWallpaperThumbnail(int index);
On 2012/03/08 03:31:58, flackr wrote:
> Only thing I might suggest is putting this in an inner namespace (like
> ash::backgrounds) to avoid having 4 new names in the ash namespace. I'm not
sure
> if this is common practice though.

+ben

It sounds good to me. But probably leave this decision to Ben since we are not
sure? :)

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.js
(right):

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:42:
this.handleImageSelected_.bind(this));
On 2012/03/08 14:30:39, James Hawkins wrote:
> Indentation is off.

Done.

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:73:
closePage_: function() {
On 2012/03/08 14:30:39, James Hawkins wrote:
> Why do you need this method?  Just use OptionsPage.closeOverlay().

There are three different places use this and one of them is an event handler
function. I thought it might worth it to have a method here. But removing this
method sounds good to me.
Done.

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:126: for
(var i = 0, url; url = images[i]; i++) {
On 2012/03/08 14:30:39, James Hawkins wrote:
> nit: No braces.

Done.

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:104:
return;
On 2012/03/08 14:30:39, James Hawkins wrote:
> NOTREACHED() should not be handled (by returning).

+james

Could you please be more specific here? It seems there are already some places
use NOTREACHED() followed by a return in the code base. I am not sure why we
shouldn't return after these checks failed. Or I understand your suggestion in a
wrong way?

[James Hawkins, 2012-03-08 17:12:07]

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/9580023/diff/46009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:104:
return;
On 2012/03/08 16:36:40, bshe wrote:
> On 2012/03/08 14:30:39, James Hawkins wrote:
> > NOTREACHED() should not be handled (by returning).
> 
> +james
> 
> Could you please be more specific here? It seems there are already some places
> use NOTREACHED() followed by a return in the code base. I am not sure why we
> shouldn't return after these checks failed. Or I understand your suggestion in
a
> wrong way?

The existing instances of this are wrong and should be fixed over time.

http://www.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

[bshe, 2012-03-08 19:55:48]

Done and inlined.

+james
Thanks for the link! I have removed the return statement.

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:11: #include
"content/public/browser/notification_observer.h"
On 2012/03/08 16:35:01, Ben Goodger (Google) wrote:
> Ash can't depend on content.

I moved the observer code to
chrome/browser/chromeos/background/desktop_background_observer.h and initialize
this observer in the PreProfileInit function in chrome_browser_main_chromeos.cc
file. Not sure this is the best place to put the observer though. Do you have
any suggestion? Thanks.

[James Hawkins, 2012-03-08 21:00:06]

lgtm

[bshe, 2012-03-09 17:08:31]

+ben

ping? Could you please take a look at this CL? 
Everything under ash/* and 
chrome/browser/chromeos/* for the observer code

Thanks!

For some context:

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/46009/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:11: #include
"content/public/browser/notification_observer.h"
On 2012/03/08 16:35:01, Ben Goodger (Google) wrote:
> Ash can't depend on content.

I moved the observer code to
chrome/browser/chromeos/background/desktop_background_observer.h and initialize
this observer in the PreProfileInit function in chrome_browser_main_chromeos.cc
file. Not sure this is the best place to put the observer though. Do you have
any suggestion? Thanks.

[Ben Goodger (Google), 2012-03-09 23:20:17]

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:32: 
function order in .cc should match .h

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:26: const SkBitmap&
GetPresetWallpaper();
GetDefaultWallpaper

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:29: const SkBitmap&
GetPreviouseWallpaper();
Describe what the previous wallpaper is used for. Also, remove the second e from
the function name.

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:32: 
kill the space

http://codereview.chromium.org/9580023/diff/65001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/9580023/diff/65001/ash/shell.h#newcode139
ash/shell.h:139: // Sets the desktop background wallpaper.
Can we pull all of this off onto a DesktopBackgroundController that the shell
owns, rather than just adding to the shell api

http://codereview.chromium.org/9580023/diff/65001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/9580023/diff/65001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:867:
chrome::NOTIFICATION_DESKTOP_BACKGROUND_CHANGED,
can you not use notifications for this at all... rather use an api ash exposes
to change the wallpaper.

[bshe, 2012-03-12 00:14:24]

Done and inlined. Thanks!

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:32: 
On 2012/03/09 23:20:17, Ben Goodger (Google) wrote:
> function order in .cc should match .h

Done.

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:26: const SkBitmap&
GetPresetWallpaper();
On 2012/03/09 23:20:17, Ben Goodger (Google) wrote:
> GetDefaultWallpaper

Done.

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:29: const SkBitmap&
GetPreviouseWallpaper();
On 2012/03/09 23:20:17, Ben Goodger (Google) wrote:
> Describe what the previous wallpaper is used for. Also, remove the second e
from
> the function name.

Done. Add comments above definition of variable previouse_wallpaper_index_.

http://codereview.chromium.org/9580023/diff/65001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:32: 
On 2012/03/09 23:20:17, Ben Goodger (Google) wrote:
> kill the space

Done.

http://codereview.chromium.org/9580023/diff/65001/ash/shell.h
File ash/shell.h (right):

http://codereview.chromium.org/9580023/diff/65001/ash/shell.h#newcode139
ash/shell.h:139: // Sets the desktop background wallpaper.
On 2012/03/09 23:20:17, Ben Goodger (Google) wrote:
> Can we pull all of this off onto a DesktopBackgroundController that the shell
> owns, rather than just adding to the shell api

Done. I am not sure if you mean to pull the following two functions as well. It
makes sense to me to pull them to DesktopBackgroundController as well, since
they also control background. But I could be wrong. And I am happy to move them
back if I am wrong. Thanks!

http://codereview.chromium.org/9580023/diff/65001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/9580023/diff/65001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:867:
chrome::NOTIFICATION_DESKTOP_BACKGROUND_CHANGED,
On 2012/03/09 23:20:17, Ben Goodger (Google) wrote:
> can you not use notifications for this at all... rather use an api ash exposes
> to change the wallpaper.

Done.

[Ben Goodger (Google), 2012-03-12 16:34:30]

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:33: void
OnDesktopBackgroundChange(int index);
Changed

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.cc (right):

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.cc:16: IDR_AURA_WALLPAPER_1,
IDR_ASH_WALLPAPER

[bshe, 2012-03-12 17:02:41]

Done and inlined.

Thanks!

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:33: void
OnDesktopBackgroundChange(int index);
On 2012/03/12 16:34:30, Ben Goodger (Google) wrote:
> Changed

Done.

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_resources.cc (right):

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.cc:16: IDR_AURA_WALLPAPER_1,
On 2012/03/12 16:34:30, Ben Goodger (Google) wrote:
> IDR_ASH_WALLPAPER

Done. I will update the binary file names once the default wallpapers are
chosen.

[tfarina, 2012-03-12 17:07:28]

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:11: #include
"third_party/skia/include/core/SkBitmap.h"
nit: you can forward declare this.

[tfarina, 2012-03-12 17:24:32]

http://codereview.chromium.org/9580023/diff/67003/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/9580023/diff/67003/ash/shell.cc#newcode12
ash/shell.cc:12: #include
"ash/desktop_background/desktop_background_resources.h"
do you really need to include desktop_background_resources.h and
desktop_background_view.h?

[bshe, 2012-03-12 17:27:30]

+tfarina

Forward declared.
Thanks!

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/9580023/diff/61025/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:11: #include
"third_party/skia/include/core/SkBitmap.h"
On 2012/03/12 17:07:28, tfarina wrote:
> nit: you can forward declare this.

Done.

[Ben Goodger (Google), 2012-03-12 17:57:17]

LG

[Ben Goodger (Google), 2012-03-12 17:57:22]

lgtm

[commit-bot: I haz the power, 2012-03-12 18:28:31]

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

[commit-bot: I haz the power, 2012-03-12 18:28:38]

Can't apply patch for file ash/accelerators/accelerator_controller.cc.
While running patch -p1 --forward --force;
patching file ash/accelerators/accelerator_controller.cc
Hunk #1 FAILED at 4.
Hunk #2 succeeded at 87 (offset -113 lines).
1 out of 2 hunks FAILED -- saving rejects to file
ash/accelerators/accelerator_controller.cc.rej

[commit-bot: I haz the power, 2012-03-12 18:38:06]

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

[commit-bot: I haz the power, 2012-03-12 19:02:28]

Try job failure for 9580023-70002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-03-13 00:17:10]

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

[commit-bot: I haz the power, 2012-03-13 00:17:17]

Can't apply patch for file ash/accelerators/accelerator_controller.cc.
While running patch -p1 --forward --force;
patching file ash/accelerators/accelerator_controller.cc
Hunk #1 FAILED at 5.
Hunk #2 succeeded at 90 (offset 3 lines).
1 out of 2 hunks FAILED -- saving rejects to file
ash/accelerators/accelerator_controller.cc.rej

[commit-bot: I haz the power, 2012-03-13 00:22:05]

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

[commit-bot: I haz the power, 2012-03-13 03:32:50]

Change committed as 126319