Use a #fefefe solid color wallpaper background before user login for GAIA login flow

Use a #fefefe solid color wallpaper background before user login for GAIA login flow.


BUG=130635
TEST=
Enable new OOBE login from about:flags and relaunch browser, in
the loggin screen, the background color should be white (#FEFEFE).

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

[bshe, 2012-06-01 21:59:09]

Hi Nikita.

Could you take a look at this? It should make the default root window background
color to #fefefe.

Thanks!

[Nikita (slow), 2012-06-04 11:23:04]

https://chromiumcodereview.appspot.com/10492003/diff/1/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://chromiumcodereview.appspot.com/10492003/diff/1/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:153:
background_layer->SetColor(SK_ColorWallpaperGray);
I'm not sure whether this affects other code paths / cases?

[bshe, 2012-06-04 13:46:22]

https://chromiumcodereview.appspot.com/10492003/diff/1/ash/desktop_background...
File ash/desktop_background/desktop_background_controller.cc (right):

https://chromiumcodereview.appspot.com/10492003/diff/1/ash/desktop_background...
ash/desktop_background/desktop_background_controller.cc:153:
background_layer->SetColor(SK_ColorWallpaperGray);
I think we dont use the solid color background at all before this CL. We will
use it more for next version of wallpaper manager. So I will change this
function to take a SkColor parameter later.

Here, I just want to minimize the change. But as you mentioned, make empty
wallpaper transparent can also work. If we want to go that path, I guess this cl
is not necessary then.

On 2012/06/04 11:23:04, Nikita Kostylev wrote:
> I'm not sure whether this affects other code paths / cases?

[bshe, 2012-06-04 18:07:19]

On 2012/06/04 13:46:22, bshe wrote:
>
https://chromiumcodereview.appspot.com/10492003/diff/1/ash/desktop_background...
> File ash/desktop_background/desktop_background_controller.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10492003/diff/1/ash/desktop_background...
> ash/desktop_background/desktop_background_controller.cc:153:
> background_layer->SetColor(SK_ColorWallpaperGray);
> I think we dont use the solid color background at all before this CL. We will
> use it more for next version of wallpaper manager. So I will change this
> function to take a SkColor parameter later.
> 
> Here, I just want to minimize the change. But as you mentioned, make empty
> wallpaper transparent can also work. If we want to go that path, I guess this
cl
> is not necessary then.
> 
> On 2012/06/04 11:23:04, Nikita Kostylev wrote:
> > I'm not sure whether this affects other code paths / cases?

As discussed, patchset 2 should load a #fefefe wallpaper before user login when
using GAIA login flow. Thanks!

[Nikita (slow), 2012-06-04 18:23:03]

lgtm with nits

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

http://codereview.chromium.org/10492003/diff/6001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:142: } else if (index ==
ash::GetSolidColorIndex()) {
I'm a bit confused why this happens inside SetLoggedInUserWallpaper() method.

Comment says
  // Loads logged in user wallpaper asynchronously and sets to current wallpaper
  // after loaded.

Given the fact that this now handles any wallpaper related initialization on
boot (both normal boot + after crash), makes sense to rename this method to
something like InitializeWallpaper?

http://codereview.chromium.org/10492003/diff/6001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:156:
background_layer->SetColor(SK_ColorWallpaperGray);
I think that it makes sense to pass color as a parameter.

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

http://codereview.chromium.org/10492003/diff/6001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.h:29: #define
SK_ColorWallpaperGray 0xFEFEFE
That is actually close to white.
SK_ColorWallpaperLogin to be more clear.

I think this is only to be used at the sign in screen and when user actually
signs in it should change to random one.

[Nikita (slow), 2012-06-05 16:37:39]

I've patches this CL and it doesn't seem to be working. 
When booting into GAIA I see black background. When selecting user at sign in
screen I see their background though.
Investigating.

[Nikita (slow), 2012-06-06 11:27:46]

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

http://codereview.chromium.org/10492003/diff/4007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:971: void
UserManagerImpl::GetLoggedInUserWallpaperProperties(
This method is called from:

1. UserManagerImpl::EnsureLoggedInUserWallpaperLoaded() on sign in.
2. SetWallpaperOptionsHandler::HandlePageShown()

http://codereview.chromium.org/10492003/diff/4007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:979: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableNewOobe)) {
This should be moved to InitializeWallpaper() to make this work as expected.

[Nikita (slow), 2012-06-07 09:40:28]

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

http://codereview.chromium.org/10492003/diff/4007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:364: if (!IsUserLoggedIn() ||
IsLoggedInAsStub()) {
https://chromiumcodereview.appspot.com/10541007/ has landed.
It preselects last logged in user on boot from sign in screen that triggers
wallpaper load. That means that you don't to set empty wallpaper in case
!IsUserLoggedIn()

[bshe, 2012-06-08 15:06:30]

Done.

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

http://codereview.chromium.org/10492003/diff/6001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:142: } else if (index ==
ash::GetSolidColorIndex()) {
It is also used when user toggle background color mode in
ash/accelerators/accelerator_controller.cc:163. But I agree rename it to
InitializeWallpaper makes more sense.

On 2012/06/04 18:23:03, Nikita Kostylev wrote:
> I'm a bit confused why this happens inside SetLoggedInUserWallpaper() method.
> 
> Comment says
>   // Loads logged in user wallpaper asynchronously and sets to current
wallpaper
>   // after loaded.
> 
> Given the fact that this now handles any wallpaper related initialization on
> boot (both normal boot + after crash), makes sense to rename this method to
> something like InitializeWallpaper?

http://codereview.chromium.org/10492003/diff/6001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:156:
background_layer->SetColor(SK_ColorWallpaperGray);
On 2012/06/04 18:23:03, Nikita Kostylev wrote:
> I think that it makes sense to pass color as a parameter.

Done.

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

http://codereview.chromium.org/10492003/diff/6001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_resources.h:29: #define
SK_ColorWallpaperGray 0xFEFEFE
On 2012/06/04 18:23:03, Nikita Kostylev wrote:
> That is actually close to white.
> SK_ColorWallpaperLogin to be more clear.
> 
> I think this is only to be used at the sign in screen and when user actually
> signs in it should change to random one.

Done.

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

http://codereview.chromium.org/10492003/diff/4007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:364: if (!IsUserLoggedIn() ||
IsLoggedInAsStub()) {
On 2012/06/07 09:40:28, Nikita Kostylev wrote:
> https://chromiumcodereview.appspot.com/10541007/ has landed.
> It preselects last logged in user on boot from sign in screen that triggers
> wallpaper load. That means that you don't to set empty wallpaper in case
> !IsUserLoggedIn()

right. And only for GAIA login flow, we need to set the solid color background.

http://codereview.chromium.org/10492003/diff/4007/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:979: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableNewOobe)) {
On 2012/06/06 11:27:46, Nikita Kostylev wrote:
> This should be moved to InitializeWallpaper() to make this work as expected.

Right. Sorry I was in a hurry. I did check on my alex device but just check the
code path for "Add user" from user pod. Thanks for investigating it.

Done

[Nikita (slow), 2012-06-08 17:24:06]

lgtm

[bshe, 2012-06-11 13:52:28]

derat@ Since you already in the reviewers list, could you please take a look at
ash files for OWNERS?
ash/*

+jhawkins
for chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc

Thanks!

[James Hawkins, 2012-06-11 14:04:04]

webui LGTM

[Daniel Erat, 2012-06-11 16:06:05]

http://codereview.chromium.org/10492003/diff/19013/ash/desktop_background/des...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/10492003/diff/19013/ash/desktop_background/des...
ash/desktop_background/desktop_background_resources.h:29: #define
SK_ColorWallpaperLogin 0xFEFEFE
make this be a const variable named something like kLoginWallpaperColor rather
than using #define

http://codereview.chromium.org/10492003/diff/19013/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/10492003/diff/19013/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager.h:108: // use a solid color(#fefefe)
wallpaper. If logged in as a stub user, uses an
nit: don't mention the specific color here; this comment is likely to get stale
if/when the color is changed later

http://codereview.chromium.org/10492003/diff/19013/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/10492003/diff/19013/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/user_manager_impl.cc:369: bool show_users;
initialize this to false or true.  you're ignoring the return value from
CrosSettings::GetBoolean(), so if the pref doesn't exist, i think that you'll be
reading uninitialized memory later.

[bshe, 2012-06-11 17:04:15]

derat@

Done. Thanks!

https://chromiumcodereview.appspot.com/10492003/diff/19013/ash/desktop_backgr...
File ash/desktop_background/desktop_background_resources.h (right):

https://chromiumcodereview.appspot.com/10492003/diff/19013/ash/desktop_backgr...
ash/desktop_background/desktop_background_resources.h:29: #define
SK_ColorWallpaperLogin 0xFEFEFE
On 2012/06/11 16:06:06, Daniel Erat wrote:
> make this be a const variable named something like kLoginWallpaperColor rather
> than using #define

Done.

https://chromiumcodereview.appspot.com/10492003/diff/19013/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager.h (right):

https://chromiumcodereview.appspot.com/10492003/diff/19013/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager.h:108: // use a solid color(#fefefe)
wallpaper. If logged in as a stub user, uses an
On 2012/06/11 16:06:06, Daniel Erat wrote:
> nit: don't mention the specific color here; this comment is likely to get
stale
> if/when the color is changed later

Done.

https://chromiumcodereview.appspot.com/10492003/diff/19013/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10492003/diff/19013/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:369: bool show_users;
Agree. 
We should probably also add a DCHECK here? This pref doesnt exist probably
suggests that there is a bug. Normally, for chromeos, this pref should always
registered. And the default value is true. 

On 2012/06/11 16:06:06, Daniel Erat wrote:
> initialize this to false or true.  you're ignoring the return value from
> CrosSettings::GetBoolean(), so if the pref doesn't exist, i think that you'll
be
> reading uninitialized memory later.

[Daniel Erat, 2012-06-11 17:12:10]

lgtm

https://chromiumcodereview.appspot.com/10492003/diff/19013/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10492003/diff/19013/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:369: bool show_users;
DCHECK sounds fine, but make sure you do:

  bool result = CrosSettings::Get()...;
  DCHECK(result) << "Unable to fetch setting " << kAccounts...;

instead of:

  DCHECK(CrosSettings::Get()...);

On 2012/06/11 17:04:15, bshe wrote:
> Agree. 
> We should probably also add a DCHECK here? This pref doesnt exist probably
> suggests that there is a bug. Normally, for chromeos, this pref should always
> registered. And the default value is true. 
> 
> On 2012/06/11 16:06:06, Daniel Erat wrote:
> > initialize this to false or true.  you're ignoring the return value from
> > CrosSettings::GetBoolean(), so if the pref doesn't exist, i think that
you'll
> be
> > reading uninitialized memory later.
>

https://chromiumcodereview.appspot.com/10492003/diff/22028/chrome/browser/chr...
File chrome/browser/chromeos/login/user_manager.h (right):

https://chromiumcodereview.appspot.com/10492003/diff/22028/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager.h:107: // Initalize wallpaper. If
logged in, load user's wallpaper. If not logged in,
nit: s/Initalize/Initializes/ (fix misspelling and add 's' to end)

[commit-bot: I haz the power, 2012-06-11 23:05:38]

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

[commit-bot: I haz the power, 2012-06-12 02:00:44]

Change committed as 141596