Fix white flash when user sings out and wallpaper animation regression

Fix white flash when user signs out and fix wallpaper animation regression


When user signs out and transit to the login screen, the wallpaper fade in from a white screen. Described in 141563, this Cl fixed it.
Also, after user login, or switch users at login screen. when wallpaper change, the new wallpaper should fade in nicely. There is a regression in recent builds. This fix fixed the regression as well. 

BUG=141563, 141676, 142042
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151393

[bshe, 2012-08-10 18:32:59]

could you please take a look? Thanks!

[Nikita (slow), 2012-08-10 18:55:09]

lgtm

[bshe, 2012-08-10 19:06:19]

+sky

Could you please take a look? Thanks!

[sky, 2012-08-10 19:20:37]

https://chromiumcodereview.appspot.com/10827284/diff/3003/ash/desktop_backgro...
File ash/desktop_background/desktop_background_controller.h (right):

https://chromiumcodereview.appspot.com/10827284/diff/3003/ash/desktop_backgro...
ash/desktop_background/desktop_background_controller.h:172:
RootWindowComponentMap root_window_component_map_;
You never clean this state as roots are removed. I think it would be better to
store this state as a property on the rootwindow.

[bshe, 2012-08-10 19:51:41]

https://chromiumcodereview.appspot.com/10827284/diff/3003/ash/desktop_backgro...
File ash/desktop_background/desktop_background_controller.h (right):

https://chromiumcodereview.appspot.com/10827284/diff/3003/ash/desktop_backgro...
ash/desktop_background/desktop_background_controller.h:172:
RootWindowComponentMap root_window_component_map_;
This map is a place to store the component for root window. When animation
finished, the component is stored as a property (kWindowDesktopComponent) on the
root window. The reason I have to wait for the animation finish is because if we
set that property immediately, the old component is removed right away. And a
white screen will be displayed. The new wallpaper/component will fade in from
that white screen. So to avoid the white screen issue, I need to set the
property after animation finished. Given that we already have a property to save
the data, I just want to confirm with you that it is OK to create another
property (something like kWindowDesktopComponentTemp?) on root window to save
the component temporarily (before animation finished). I am not familiar with
root window property, so I am not sure if it is cheap enough. Thanks for
suggestion.

On 2012/08/10 19:20:37, sky wrote:
> You never clean this state as roots are removed. I think it would be better to
> store this state as a property on the rootwindow.

[sky, 2012-08-10 22:18:43]

Yes, you can add another property.

  -Scott

On Fri, Aug 10, 2012 at 12:51 PM,  <bshe@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10827284/diff/3003/ash/desktop_backgro...
> File ash/desktop_background/desktop_background_controller.h (right):
>
>
https://chromiumcodereview.appspot.com/10827284/diff/3003/ash/desktop_backgro...
> ash/desktop_background/desktop_background_controller.h:172:
> RootWindowComponentMap root_window_component_map_;
> This map is a place to store the component for root window. When
> animation finished, the component is stored as a property
> (kWindowDesktopComponent) on the root window. The reason I have to wait
> for the animation finish is because if we set that property immediately,
> the old component is removed right away. And a white screen will be
> displayed. The new wallpaper/component will fade in from that white
> screen. So to avoid the white screen issue, I need to set the property
> after animation finished. Given that we already have a property to save
> the data, I just want to confirm with you that it is OK to create
> another property (something like kWindowDesktopComponentTemp?) on root
> window to save the component temporarily (before animation finished). I
> am not familiar with root window property, so I am not sure if it is
> cheap enough. Thanks for suggestion.
>
>
> On 2012/08/10 19:20:37, sky wrote:
>>
>> You never clean this state as roots are removed. I think it would be
>
> better to
>>
>> store this state as a property on the rootwindow.
>
>
> https://chromiumcodereview.appspot.com/10827284/

[bshe, 2012-08-13 16:07:19]

sky@
Thanks for suggestion. Could you take another look?

On 2012/08/10 22:18:43, sky wrote:
> Yes, you can add another property.
> 
>   -Scott
> 
> On Fri, Aug 10, 2012 at 12:51 PM,  <mailto:bshe@chromium.org> wrote:
> >
> >
>
https://chromiumcodereview.appspot.com/10827284/diff/3003/ash/desktop_backgro...
> > File ash/desktop_background/desktop_background_controller.h (right):
> >
> >
>
https://chromiumcodereview.appspot.com/10827284/diff/3003/ash/desktop_backgro...
> > ash/desktop_background/desktop_background_controller.h:172:
> > RootWindowComponentMap root_window_component_map_;
> > This map is a place to store the component for root window. When
> > animation finished, the component is stored as a property
> > (kWindowDesktopComponent) on the root window. The reason I have to wait
> > for the animation finish is because if we set that property immediately,
> > the old component is removed right away. And a white screen will be
> > displayed. The new wallpaper/component will fade in from that white
> > screen. So to avoid the white screen issue, I need to set the property
> > after animation finished. Given that we already have a property to save
> > the data, I just want to confirm with you that it is OK to create
> > another property (something like kWindowDesktopComponentTemp?) on root
> > window to save the component temporarily (before animation finished). I
> > am not familiar with root window property, so I am not sure if it is
> > cheap enough. Thanks for suggestion.
> >
> >
> > On 2012/08/10 19:20:37, sky wrote:
> >>
> >> You never clean this state as roots are removed. I think it would be
> >
> > better to
> >>
> >> store this state as a property on the rootwindow.
> >
> >
> > https://chromiumcodereview.appspot.com/10827284/

[sky, 2012-08-13 17:31:05]

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

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:250: if
(desktop_background_mode_ != BACKGROUND_IMAGE) {
Do you even need the if now?

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_widget_controller.h (right):

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
ash/desktop_background/desktop_background_widget_controller.h:25: // desctructor
is called. It immediately remove the widget or layer it
Move this to the class level. Also, its rather confusing. I think you mean
something like:

DesktopBackgroundWidgetController is installed as an owned property on the
RootWindow. To avoid a white flash during wallpaper changes the old
DesktopBackgroundWidgetController is moved to a secondary property (XXX). When
the animation completes the old DesktopBackgroundWidgetController is destroyed.

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
ash/desktop_background/desktop_background_widget_controller.h:52: // This class
wrappes a DesktopBackgroundWidgetController pointer. It is used to
wraps

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
ash/desktop_background/desktop_background_widget_controller.h:68:
DISALLOW_COPY_AND_ASSIGN(ComponentWrapper);
nit: newline between 67/68.

[bshe, 2012-08-13 18:28:22]

Done. Thanks!

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

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:250: if
(desktop_background_mode_ != BACKGROUND_IMAGE) {
No need. Removed it.

On 2012/08/13 17:31:05, sky wrote:
> Do you even need the if now?

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_widget_controller.h (right):

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
ash/desktop_background/desktop_background_widget_controller.h:25: // desctructor
is called. It immediately remove the widget or layer it
On 2012/08/13 17:31:05, sky wrote:
> Move this to the class level. Also, its rather confusing. I think you mean
> something like:
> 
> DesktopBackgroundWidgetController is installed as an owned property on the
> RootWindow. To avoid a white flash during wallpaper changes the old
> DesktopBackgroundWidgetController is moved to a secondary property (XXX). When
> the animation completes the old DesktopBackgroundWidgetController is
destroyed.

Done.

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
ash/desktop_background/desktop_background_widget_controller.h:52: // This class
wrappes a DesktopBackgroundWidgetController pointer. It is used to
On 2012/08/13 17:31:05, sky wrote:
> wraps

Done.

http://codereview.chromium.org/10827284/diff/5005/ash/desktop_background/desk...
ash/desktop_background/desktop_background_widget_controller.h:68:
DISALLOW_COPY_AND_ASSIGN(ComponentWrapper);
On 2012/08/13 17:31:05, sky wrote:
> nit: newline between 67/68.

Done.

[sky, 2012-08-13 20:57:33]

LGTM

[commit-bot: I haz the power, 2012-08-13 21:02:45]

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

[commit-bot: I haz the power, 2012-08-13 23:50:44]

Change committed as 151393