Save user selected wallpaper information to local state.

This Cl saves user selected wallpaper information to local state. (For new wallpaper manager only).


BUG=118684
TEST=

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

[bshe, 2012-07-23 19:16:21]

Hi Nikita.

Could you please take a look at this CL?

Basically, this CL implements function to save wallpaper information (new
wallpaper manager) to local state for a specific user.

Thanks!

[Nikita (slow), 2012-07-23 22:06:46]

lgtm

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:170:
chromeos::WallpaperManager::Get()->SetWallpaperFromImageSkia(layout_,
nit: Save WallpaperManager instance to local variable. Would improve readability
a bit.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:31: const char
kUsersWallpaperInfos[] = "UsersWallpaperInfos";
s/Infos/Info/

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:93: // in memory for them.
Do those users have access to custom wallpaper UI?
Probably they shouldn't as UI just silently does nothing.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:94: if
(UserManager::Get()->IsCurrentUserEphemeral()) {
nit: could omit { }

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.h (right):

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:45: void
SaveUserWallpaperInfos(const std::string& username,
s/Infos/Info/

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:46: std::string file_name,
std::string&

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:58: void
SetWallpaperFromImageSkia(ash::WallpaperLayout layout,
Please bring signature closer to SetWallpaperFromFilePath,
probably place wallpaper as a first parameter.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:72: void
OnWallpaperLoaded(ash::WallpaperLayout layout,
Please add comment

[bshe, 2012-07-24 16:21:10]

thanks.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:170:
chromeos::WallpaperManager::Get()->SetWallpaperFromImageSkia(layout_,
On 2012/07/23 22:06:47, Nikita Kostylev wrote:
> nit: Save WallpaperManager instance to local variable. Would improve
readability
> a bit.

Done.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:31: const char
kUsersWallpaperInfos[] = "UsersWallpaperInfos";
On 2012/07/23 22:06:47, Nikita Kostylev wrote:
> s/Infos/Info/

Done.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:93: // in memory for them.
As far as I can remember, only guest user can't access custom wallpaper UI. Are
you seeing that other type of ephemeral user can't set wallpaper to custom
wallpaper?

On 2012/07/23 22:06:47, Nikita Kostylev wrote:
> Do those users have access to custom wallpaper UI?
> Probably they shouldn't as UI just silently does nothing.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:94: if
(UserManager::Get()->IsCurrentUserEphemeral()) {
On 2012/07/23 22:06:47, Nikita Kostylev wrote:
> nit: could omit { }

Done.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.h (right):

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:45: void
SaveUserWallpaperInfos(const std::string& username,
On 2012/07/23 22:06:47, Nikita Kostylev wrote:
> s/Infos/Info/

Done.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:46: std::string file_name,
On 2012/07/23 22:06:47, Nikita Kostylev wrote:
> std::string&

Done.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:58: void
SetWallpaperFromImageSkia(ash::WallpaperLayout layout,
On 2012/07/23 22:06:47, Nikita Kostylev wrote:
> Please bring signature closer to SetWallpaperFromFilePath,
> probably place wallpaper as a first parameter.

Done.

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.h:72: void
OnWallpaperLoaded(ash::WallpaperLayout layout,
On 2012/07/23 22:06:47, Nikita Kostylev wrote:
> Please add comment

Done.

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

still lgtm

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10815047/diff/1008/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/wallpaper_manager.cc:93: // in memory for them.
On 2012/07/24 16:21:10, bshe wrote:
> As far as I can remember, only guest user can't access custom wallpaper UI.
Are
> you seeing that other type of ephemeral user can't set wallpaper to custom
> wallpaper?
> 
> On 2012/07/23 22:06:47, Nikita Kostylev wrote:
> > Do those users have access to custom wallpaper UI?
> > Probably they shouldn't as UI just silently does nothing.
> 

But any Ephemeral user would exit here.
What would be the result of it?

http://codereview.chromium.org/10815047/diff/7002/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/10815047/diff/7002/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:100:
WallpaperSetWallpaperFunction::WallpaperSetWallpaperFunction(){
nit: missing space before {

http://codereview.chromium.org/10815047/diff/7002/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:103:
WallpaperSetWallpaperFunction::~WallpaperSetWallpaperFunction(){
nit: missing space before {

[bshe, 2012-07-30 14:37:48]

+battre for OWNERS:
chrome/browser/prefs/browser_prefs.cc

[battre, 2012-07-30 15:00:56]

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:31: const char
kUsersWallpaperInfo[] = "UsersWallpaperInfo";
this one should go to pref_names.h and follow the linux_hacker_style_notation
found there. Please check whether this needs to be a top-level element or
whether this can be grouped with something already listed there.

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:57:
PrefService::UNSYNCABLE_PREF);
- will there be only one user wallpaper?
- does it make sense to register a default here so you don't have to deal with
empty dictionaries? Maybe containing just a white image?

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:105: file_name);
nit: indentation

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:107: new
base::FundamentalValue(layout));
The more idiomatic way would be
wallpaper_properties->SetInteger(kWallpaperLayoutNodeName,
                                 layout);
(same below)

[bshe, 2012-07-30 18:05:05]

battre@

Done. Thanks!

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:31: const char
kUsersWallpaperInfo[] = "UsersWallpaperInfo";
On 2012/07/30 15:00:56, battre wrote:
> this one should go to pref_names.h and follow the linux_hacker_style_notation
> found there. Please check whether this needs to be a top-level element or
> whether this can be grouped with something already listed there.

Done. It is wallpaper specific and I can't find any other prefs that is related
to wallpaper in pref_name.h file. So I make it a top-level element.

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:57:
PrefService::UNSYNCABLE_PREF);
On 2012/07/30 15:00:56, battre wrote:
> - will there be only one user wallpaper?

Each user will have a record in the kUsersWallpaperInfo dictionary and there is
only one such dictionary in Local State.

> - does it make sense to register a default here so you don't have to deal with
> empty dictionaries? Maybe containing just a white image?

The structure of the dictionary is something like this:
"UserWallpapersProperties": {
      "username@gmail.com": {
         "date": "12987748800000000",
         "layout": 1,
         "file": "wallpaper.jpg",
         "type": 1
      }
}
Every item in the dictionary is associated with an email(username). When a user
first boot up a chrome book, since there is no valid user yet, I am not sure if
I should create a default username/email to register this pref. Once a user
login, a default wallpaper record associated with that user will be saved in the
dictionary.

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:105: file_name);
On 2012/07/30 15:00:56, battre wrote:
> nit: indentation

Done.

http://codereview.chromium.org/10815047/diff/22001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:107: new
base::FundamentalValue(layout));
On 2012/07/30 15:00:56, battre wrote:
> The more idiomatic way would be
> wallpaper_properties->SetInteger(kWallpaperLayoutNodeName,
>                                  layout);
> (same below)

Done.

[battre, 2012-07-31 14:40:17]

lgtm

http://codereview.chromium.org/10815047/diff/23005/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10815047/diff/23005/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:103: file_name);
nit: no need to wrap these lines.

[bshe, 2012-07-31 14:55:28]

done. Thanks.

http://codereview.chromium.org/10815047/diff/23005/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10815047/diff/23005/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:103: file_name);
On 2012/07/31 14:40:17, battre wrote:
> nit: no need to wrap these lines.

Done.

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

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

[commit-bot: I haz the power, 2012-07-31 14:56:22]

Presubmit check for 10815047-22005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.3s to calculate.

[bshe, 2012-07-31 15:00:44]

+jhawkins for owners:
chrome/browser/chromeos/extensions/wallpaper_private_api.cc
chrome/common/*

Thanks

[James Hawkins, 2012-07-31 16:50:11]

lgtm

[commit-bot: I haz the power, 2012-07-31 18:45:25]

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

[commit-bot: I haz the power, 2012-07-31 20:08:16]

Change committed as 149259