2nd display should show the same background as login/lock screen:

2nd display should show the same background as login/lock screen:

Added special container for background on lock screen.
Background view can now be created in specific container.
Disabled lock screen wallpaper implementation based on serving PNG image via data source.

BUG=136853, 137581
TEST=Lock screen on multimonitor configuration. Check that windows on secondary display are hidden with background.

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

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

[Denis Kuznetsov (DE-MUC), 2012-07-20 16:07:09]

Ben, look on ash/ part.

Nikita, look on chrome/ part.

[Denis Kuznetsov (DE-MUC), 2012-07-20 16:08:18]

s/Ben/Sky/

[Nikita (slow), 2012-07-20 18:39:58]

+Biao

[sky, 2012-07-20 23:57:04]

Can we make chromeos not have to call into ash to manipulate the background.
Instead chromeos should tell ash the screen has been locked, at which point it
installs the necessary backgrounds.

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.cc:205: 
nit: remove newline.

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.cc:264: if
(BACKGROUND_IMAGE == desktop_background_mode_) {
spacing is off in the rest of this function.

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.cc:280:
Shell::RootWindowList root_windows = Shell::GetAllRootWindows();
spacing is off.

http://codereview.chromium.org/10810039/diff/1/ash/wm/root_window_layout_mana...
File ash/wm/root_window_layout_manager.h (right):

http://codereview.chromium.org/10810039/diff/1/ash/wm/root_window_layout_mana...
ash/wm/root_window_layout_manager.h:71: views::Widget*
background_widget_[kShellWindowCount];
I don't understand this. Why do we need a background widget/layer per container?

[bshe, 2012-07-23 00:19:36]

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.cc:260: void
DesktopBackgroundController::CreateLockScreenDesktops() {
I am guessing you plan use the new container to replace the data source solution
you previously implement for wallpaper at lock screen? In that case, do we still
need the post processed wallpaper? It seems the new lock screen container will
paint exactly the way DesktopBackgroundContainer did.

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.h:121: int container_id);
Nit: Could you make the last two argument align with the first argument?

[Denis Kuznetsov (DE-MUC), 2012-07-23 14:01:08]

>Can we make chromeos not have to call into ash to manipulate the background.
>Instead chromeos should tell ash the screen has been locked, at which point it
installs the necessary backgrounds.
I've just tried to follow the pattern - as for now,
desktop_background_controller is a single way to manipulate background from
chrome (see user_manager_impl.cc, wallpaper_manager.cc,
set_wallpaper_options_handler.cc), with ash_user_wallpaper_delegate.cc for
reverse calls.
I'm still somewhat new to chrome team, and I'm not feeling sure enough to handle
this refactoring.

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.cc:205: 
On 2012/07/20 23:57:04, sky wrote:
> nit: remove newline.

Done.

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.cc:260: void
DesktopBackgroundController::CreateLockScreenDesktops() {
On 2012/07/23 00:19:36, bshe wrote:
> I am guessing you plan use the new container to replace the data source
solution
> you previously implement for wallpaper at lock screen? In that case, do we
still
> need the post processed wallpaper? It seems the new lock screen container will
> paint exactly the way DesktopBackgroundContainer did.
Yes, this is the plan. However, there is an issue with graphics acceleration
(that is required for transparency in webUI), that costs us about 700ms on
startup. So we leave data source solution in code as a backup plan.
So that task is lower priority now, but it can not be dropped yet.

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.cc:264: if
(BACKGROUND_IMAGE == desktop_background_mode_) {
On 2012/07/20 23:57:04, sky wrote:
> spacing is off in the rest of this function.

Done.

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.cc:280:
Shell::RootWindowList root_windows = Shell::GetAllRootWindows();
On 2012/07/20 23:57:04, sky wrote:
> spacing is off.

Done.

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.h:121: int container_id);
On 2012/07/23 00:19:36, bshe wrote:
> Nit: Could you make the last two argument align with the first argument?

Done.

http://codereview.chromium.org/10810039/diff/1/ash/wm/root_window_layout_mana...
File ash/wm/root_window_layout_manager.h (right):

http://codereview.chromium.org/10810039/diff/1/ash/wm/root_window_layout_mana...
ash/wm/root_window_layout_manager.h:71: views::Widget*
background_widget_[kShellWindowCount];
On 2012/07/20 23:57:04, sky wrote:
> I don't understand this. Why do we need a background widget/layer per
container?
I'd say we have a kind of technical debt here: it is actually a layout manager,
and it should not know anything about Background - only about the components to
lay off.
However, existing code names these components as "Background", have just one of
each type, and handles lifetime management for them (using scoped_ptr /
CloseNow()).

On my side of changes, I need to add two more possible objects (Widget/Layer) to
existing code. I can do it by either using named fields (and introducing new set
methods), or by adding some identifier parameter to existing methods.
I already have "natural" identifier - id of container. I could introduce some
another id, and keep mapping at some place in code, but I'm not sure it would
make code more clear (although we have about 200 bytes overhead with this
solution).

I can rename existing methods to remove confusing "background" name from them
(e.g. replace it with "FullScreen"/"FullWindow"), and make memory management a
bit more clear in code.

I already have natural ID

[bshe, 2012-07-23 14:36:28]

wallpaper lgtm

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
ash/desktop_background/desktop_background_controller.cc:260: void
DesktopBackgroundController::CreateLockScreenDesktops() {
Got it. I will drop the priority of post processed wallpaper for now. Actually,
it seems there is no reliable way to get a screen size bitmap from desktop
background view. I originally thought I can use the canvas object in OnPaint
function to extract the bitmap. But it seems we can't actually rely on canvas
since it may only draw a tile of the bitmap at one time. So we probably need
some ugly hacks to get the screen size bitmap from desktop background view. I
will only work on it if it becomes real necessary. 

On 2012/07/23 14:01:09, Denis Kuznetsov wrote:
> On 2012/07/23 00:19:36, bshe wrote:
> > I am guessing you plan use the new container to replace the data source
> solution
> > you previously implement for wallpaper at lock screen? In that case, do we
> still
> > need the post processed wallpaper? It seems the new lock screen container
will
> > paint exactly the way DesktopBackgroundContainer did.
> Yes, this is the plan. However, there is an issue with graphics acceleration
> (that is required for transparency in webUI), that costs us about 700ms on
> startup. So we leave data source solution in code as a backup plan.
> So that task is lower priority now, but it can not be dropped yet.

[sky, 2012-07-23 16:10:46]

On Mon, Jul 23, 2012 at 7:01 AM,  <antrim@chromium.org> wrote:
>> Can we make chromeos not have to call into ash to manipulate the
>> background.
>> Instead chromeos should tell ash the screen has been locked, at which
>> point it
>
> installs the necessary backgrounds.
> I've just tried to follow the pattern - as for now,
> desktop_background_controller is a single way to manipulate background from
> chrome (see user_manager_impl.cc, wallpaper_manager.cc,
> set_wallpaper_options_handler.cc), with ash_user_wallpaper_delegate.cc for
> reverse calls.
> I'm still somewhat new to chrome team, and I'm not feeling sure enough to
> handle
> this refactoring.

Could you do the refactoring before landing this change then? It
should make things simpler, right?

>
>
>
http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
> File ash/desktop_background/desktop_background_controller.cc (right):
>
>
http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
> ash/desktop_background/desktop_background_controller.cc:205:
> On 2012/07/20 23:57:04, sky wrote:
>>
>> nit: remove newline.
>
>
> Done.
>
>
>
http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
> ash/desktop_background/desktop_background_controller.cc:260: void
> DesktopBackgroundController::CreateLockScreenDesktops() {
> On 2012/07/23 00:19:36, bshe wrote:
>>
>> I am guessing you plan use the new container to replace the data
>
> source solution
>>
>> you previously implement for wallpaper at lock screen? In that case,
>
> do we still
>>
>> need the post processed wallpaper? It seems the new lock screen
>
> container will
>>
>> paint exactly the way DesktopBackgroundContainer did.
>
> Yes, this is the plan. However, there is an issue with graphics
> acceleration (that is required for transparency in webUI), that costs us
> about 700ms on startup. So we leave data source solution in code as a
> backup plan.
> So that task is lower priority now, but it can not be dropped yet.
>
>
>
http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
> ash/desktop_background/desktop_background_controller.cc:264: if
> (BACKGROUND_IMAGE == desktop_background_mode_) {
> On 2012/07/20 23:57:04, sky wrote:
>>
>> spacing is off in the rest of this function.
>
>
> Done.
>
>
>
http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
> ash/desktop_background/desktop_background_controller.cc:280:
> Shell::RootWindowList root_windows = Shell::GetAllRootWindows();
> On 2012/07/20 23:57:04, sky wrote:
>>
>> spacing is off.
>
>
> Done.
>
>
>
http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
> File ash/desktop_background/desktop_background_controller.h (right):
>
>
http://codereview.chromium.org/10810039/diff/1/ash/desktop_background/desktop...
> ash/desktop_background/desktop_background_controller.h:121: int
> container_id);
> On 2012/07/23 00:19:36, bshe wrote:
>>
>> Nit: Could you make the last two argument align with the first
>
> argument?
>
> Done.
>
>
>
http://codereview.chromium.org/10810039/diff/1/ash/wm/root_window_layout_mana...
> File ash/wm/root_window_layout_manager.h (right):
>
>
http://codereview.chromium.org/10810039/diff/1/ash/wm/root_window_layout_mana...
> ash/wm/root_window_layout_manager.h:71: views::Widget*
> background_widget_[kShellWindowCount];
> On 2012/07/20 23:57:04, sky wrote:
>>
>> I don't understand this. Why do we need a background widget/layer per
>
> container?
> I'd say we have a kind of technical debt here: it is actually a layout
> manager, and it should not know anything about Background - only about
> the components to lay off.
> However, existing code names these components as "Background", have just
> one of each type, and handles lifetime management for them (using
> scoped_ptr / CloseNow()).
>
> On my side of changes, I need to add two more possible objects
> (Widget/Layer) to existing code. I can do it by either using named
> fields (and introducing new set methods), or by adding some identifier
> parameter to existing methods.
> I already have "natural" identifier - id of container. I could introduce
> some another id, and keep mapping at some place in code, but I'm not
> sure it would make code more clear (although we have about 200 bytes
> overhead with this solution).
>
> I can rename existing methods to remove confusing "background" name from
> them (e.g. replace it with "FullScreen"/"FullWindow"), and make memory
> management a bit more clear in code.
>
> I already have natural ID

I still don't understand why you need 2 backgrounds. Additionally your
patch effectively allows for kShellWindowCount backgrounds. We never
want to support that many backgrounds, so I'm hesitant to open up a
generic API like you're proposing.

  -Scott

[Denis Kuznetsov (DE-MUC), 2012-07-25 17:05:24]

> I still don't understand why you need 2 backgrounds. Additionally your
> patch effectively allows for kShellWindowCount backgrounds. We never
> want to support that many backgrounds, so I'm hesitant to open up a
> generic API like you're proposing.
> 
>   -Scott

Refactored code -- background components are reused now, and just reparented on
lock/unlock.

[Nikita (slow), 2012-07-25 20:45:24]

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
File ash/desktop_background/desktop_background_component.cc (right):

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_component.cc:33: if (layer_.get()) {
nit: no need for {}

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
File ash/desktop_background/desktop_background_component.h (right):

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_component.h:20:
DesktopBackgroundComponent(views::Widget* widget);
explicit

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_component.h:21:
DesktopBackgroundComponent(ui::Layer* layer);
explicit

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_component.h:21:
DesktopBackgroundComponent(ui::Layer* layer);
Please add comment that would clarify that layer ownership is transferred.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:100: //Install wallpaper
component for all available windows.
nit: insert space between // and comment text.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:205: container_id);
nit: indentation is off

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:211: root_window,
nit: indentation

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:216: }
default:
  NOTREACHED();

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:224: ui::Layer*
background_layer = new ui::Layer(ui::LAYER_SOLID_COLOR);
Indentation is off.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:237:
Shell::RootWindowList root_windows = Shell::GetAllRootWindows();
237-240 could be extracted to a separate function.
261-263

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:239: iter !=
root_windows.end(); ++iter)
nit: add {}

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:246: iter !=
root_windows.end(); ++iter) {
nit: Fix indent.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:262: iter !=
root_windows.end(); ++iter)
nit: Add {}

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:280: 
nit: nuke empty line

http://codereview.chromium.org/10810039/diff/13005/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

http://codereview.chromium.org/10810039/diff/13005/ash/shell_window_ids.h#new...
ash/shell_window_ids.h:61: const int
kShellWindowId_LockScreenBackgroundContainer = 12;
Add to shell_unittest.cc

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
File ash/wm/root_window_layout_manager.cc (right):

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.cc:6: #include
"ash/desktop_background/desktop_background_component.h"
nit: This header should go with the next group.

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.cc:42: internal::DesktopBackgroundComponent*
component = owner_->
nit: Place owner_->GetProperty() on the same line.

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
File ash/wm/root_window_layout_manager.h (right):

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.h:38: return
background_widget_[kShellWindowId_DesktopBackgroundContainer];
nit: fix indentation

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.h:40: ui::Layer* background_layer() {
nit: fix indentation

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.h:41: return
background_layer_[kShellWindowId_DesktopBackgroundContainer].get();
Makes sense to move this implementation to .cc

http://codereview.chromium.org/10810039/diff/13005/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/screen_account_picker.js (right):

http://codereview.chromium.org/10810039/diff/13005/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_account_picker.js:173:
//TODO(antrim): remove whole method once 136853 is accepted.
nit: add space betweeb // and TODO

[sky, 2012-07-25 20:47:36]

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
File ash/desktop_background/desktop_background_component.cc (right):

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
ash/desktop_background/desktop_background_component.cc:46: void
DesktopBackgroundComponent::Reparent(aura::RootWindow* root_window ,
remove white space

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
File ash/desktop_background/desktop_background_component.h (right):

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
ash/desktop_background/desktop_background_component.h:17: // effective
Layer-based for solid color, and Widget-based for images.
Change the implementation so we use a Widget for both. When you init the Widget
you can specify the the layer type in the initparams. This will simplify the
code and make it less error prone.

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
ash/desktop_background/desktop_background_component.h:18: class
DesktopBackgroundComponent {
Name this DesktopBackgroundWidgetController

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
ash/desktop_background/desktop_background_component.h:20:
DesktopBackgroundComponent(views::Widget* widget);
explicit

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
File ash/desktop_background/desktop_background_controller.cc (right):

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.cc:100: //Install wallpaper
component for all available windows.
nit: space after //

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.cc:285: void
DesktopBackgroundController::CreateLockScreenDesktops() {
Position should match that of header.

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.cc:288: locked_ = true;
Why do we need locked_ cached in here?

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/wm/root_window...
File ash/wm/root_window_layout_manager.h (right):

https://chromiumcodereview.appspot.com/10810039/diff/13005/ash/wm/root_window...
ash/wm/root_window_layout_manager.h:58: views::Widget*
background_widget_[kShellWindowCount];
Do we need these changes?

[Denis Kuznetsov (DE-MUC), 2012-07-26 13:51:12]

Made all fixes (mostly nits) except Scott's suggestion to use Widget instead of
Layer. I will commit that later (maybe as a separate CL) so that I don't block
Oshima.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
File ash/desktop_background/desktop_background_component.cc (right):

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_component.cc:33: if (layer_.get()) {
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: no need for {}

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_component.cc:46: void
DesktopBackgroundComponent::Reparent(aura::RootWindow* root_window ,
On 2012/07/25 20:47:36, sky wrote:
> remove white space

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
File ash/desktop_background/desktop_background_component.h (right):

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_component.h:18: class
DesktopBackgroundComponent {
On 2012/07/25 20:47:36, sky wrote:
> Name this DesktopBackgroundWidgetController

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_component.h:20:
DesktopBackgroundComponent(views::Widget* widget);
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> explicit

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_component.h:21:
DesktopBackgroundComponent(ui::Layer* layer);
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> explicit

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:100: //Install wallpaper
component for all available windows.
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: insert space between // and comment text.

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:205: container_id);
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: indentation is off

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:211: root_window,
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: indentation

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:216: }
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> default:
>   NOTREACHED();

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:224: ui::Layer*
background_layer = new ui::Layer(ui::LAYER_SOLID_COLOR);
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> Indentation is off.

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:237:
Shell::RootWindowList root_windows = Shell::GetAllRootWindows();
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> 237-240 could be extracted to a separate function.
> 261-263

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:239: iter !=
root_windows.end(); ++iter)
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: add {}

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:246: iter !=
root_windows.end(); ++iter) {
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: Fix indent.

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:246: iter !=
root_windows.end(); ++iter) {
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: Fix indent.

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:262: iter !=
root_windows.end(); ++iter)
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: Add {}

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:280: 
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: nuke empty line

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:285: void
DesktopBackgroundController::CreateLockScreenDesktops() {
On 2012/07/25 20:47:36, sky wrote:
> Position should match that of header.

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:288: locked_ = true;
On 2012/07/25 20:47:36, sky wrote:
> Why do we need locked_ cached in here?
Because we want to add ui components to correct container, if number of monitors
change while screen is locked (see InstallComponent call from OnRootWindowAdded)

http://codereview.chromium.org/10810039/diff/13005/ash/shell_window_ids.h
File ash/shell_window_ids.h (right):

http://codereview.chromium.org/10810039/diff/13005/ash/shell_window_ids.h#new...
ash/shell_window_ids.h:61: const int
kShellWindowId_LockScreenBackgroundContainer = 12;
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> Add to shell_unittest.cc
Already done, line 61.

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
File ash/wm/root_window_layout_manager.cc (right):

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.cc:6: #include
"ash/desktop_background/desktop_background_component.h"
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: This header should go with the next group.

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.cc:42: internal::DesktopBackgroundComponent*
component = owner_->
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: Place owner_->GetProperty() on the same line.

Done.

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
File ash/wm/root_window_layout_manager.h (right):

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.h:38: return
background_widget_[kShellWindowId_DesktopBackgroundContainer];
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: fix indentation
Removed as not needed.

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.h:40: ui::Layer* background_layer() {
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: fix indentation
Removed as not needed.

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.h:41: return
background_layer_[kShellWindowId_DesktopBackgroundContainer].get();
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> Makes sense to move this implementation to .cc
Removed as not needed.

http://codereview.chromium.org/10810039/diff/13005/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.h:58: views::Widget*
background_widget_[kShellWindowCount];
On 2012/07/25 20:47:36, sky wrote:
> Do we need these changes?

Done.

http://codereview.chromium.org/10810039/diff/13005/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/screen_account_picker.js (right):

http://codereview.chromium.org/10810039/diff/13005/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_account_picker.js:173:
//TODO(antrim): remove whole method once 136853 is accepted.
On 2012/07/25 20:45:24, Nikita Kostylev wrote:
> nit: add space betweeb // and TODO

Done.

[Nikita (slow), 2012-07-28 01:25:25]

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
File ash/desktop_background/desktop_background_controller.cc (right):

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:7: #include
"ash/desktop_background/desktop_background_widget_controller.h"
nit: sort

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:100: // Install
wallpaper component for all available windows.
nit: Not method name speaks for itself, comment should be removed.

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:185: iter !=
root_windows.end(); ++iter) {
nit: Indentation is not fixed. Should be 

for (Shell::RootWindowList::iterator iter = root_windows.begin();
     iter != root_windows.end(); ++iter) {

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:279: default:
nit: Should also have {}
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:288: iter !=
root_windows.end(); ++iter) {
nit: +1 indent space

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_controller.cc:309: return  locked ?
internal::kShellWindowId_LockScreenBackgroundContainer :
nit: Remove extra space.

http://codereview.chromium.org/10810039/diff/11002/ash/wm/root_window_layout_...
File ash/wm/root_window_layout_manager.cc (right):

http://codereview.chromium.org/10810039/diff/11002/ash/wm/root_window_layout_...
ash/wm/root_window_layout_manager.cc:42:
internal::DesktopBackgroundWidgetController* background = owner_->GetProperty(
Sorry, I wasn't clear enough. This code is more readable:

  internal::DesktopBackgroundWidgetController* background =
      owner_->GetProperty(internal::kWindowDesktopComponent);

[Nikita (slow), 2012-07-28 01:32:16]

lgtm with all those nits

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
File ash/desktop_background/desktop_background_widget_controller.cc (right):

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_widget_controller.cc:20: :
widget_(widget) {
nit: lines 19-20 could be combined.

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_widget_controller.cc:24: ui::Layer*
layer)
nit: lines 24-25 could be combined.

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_widget_controller.cc:35: if
(layer_.get())
else if

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_widget_controller.cc:42: if
(layer_.get())
else if

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_widget_controller.cc:54: if
(layer_.get()) {
else if

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
File ash/desktop_background/desktop_background_widget_controller.h (right):

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_widget_controller.h:5: #ifndef
ASH_DESKTOP_BACKGROUND_DESKTOP_BACKGROUND_COMPONENT_H_
Please rename according to the new file name.

http://codereview.chromium.org/10810039/diff/11002/ash/desktop_background/des...
ash/desktop_background/desktop_background_widget_controller.h:26: // Set bounds
for component.
nit: Usage of word "component" is not clear in this header.
Either remove or explain what that means.

[Denis Kuznetsov (DE-MUC), 2012-07-30 15:40:02]

Fixes for all Nikita's comments. Still waiting for Scott's review.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
File ash/desktop_background/desktop_background_controller.cc (right):

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.cc:7: #include
"ash/desktop_background/desktop_background_widget_controller.h"
On 2012/07/28 01:25:25, Nikita Kostylev wrote:
> nit: sort

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.cc:100: // Install
wallpaper component for all available windows.
On 2012/07/28 01:25:25, Nikita Kostylev wrote:
> nit: Not method name speaks for itself, comment should be removed.

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.cc:185: iter !=
root_windows.end(); ++iter) {
On 2012/07/28 01:25:25, Nikita Kostylev wrote:
> nit: Indentation is not fixed. Should be 
> 
> for (Shell::RootWindowList::iterator iter = root_windows.begin();
>      iter != root_windows.end(); ++iter) {

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.cc:279: default:
On 2012/07/28 01:25:25, Nikita Kostylev wrote:
> nit: Should also have {}
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.cc:288: iter !=
root_windows.end(); ++iter) {
On 2012/07/28 01:25:25, Nikita Kostylev wrote:
> nit: +1 indent space

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.cc:309: return  locked ?
internal::kShellWindowId_LockScreenBackgroundContainer :
On 2012/07/28 01:25:25, Nikita Kostylev wrote:
> nit: Remove extra space.

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
File ash/desktop_background/desktop_background_widget_controller.cc (right):

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_widget_controller.cc:20: :
widget_(widget) {
On 2012/07/28 01:32:17, Nikita Kostylev wrote:
> nit: lines 19-20 could be combined.

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_widget_controller.cc:24: ui::Layer*
layer)
On 2012/07/28 01:32:17, Nikita Kostylev wrote:
> nit: lines 24-25 could be combined.

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_widget_controller.cc:35: if
(layer_.get())
On 2012/07/28 01:32:17, Nikita Kostylev wrote:
> else if

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_widget_controller.cc:42: if
(layer_.get())
On 2012/07/28 01:32:17, Nikita Kostylev wrote:
> else if

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_widget_controller.cc:54: if
(layer_.get()) {
On 2012/07/28 01:32:17, Nikita Kostylev wrote:
> else if

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
File ash/desktop_background/desktop_background_widget_controller.h (right):

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_widget_controller.h:5: #ifndef
ASH_DESKTOP_BACKGROUND_DESKTOP_BACKGROUND_COMPONENT_H_
On 2012/07/28 01:32:17, Nikita Kostylev wrote:
> Please rename according to the new file name.

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/desktop_backgr...
ash/desktop_background/desktop_background_widget_controller.h:26: // Set bounds
for component.
On 2012/07/28 01:32:17, Nikita Kostylev wrote:
> nit: Usage of word "component" is not clear in this header.
> Either remove or explain what that means.

Done.

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/wm/root_window...
File ash/wm/root_window_layout_manager.cc (right):

https://chromiumcodereview.appspot.com/10810039/diff/11002/ash/wm/root_window...
ash/wm/root_window_layout_manager.cc:42:
internal::DesktopBackgroundWidgetController* background = owner_->GetProperty(
On 2012/07/28 01:25:25, Nikita Kostylev wrote:
> Sorry, I wasn't clear enough. This code is more readable:
> 
>   internal::DesktopBackgroundWidgetController* background =
>       owner_->GetProperty(internal::kWindowDesktopComponent);

Done.

[Ben Goodger (Google), 2012-07-30 21:08:32]

lgtm w/nits

https://chromiumcodereview.appspot.com/10810039/diff/25001/ash/desktop_backgr...
File ash/desktop_background/desktop_background_widget_controller.h (right):

https://chromiumcodereview.appspot.com/10810039/diff/25001/ash/desktop_backgr...
ash/desktop_background/desktop_background_widget_controller.h:41:
scoped_ptr<ui::Layer> layer_;
outdent both these lines by 1.

DISALLOW_COPY_AND_ASSIGN

https://chromiumcodereview.appspot.com/10810039/diff/25001/chrome/browser/chr...
File chrome/browser/chromeos/login/screen_locker.cc (right):

https://chromiumcodereview.appspot.com/10810039/diff/25001/chrome/browser/chr...
chrome/browser/chromeos/login/screen_locker.cc:149:
desktop_background_controller()->
same ehre

https://chromiumcodereview.appspot.com/10810039/diff/25001/chrome/browser/chr...
chrome/browser/chromeos/login/screen_locker.cc:328:
MoveDesktopToUnlockedContainer();
will this not fit one line up?

[bshe, 2012-07-30 21:38:20]

https://chromiumcodereview.appspot.com/10810039/diff/25001/ash/desktop_backgr...
File ash/desktop_background/desktop_background_controller.h (right):

https://chromiumcodereview.appspot.com/10810039/diff/25001/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.h:118: // Add layer with
solid |color| to container |container_id| in |root_window|
nit: Add/Adds

https://chromiumcodereview.appspot.com/10810039/diff/25001/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.h:121: int container_id);
nit: indent is off here.

https://chromiumcodereview.appspot.com/10810039/diff/25001/ash/desktop_backgr...
File ash/desktop_background/desktop_background_widget_controller.cc (right):

https://chromiumcodereview.appspot.com/10810039/diff/25001/ash/desktop_backgr...
ash/desktop_background/desktop_background_widget_controller.cc:26: 
nit: remove the extra line here.

[commit-bot: I haz the power, 2012-07-31 13:47:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/10810039/19005

[commit-bot: I haz the power, 2012-07-31 14:13:28]

Try job failure for 10810039-19005 (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-07-31 14:43:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/10810039/11007

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

Try job failure for 10810039-11007 (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-07-31 18:57:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/10810039/31001

[commit-bot: I haz the power, 2012-07-31 19:25:16]

Try job failure for 10810039-31001 (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...

[Nikita (slow), 2012-08-03 03:53:17]

If you could not fix that unittest please comment that part and commit as is and
fix it in the follow up CL.

lgtm

[commit-bot: I haz the power, 2012-08-03 15:08:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/10810039/29003

[commit-bot: I haz the power, 2012-08-03 15:08:07]

Failed to apply patch for ash/ash.gyp:
While running patch -p1 --forward --force;
patching file ash/ash.gyp
Hunk #1 FAILED at 61.
1 out of 1 hunk FAILED -- saving rejects to file ash/ash.gyp.rej

[commit-bot: I haz the power, 2012-08-03 15:52:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/10810039/26023

[commit-bot: I haz the power, 2012-08-03 17:37:34]

Change committed as 149869

[Denis Kuznetsov (DE-MUC), 2012-08-06 13:35:23]

Ben/Scott, please take a look on changes since #12.
Biao, please look on how merge is done in desktop_background_controller.cc

[sky, 2012-08-06 15:12:37]

Since Ben took over the review on this, I'm removing myself as a reviewer.

  -Scott

On Mon, Aug 6, 2012 at 6:35 AM,  <antrim@chromium.org> wrote:
> Ben/Scott, please take a look on changes since #12.
> Biao, please look on how merge is done in desktop_background_controller.cc
>
> http://codereview.chromium.org/10810039/

[Ben Goodger (Google), 2012-08-06 19:41:36]

lgtm

[commit-bot: I haz the power, 2012-08-07 02:54:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/10810039/14023

[commit-bot: I haz the power, 2012-08-07 04:17:34]

Change committed as 150271