Using random wallpaper until user select one

Using a random wallpaper each time user login. If user specifically select a wallpaper, then use the selected wallpaper.

It also avoid to save wallpaper index to local state when an ephemeral user logged in.

BUG=120215
TEST=None

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

[bshe, 2012-03-26 14:41:33]

Hi Nikita.

This CL address your comments in here:
http://codereview.chromium.org/9839098/

It seems Demo/Stub/ephemeral state enabled users still have the ability to
change wallpaper. So instead of change it to IsCurrentUserEphemeral in
GetUserWallpaperIndex method, I add extra conditions to enable the feature for
these users.

Thanks!

[Nikita (slow), 2012-03-26 15:31:58]

lgtm

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:919: if (IsLoggedInAsGuest()
|| !IsUserLoggedIn())
if (!IsUserLoggedIn())

guest is handled below.

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.h:233: int
ephemeral_user_wallpaper_index_;
Do you think this may be moved somewhere to ash/* instead?

[bshe, 2012-03-26 16:12:10]

Inlined.
Thanks for quick review!

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:919: if (IsLoggedInAsGuest()
|| !IsUserLoggedIn())
It seems we want to always return the same index for guest user. So I add this
conditions here. The ephemeral_user_wallpaper_index_ might change if guest user
try to set a wallpaper. Ideally, guest user shouldn't. We can get rid of the
guest user check after issue 26900 get fixed. But it might be safer to keep it
anyway.


On 2012/03/26 15:31:58, Nikita Kostylev wrote:
> if (!IsUserLoggedIn())
> 
> guest is handled below.

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.h:233: int
ephemeral_user_wallpaper_index_;
I can probably put it in ash/desktop_background/desktop_background_controller
with some plumbing work. I just thought it might be a good idea to put
everything user related to user_manager_impl. I didnt see similar cache pattern
used in this file though. I am wondering is there any particular reason why it
is not preferred? 

On 2012/03/26 15:31:58, Nikita Kostylev wrote:
> Do you think this may be moved somewhere to ash/* instead?

[Nikita (slow), 2012-03-26 16:50:07]

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:919: if (IsLoggedInAsGuest()
|| !IsUserLoggedIn())
On 2012/03/26 16:12:10, bshe wrote:
> It seems we want to always return the same index for guest user. 

Ok, now I've checked that kGuestWallpaperIndex is fixed.

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://chromiumcodereview.appspot.com/9856016/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.h:233: int
ephemeral_user_wallpaper_index_;
On 2012/03/26 16:12:10, bshe wrote:
> I can probably put it in ash/desktop_background/desktop_background_controller
> with some plumbing work. I just thought it might be a good idea to put
> everything user related to user_manager_impl. I didnt see similar cache
pattern
> used in this file though. I am wondering is there any particular reason why it
> is not preferred? 

I've thought that ash should have something like current wallpaper index but it
seems that it only has current wallpaper bitmap. And it doesn't make sense to
cache index in ash as it always is fetched from LocalState if needed. If we'd
have more such cases when we'll need to cache local_state data, we'll have to
provide some in memory storage.

[Mattias Nissler (ping if slow), 2012-03-26 17:24:58]

Meta question: Why do we cache the wallpaper in local state and not in user
prefs in the first place?

Also, you want this to be reviewed by Bartosz (added), who is the ephemeral
users expert.

[Nikita (slow), 2012-03-26 17:38:33]

On 2012/03/26 17:24:58, Mattias Nissler wrote:
> Meta question: Why do we cache the wallpaper in local state and not in user
> prefs in the first place?

I guess that was because we already store information about all users in
LocalState - images, list of users. But that is because they're needed at login
screen.

Agree that wallpaper pref should be moved to Preferences instead.

const char UserManager::kLoggedInUsers[] = "LoggedInUsers";
const char UserManager::kUserWallpapers[] = "UserWallpapers";
const char UserManager::kUserImages[] = "UserImages";

[Mattias Nissler (ping if slow), 2012-03-26 17:44:29]

On 2012/03/26 17:38:33, Nikita Kostylev wrote:
> On 2012/03/26 17:24:58, Mattias Nissler wrote:
> > Meta question: Why do we cache the wallpaper in local state and not in user
> > prefs in the first place?
> 
> I guess that was because we already store information about all users in
> LocalState - images, list of users. But that is because they're needed at
login
> screen.

I see. We actually need to be very careful about cases like this, since one of
the selling points of Chrome OS is that all user data is kept private in
cryptohome. What if a user likes to configure an engagement ring wallpaper and
make sure it doesn't leak? ;)

IMHO, the rule should be: Only stuff that applies to pre-login experience or is
deliberately shared over all users can go into local state. Everything else
needs to be stored inside the cryptohome, and Preferences is one of the simpler
ways of achieving that.

> 
> Agree that wallpaper pref should be moved to Preferences instead.
> 
> const char UserManager::kLoggedInUsers[] = "LoggedInUsers";
> const char UserManager::kUserWallpapers[] = "UserWallpapers";
> const char UserManager::kUserImages[] = "UserImages";

[Nikita (slow), 2012-03-26 17:51:51]

On 2012/03/26 17:44:29, Mattias Nissler wrote:
> On 2012/03/26 17:38:33, Nikita Kostylev wrote:
> > On 2012/03/26 17:24:58, Mattias Nissler wrote:
> > > Meta question: Why do we cache the wallpaper in local state and not in
user
> > > prefs in the first place?
> > 
> > I guess that was because we already store information about all users in
> > LocalState - images, list of users. But that is because they're needed at
> login
> > screen.
> 
> I see. We actually need to be very careful about cases like this, since one of
> the selling points of Chrome OS is that all user data is kept private in
> cryptohome. What if a user likes to configure an engagement ring wallpaper and
> make sure it doesn't leak? ;)

To this particular point. Even if such setting is stored in Local State, that
file itself is not accessible.

It may become accessible when we provide a way to diagnose network issues which
will be the same as generate_logs script but may include less files. Local State
presence is questionable. For reference: http://crosbug.com/25700

But again I agree that wallpaper index setting should be moved to Preferences.

[bshe, 2012-03-26 18:18:37]

Thanks for suggestions. I will move the index from local state to preferences in
a separate CL as we dont need it for pre-login.


On 2012/03/26 17:51:51, Nikita Kostylev wrote:
> On 2012/03/26 17:44:29, Mattias Nissler wrote:
> > On 2012/03/26 17:38:33, Nikita Kostylev wrote:
> > > On 2012/03/26 17:24:58, Mattias Nissler wrote:
> > > > Meta question: Why do we cache the wallpaper in local state and not in
> user
> > > > prefs in the first place?
> > > 
> > > I guess that was because we already store information about all users in
> > > LocalState - images, list of users. But that is because they're needed at
> > login
> > > screen.
> > 
> > I see. We actually need to be very careful about cases like this, since one
of
> > the selling points of Chrome OS is that all user data is kept private in
> > cryptohome. What if a user likes to configure an engagement ring wallpaper
and
> > make sure it doesn't leak? ;)
> 
> To this particular point. Even if such setting is stored in Local State, that
> file itself is not accessible.
> 
> It may become accessible when we provide a way to diagnose network issues
which
> will be the same as generate_logs script but may include less files. Local
State
> presence is questionable. For reference: http://crosbug.com/25700
> 
> But again I agree that wallpaper index setting should be moved to Preferences.

[commit-bot: I haz the power, 2012-03-26 19:34:25]

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

[commit-bot: I haz the power, 2012-03-26 19:34:27]

Can't apply patch for file chrome/browser/chromeos/login/user_manager_impl.h.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/login/user_manager_impl.h
Hunk #1 FAILED at 229.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/login/user_manager_impl.h.rej

[bshe, 2012-03-26 20:41:52]

+nkostylev
could you please take another look? I was assigned another M19 bug which related
to the user wallpaper index (see issue 120215). Rather than make one depends on
the other, I thought it might be more efficient to just combine these two into
one CL.

+james for owners: chrome/browser/resources

Thanks!

[James Hawkins, 2012-03-26 20:44:04]

lgtm

[Ivan Korotkov, 2012-03-26 20:52:13]

LGTM with nits

http://codereview.chromium.org/9856016/diff/9001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

http://codereview.chromium.org/9856016/diff/9001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/user_images_grid.js:131:
this.inProgramSelection_ = true;
Can you replace this with this.selectedItemIndex = i to prevent duplication?

http://codereview.chromium.org/9856016/diff/9001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/user_images_grid.js:145:
this.selectionModel.selectedIndex = index;
I guess you should be updating leadIndex here as well.

[bshe, 2012-03-26 21:02:20]

Done and inlined. Thanks!

http://codereview.chromium.org/9856016/diff/9001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

http://codereview.chromium.org/9856016/diff/9001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/user_images_grid.js:131:
this.inProgramSelection_ = true;
On 2012/03/26 20:52:13, Ivan Korotkov wrote:
> Can you replace this with this.selectedItemIndex = i to prevent duplication?

Done.

http://codereview.chromium.org/9856016/diff/9001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/user_images_grid.js:145:
this.selectionModel.selectedIndex = index;
It seems set selectedIndex also changes lead and anchor indexes if index is
nonegative. And here the index should not be negative. So I didn't add it.



On 2012/03/26 20:52:13, Ivan Korotkov wrote:
> I guess you should be updating leadIndex here as well.

[Ivan Korotkov, 2012-03-26 21:05:30]

http://codereview.chromium.org/9856016/diff/9001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/user_images_grid.js (right):

http://codereview.chromium.org/9856016/diff/9001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/user_images_grid.js:145:
this.selectionModel.selectedIndex = index;
On 2012/03/26 21:02:21, bshe wrote:
> It seems set selectedIndex also changes lead and anchor indexes if index is
> nonegative. And here the index should not be negative. So I didn't add it.
> 
> 
> 
> On 2012/03/26 20:52:13, Ivan Korotkov wrote:
> > I guess you should be updating leadIndex here as well.
> 

Right, must be a recent change to ListSelectionModel.

[bshe, 2012-03-26 21:19:03]

+Dave
Could you please take a look?
It seems the local time is 1am for Nikita. And since it includes a M19 bug which
assigned to me today. I hope to land it asap.

Thanks!

[bartfab (slow), 2012-03-27 07:48:49]

I cannot officially review as I am not a committer yet. Still, here is my take:

I did not see the original wallpaper code go in and I am concerned about the way
it works:

* First, this should be a preference, as Mattias said.
* Second, you have special case code that avoids writing to Local State if the
current user is ephemeral. But you have no code to clean up Local State when
ephemeral users are turned on for an existing device.

Fortunately, the second concern will go away when you fix the first - you will
no longer be storing the wallpaper in the Local State at all.

[bshe, 2012-03-27 13:52:58]

Thanks!
I will submit a separate CL for storing it to preference. And it will address
your two concerns. I have tracked it in issue 120276. 

On 2012/03/27 07:48:49, bartfab wrote:
> I cannot officially review as I am not a committer yet. Still, here is my
take:
> 
> I did not see the original wallpaper code go in and I am concerned about the
way
> it works:
> 
> * First, this should be a preference, as Mattias said.
> * Second, you have special case code that avoids writing to Local State if the
> current user is ephemeral. But you have no code to clean up Local State when
> ephemeral users are turned on for an existing device.
> 
> Fortunately, the second concern will go away when you fix the first - you will
> no longer be storing the wallpaper in the Local State at all.

[Nikita (slow), 2012-03-27 14:25:33]

I think that this CL should be abandoned and
http://code.google.com/p/chromium/issues/detail?id=120215
http://code.google.com/p/chromium/issues/detail?id=120276

Fixed in a single CL.

[bartfab (slow), 2012-03-27 15:03:49]

Wallpaper preferences were being written to Local Store before this CL landed
already. Reverting and abandoning this CL will not be enough to fix the
situation. Better focus on the two issues Nikita pointed out ASAP while leaving
this sad CL in IMHO.

[Nikita (slow), 2012-03-27 15:29:31]

lgtm

[bshe, 2012-03-27 23:30:33]

On 2012/03/27 15:29:31, Nikita Kostylev wrote:
> lgtm

It seems the preference bug is punted to M20 to reduce risk for M19. So I will
still commit this one to fix issue 120215. Using preference and local state
clean up will be in a separate CL for M20.

[commit-bot: I haz the power, 2012-03-27 23:31:32]

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

[commit-bot: I haz the power, 2012-03-28 01:04:52]

Try job failure for 9856016-13004 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "update" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-28 01:14:03]

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

[commit-bot: I haz the power, 2012-03-28 03:58:03]

Try job failure for 9856016-13004 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "ui_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-28 04:26:07]

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

[commit-bot: I haz the power, 2012-03-28 06:59:21]

Try job failure for 9856016-13004 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "ui_tests, browser_tests,
installer_util_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-28 08:24:45]

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

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

Try job failure for 9856016-13004 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.