Implement user selected wallpaper feature.

Implement user selected wallpaper feature.


BUG=123612, 122791
TEST=Verify if custom wallpaper is supported


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

[bshe, 2012-05-04 15:54:02]

Hi Rob.

Could you please take a look? Thanks!

[bshe, 2012-05-04 17:48:55]

+ben for ash/*
+jhawkins for options2/*

[flackr, 2012-05-04 19:06:17]

Mostly style / grammar nits.

https://chromiumcodereview.appspot.com/10375010/diff/2001/ash/desktop_backgro...
File ash/desktop_background/desktop_background_controller.cc (right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/ash/desktop_backgro...
ash/desktop_background/desktop_background_controller.cc:107: const SkBitmap&
wallpaper, WallpaperLayout layout) {
I believe we want 1 argument per line in function decl.

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

https://chromiumcodereview.appspot.com/10375010/diff/2001/ash/desktop_backgro...
ash/desktop_background/desktop_background_controller.h:54: // Called when user
select a file from file system or changed the layout.
s/select/selects
s/changed/changes

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/login/user_manager_impl.cc:59: #include
"skia/ext/image_operations.h"
Sort

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/login/user_manager_impl.cc:1249: void
UserManagerImpl::OnCustomWallpaperThumbnailLoaded(const std::string& email,
Push email to new line and line up arguments

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/login/user_manager_impl.cc:1303: if
(!SaveBitmapToFile(wallpaper, wallpaper_path))
So when you set a custom wallpaper we decode it, and then re-encode and save it?
I realize this is necessary for the thumbnail but we should probably save the
source wallpaper file unchanged.

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/login/user_manager_impl.cc:1520: /*
Remove testing code.

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/login/user_manager_impl.h:198: // Asynchronous load
custom wallpaper thumbnail.
Loads custom wallpaper thumbnail asynchronously.

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/login/user_manager_impl.h:203: // Cache the loaded
wallpaper to the given user.
s/Cache/Caches
s/to/for

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/login/user_manager_impl.h:207: // Generate a 128x80
thumbnail and save it local file system.
s/Generate/Generates
s/save/saves

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/chro...
chrome/browser/chromeos/login/user_manager_impl.h:242: // Save |image| to the
specified |image_path|. Runs on FILE thread.
s/Save/Saves

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/reso...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.html
(right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.html:26:
<button id="set-custome-wallpaper" i18n-content="customeWallpaper"></button>
do you mean custom (s/custome/custom)?

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/reso...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.js
(right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:113: * param
{string} layouts Available wallpaper layout.
s/layout/layouts

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:117:
populateWallpaperLayout_: function(layouts, selectedLayout) {
s/populateWallpaperLayout/populateWallpaperLayouts

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:146: //var
value = event.target.options[event.target.selectedIndex].value;
Remove commented code.

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:159: if
(url.indexOf(CustomWallpaperPrefix) != -1) {
This is strictly a prefix right? Maybe if == 0?

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:266: '?id='
+ (new Date()).getTime();
This is non-trivial and non-deterministic, I think you should use a regular
function for this.

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/reso...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:289:
$('set-wallpaper-layout').hidden = false;
We're updating the visibility of attribution-label and set-wallpaper-layout in
many places with the same conditions (custom, !custom), this should be
consolidated this into a single method.

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:59:
if (select_file_dialog_.get())
Only indent 2

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:259:
//SetCustomWallpaperThumb();
Remove commented code.

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:41:
// Sends layout options for custome wallpaper. Only exposed CENTER,
s/custome/custom

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:81:
scoped_refptr<SelectFileDialog> select_file_dialog_;
Comment.

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc
(right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc:63:
path.find_first_of("?") - strlen(kCustomWallpaperPrefix));
What if there is no "?"?

[Nikita (slow), 2012-05-05 14:32:11]

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

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:632:
SaveWallpaperToLocalState(username, file_path, layout, User::CUSTOMIZED);
So we've agreed to store custom wallpaper in Local State and to preload it on
sign in screen? I've thought that it should not be preloaded.

[bshe, 2012-05-05 18:59:44]

On 2012/05/05 14:32:11, Nikita Kostylev wrote:
>
http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/user_manager_impl.cc (right):
> 
>
http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/user_manager_impl.cc:632:
> SaveWallpaperToLocalState(username, file_path, layout, User::CUSTOMIZED);
> So we've agreed to store custom wallpaper in Local State and to preload it on
> sign in screen? I've thought that it should not be preloaded.

I believe so. The argument is we already allow user profile picture in sign in
screen, so it should not too bad to allow wallpaper. As long as user aware of
it.

[James Hawkins, 2012-05-06 15:49:49]

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.html
(right):

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.html:26:
<button id="set-custome-wallpaper" i18n-content="customeWallpaper"></button>
80 cols.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.html:28:
</select>
The closing tag should be on the line above.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.js
(right):

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:10: /**
@const */ var CustomWallpaperPrefix = 'chrome://wallpaper/custom_';
nit: Variables start lowerCase.

However, this is a const so it should be all caps.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:63:
this.oldImage_ = null;
Document.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:71:
void SetCustomWallpaperThumb();
'Thumb' is not an appropriate abbreviation for 'thumbnail'.

[Ben Goodger (Google), 2012-05-07 15:35:08]

ash LGTM

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

http://codereview.chromium.org/10375010/diff/2001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:157:
internal::CreateDesktopBackground(*wo->wallpaper(),
seems unnecessary?

[Ben Goodger (Google), 2012-05-07 15:36:25]

http://codereview.chromium.org/10375010/diff/2001/chrome/common/chrome_notifi...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/10375010/diff/2001/chrome/common/chrome_notifi...
chrome/common/chrome_notification_types.h:856:
NOTIFICATION_LOGIN_USER_WALLPAPER_THUMBNAIL_UPDATED,
Can you do this without notifications? Notifications are a generally horrible
system, and should be avoided.

[bshe, 2012-05-08 22:22:18]

+flackr
+jhawkins
Done.

Working on ben's review.

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

http://codereview.chromium.org/10375010/diff/2001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.cc:107: const SkBitmap&
wallpaper, WallpaperLayout layout) {
On 2012/05/04 19:06:17, flackr wrote:
> I believe we want 1 argument per line in function decl.

Done.

http://codereview.chromium.org/10375010/diff/2001/ash/desktop_background/desk...
File ash/desktop_background/desktop_background_controller.h (right):

http://codereview.chromium.org/10375010/diff/2001/ash/desktop_background/desk...
ash/desktop_background/desktop_background_controller.h:54: // Called when user
select a file from file system or changed the layout.
On 2012/05/04 19:06:17, flackr wrote:
> s/select/selects
> s/changed/changes

Done.

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

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:59: #include
"skia/ext/image_operations.h"
On 2012/05/04 19:06:17, flackr wrote:
> Sort

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:632:
SaveWallpaperToLocalState(username, file_path, layout, User::CUSTOMIZED);
On 2012/05/05 14:32:12, Nikita Kostylev wrote:
> So we've agreed to store custom wallpaper in Local State and to preload it on
> sign in screen? I've thought that it should not be preloaded.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:1249: void
UserManagerImpl::OnCustomWallpaperThumbnailLoaded(const std::string& email,
On 2012/05/04 19:06:17, flackr wrote:
> Push email to new line and line up arguments

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:1303: if
(!SaveBitmapToFile(wallpaper, wallpaper_path))
On 2012/05/04 19:06:17, flackr wrote:
> So when you set a custom wallpaper we decode it, and then re-encode and save
it?
> I realize this is necessary for the thumbnail but we should probably save the
> source wallpaper file unchanged.

Make sense. Could I do it in a separate CL (this is already a large CL)? It
doesn't look very trivial to me, unless I miss something. There is no easy way
to save a SkBitmap to file system without encoding. I can of course just save
the string read from file directly, but it involves changes to image_decoder.*
files, which included in lots of place. Does add a TODO here sounds fine to you?

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:1520: /*
On 2012/05/04 19:06:17, flackr wrote:
> Remove testing code.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.h (right):

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.h:198: // Asynchronous load
custom wallpaper thumbnail.
On 2012/05/04 19:06:17, flackr wrote:
> Loads custom wallpaper thumbnail asynchronously.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.h:203: // Cache the loaded
wallpaper to the given user.
On 2012/05/04 19:06:17, flackr wrote:
> s/Cache/Caches
> s/to/for

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.h:207: // Generate a 128x80
thumbnail and save it local file system.
On 2012/05/04 19:06:17, flackr wrote:
> s/Generate/Generates
> s/save/saves

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.h:242: // Save |image| to the
specified |image_path|. Runs on FILE thread.
On 2012/05/04 19:06:17, flackr wrote:
> s/Save/Saves

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.html
(right):

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.html:26:
<button id="set-custome-wallpaper" i18n-content="customeWallpaper"></button>
On 2012/05/04 19:06:17, flackr wrote:
> do you mean custom (s/custome/custom)?

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.html:26:
<button id="set-custome-wallpaper" i18n-content="customeWallpaper"></button>
On 2012/05/06 15:49:50, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.html:28:
</select>
On 2012/05/06 15:49:50, James Hawkins wrote:
> The closing tag should be on the line above.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.js
(right):

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:10: /**
@const */ var CustomWallpaperPrefix = 'chrome://wallpaper/custom_';
On 2012/05/06 15:49:50, James Hawkins wrote:
> nit: Variables start lowerCase.
> 
> However, this is a const so it should be all caps.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:63:
this.oldImage_ = null;
On 2012/05/06 15:49:50, James Hawkins wrote:
> Document.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:113: * param
{string} layouts Available wallpaper layout.
On 2012/05/04 19:06:17, flackr wrote:
> s/layout/layouts

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:117:
populateWallpaperLayout_: function(layouts, selectedLayout) {
On 2012/05/04 19:06:17, flackr wrote:
> s/populateWallpaperLayout/populateWallpaperLayouts

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:146: //var
value = event.target.options[event.target.selectedIndex].value;
On 2012/05/04 19:06:17, flackr wrote:
> Remove commented code.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:159: if
(url.indexOf(CustomWallpaperPrefix) != -1) {
On 2012/05/04 19:06:17, flackr wrote:
> This is strictly a prefix right? Maybe if == 0?

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:266: '?id='
+ (new Date()).getTime();
On 2012/05/04 19:06:17, flackr wrote:
> This is non-trivial and non-deterministic, I think you should use a regular
> function for this.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:289:
$('set-wallpaper-layout').hidden = false;
On 2012/05/04 19:06:17, flackr wrote:
> We're updating the visibility of attribution-label and set-wallpaper-layout in
> many places with the same conditions (custom, !custom), this should be
> consolidated this into a single method.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:59:
if (select_file_dialog_.get())
On 2012/05/04 19:06:17, flackr wrote:
> Only indent 2

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:259:
//SetCustomWallpaperThumb();
On 2012/05/04 19:06:17, flackr wrote:
> Remove commented code.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:41:
// Sends layout options for custome wallpaper. Only exposed CENTER,
On 2012/05/04 19:06:17, flackr wrote:
> s/custome/custom

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:71:
void SetCustomWallpaperThumb();
On 2012/05/06 15:49:50, James Hawkins wrote:
> 'Thumb' is not an appropriate abbreviation for 'thumbnail'.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:81:
scoped_refptr<SelectFileDialog> select_file_dialog_;
On 2012/05/04 19:06:17, flackr wrote:
> Comment.

Done.

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc
(right):

http://codereview.chromium.org/10375010/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc:63:
path.find_first_of("?") - strlen(kCustomWallpaperPrefix));
On 2012/05/04 19:06:17, flackr wrote:
> What if there is no "?"?

If the path can pass the first StartsWithASCII check, it should contain a "?" in
the url. But to be more safe, I added extra check as you suggested.

[James Hawkins, 2012-05-09 18:21:23]

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.js
(right):

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:112: *
Populate the drop down box for custom wallpaper layouts.
s/Populate/Populates/

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:245: *
Called when user select a valid file from file system. Display layout
nit: Start method documentation with what the method does, e.g. "Displays
layout...".  Then append the circumstances for when it's called, e.g. "Called
when...".

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:259: *
Return url of current user's custom wallpaper thumbnail.
nit: Returns.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:268: *
Update the visibility of attribution-label and set-wallpaper-layout.
nit: Updates.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:186:
downloads_path,
Save rows; consolidate parameters where possible to fill 80 cols.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:210:
return;
Don't handle NOTREACHED blocks by returning.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:41:
// Sends layout options for custom wallpaper. Only exposed CENTER,
s/exposed/exposes/

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:55:
// SelectFileDialog::Delegate implementation.
Interface implementations need to be grouped together.  I suggest moving this to
the top of the private section.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc
(right):

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc:59:
bool IsCustomWallpaperPath(const std::string& path, std::string& email) {
Don't use non-const ref.  Use a pointer instead.

[bshe, 2012-05-09 18:51:54]

+ben Could you please review patch set 4? Thanks!

https://chromiumcodereview.appspot.com/10375010/diff/2001/ash/desktop_backgro...
File ash/desktop_background/desktop_background_controller.cc (right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/ash/desktop_backgro...
ash/desktop_background/desktop_background_controller.cc:157:
internal::CreateDesktopBackground(*wo->wallpaper(),
On 2012/05/07 15:35:08, Ben Goodger (Google) wrote:
> seems unnecessary?

Sorry, just to clarify, do you mean two parameters are unnecessary (should be
just use one parameter)?

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/common/chrom...
File chrome/common/chrome_notification_types.h (right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/chrome/common/chrom...
chrome/common/chrome_notification_types.h:856:
NOTIFICATION_LOGIN_USER_WALLPAPER_THUMBNAIL_UPDATED,
On 2012/05/07 15:36:25, Ben Goodger (Google) wrote:
> Can you do this without notifications? Notifications are a generally horrible
> system, and should be avoided.

I use a delegate to replace the notification. Not sure if this is the correct
way. Could you please take a look?

[Ben Goodger (Google), 2012-05-09 19:53:13]

What files should I look in to find the differences?

https://chromiumcodereview.appspot.com/10375010/diff/2001/ash/desktop_backgro...
File ash/desktop_background/desktop_background_controller.cc (right):

https://chromiumcodereview.appspot.com/10375010/diff/2001/ash/desktop_backgro...
ash/desktop_background/desktop_background_controller.cc:157:
internal::CreateDesktopBackground(*wo->wallpaper(),
On 2012/05/09 18:51:54, bshe wrote:
> On 2012/05/07 15:35:08, Ben Goodger (Google) wrote:
> > seems unnecessary?
> 
> Sorry, just to clarify, do you mean two parameters are unnecessary (should be
> just use one parameter)?

The linewrapping is unnecessary.

[bshe, 2012-05-09 19:59:39]

On 2012/05/09 19:53:13, Ben Goodger (Google) wrote:
> What files should I look in to find the differences?
> 
>
https://chromiumcodereview.appspot.com/10375010/diff/2001/ash/desktop_backgro...
> File ash/desktop_background/desktop_background_controller.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10375010/diff/2001/ash/desktop_backgro...
> ash/desktop_background/desktop_background_controller.cc:157:
> internal::CreateDesktopBackground(*wo->wallpaper(),
> On 2012/05/09 18:51:54, bshe wrote:
> > On 2012/05/07 15:35:08, Ben Goodger (Google) wrote:
> > > seems unnecessary?
> > 
> > Sorry, just to clarify, do you mean two parameters are unnecessary (should
be
> > just use one parameter)?
> 
> The linewrapping is unnecessary.

Sorry. The files are:
chrome/browser/chromeos/login/user_manager.h
chrome/browser/chromeos/login/user_manager_impl.h
chrome/browser/chromeos/login/user_manager_impl.cc
for delegate plumbing and
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc
for removing the notifications.
Basically everything in patchset 4(delta 3).

[bshe, 2012-05-09 20:37:14]

+james for patchset 5
chrome/browser/ui/webui/*
chrome/browser/resources/options2

Thanks!

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/set_wallpaper_options.js
(right):

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:112: *
Populate the drop down box for custom wallpaper layouts.
On 2012/05/09 18:21:25, James Hawkins wrote:
> s/Populate/Populates/

Done.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:245: *
Called when user select a valid file from file system. Display layout
On 2012/05/09 18:21:25, James Hawkins wrote:
> nit: Start method documentation with what the method does, e.g. "Displays
> layout...".  Then append the circumstances for when it's called, e.g. "Called
> when...".

Done.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:259: *
Return url of current user's custom wallpaper thumbnail.
On 2012/05/09 18:21:25, James Hawkins wrote:
> nit: Returns.

Done.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js:268: *
Update the visibility of attribution-label and set-wallpaper-layout.
On 2012/05/09 18:21:25, James Hawkins wrote:
> nit: Updates.

Done.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:186:
downloads_path,
On 2012/05/09 18:21:25, James Hawkins wrote:
> Save rows; consolidate parameters where possible to fill 80 cols.

Done.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:41:
// Sends layout options for custom wallpaper. Only exposed CENTER,
On 2012/05/09 18:21:25, James Hawkins wrote:
> s/exposed/exposes/

Done.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:55:
// SelectFileDialog::Delegate implementation.
On 2012/05/09 18:21:25, James Hawkins wrote:
> Interface implementations need to be grouped together.  I suggest moving this
to
> the top of the private section. 

Done.

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc
(right):

http://codereview.chromium.org/10375010/diff/13003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc:59:
bool IsCustomWallpaperPath(const std::string& path, std::string& email) {
On 2012/05/09 18:21:25, James Hawkins wrote:
> Don't use non-const ref.  Use a pointer instead.

Done.

[flackr, 2012-05-09 21:26:16]

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

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.h:54: // Called when user
selects a file from file system or changes the layout.
Describe what the function does, after which you can mention when it's called.
FYI some of the other comments in this file are wrong, should be "Cancels
the..", "Loads logged in..".

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
ash/desktop_background/desktop_background_view.cc:131: WallpaperLayout layout) {
As before, s/layout/wallpaper_layout.

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
File ash/desktop_background/desktop_background_view.h (right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
ash/desktop_background/desktop_background_view.h:19: DesktopBackgroundView(const
SkBitmap& wallpaper, WallpaperLayout layout);
s/layout/wallpaper_layout for consistency.

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/shell_factory.h
File ash/shell_factory.h (right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/shell_factory....
ash/shell_factory.h:25: WallpaperLayout layout);
s/layout/wallpaper_layout

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
File chrome/browser/chromeos/login/user.cc (right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user.cc:64: void User::SetWallpaperThumbnail(const
SkBitmap& wallpaper_thumb) {
You should probably change wallpaper_thumb to wallpaper_thumbnail too.

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

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager.h:179: // Update custom wallpaper to
selected layout and saves layout to Local
s/Update/Updates

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

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:659: // For wallpapers, save
the image without resize.
s/resize/resizing

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1285: kThumbnailWidth,
kThumbnailHeight);
Will this be the correct aspect ratio?

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1323: // TODO(bshe): We
should save the original file unchanged instead of re-encode
s/re-encode/re-encoding

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1324: // it and save it.
s/it and save/and saving

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1373: // TODO(bshe): we
probably need to save wallpaper_path instead of index.
s/we/We

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:98:
void SetWallpaperOptionsHandler::FileSelected(const FilePath& path, int index,
One parameter per line when wrapping decl.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:145:
int index = layout;
Should be able to use:
FundamentalValue selected_value(static_cast<int>(layout));

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:185:
return;
Either we expect this case or don't return for NOTREACHED.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:70:
// Redraw the wallpaper with specified wallpaper layout.
s/Redraw/Redraws

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc
(right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc:61:
path.find_first_of("?") == std::string::npos)
Not having a ? doesn't mean it's definitely not a custom wallpaper path, even
though the convention is that we always put the ? in. Can you move the check for
? to change the substr?

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc:64:
path.find_first_of("?") - strlen(kCustomWallpaperPrefix));
std::string::substr can take just start position, this will be much cleaner.

[Nikita (slow), 2012-05-10 12:19:50]

On 2012/05/05 18:59:44, bshe wrote:
> On 2012/05/05 14:32:11, Nikita Kostylev wrote:
> >
>
http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
> > File chrome/browser/chromeos/login/user_manager_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/10375010/diff/2001/chrome/browser/chromeos/log...
> > chrome/browser/chromeos/login/user_manager_impl.cc:632:
> > SaveWallpaperToLocalState(username, file_path, layout, User::CUSTOMIZED);
> > So we've agreed to store custom wallpaper in Local State and to preload it
on
> > sign in screen? I've thought that it should not be preloaded.
> 
> I believe so. The argument is we already allow user profile picture in sign in
> screen, so it should not too bad to allow wallpaper. As long as user aware of
> it.

Right, so last think Alex commented was:
We may warn you when you do it but I don't necessarily agree this is a huge
issue. We allow you to set random profile pics and show them. We could add an
option ( [ ] Don't show on login/lock screen) but I'd think we can go without
it.

[bshe, 2012-05-10 16:10:25]

+flackr

Done. Could you please take a look at patchset 7? Thanks!

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

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
ash/desktop_background/desktop_background_controller.h:54: // Called when user
selects a file from file system or changes the layout.
On 2012/05/09 21:26:16, flackr wrote:
> Describe what the function does, after which you can mention when it's called.
> FYI some of the other comments in this file are wrong, should be "Cancels
> the..", "Loads logged in..".

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
File ash/desktop_background/desktop_background_view.cc (right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
ash/desktop_background/desktop_background_view.cc:131: WallpaperLayout layout) {
On 2012/05/09 21:26:16, flackr wrote:
> As before, s/layout/wallpaper_layout.

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
File ash/desktop_background/desktop_background_view.h (right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/desktop_backgr...
ash/desktop_background/desktop_background_view.h:19: DesktopBackgroundView(const
SkBitmap& wallpaper, WallpaperLayout layout);
On 2012/05/09 21:26:16, flackr wrote:
> s/layout/wallpaper_layout for consistency.

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/shell_factory.h
File ash/shell_factory.h (right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/ash/shell_factory....
ash/shell_factory.h:25: WallpaperLayout layout);
On 2012/05/09 21:26:16, flackr wrote:
> s/layout/wallpaper_layout

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
File chrome/browser/chromeos/login/user.cc (right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user.cc:64: void User::SetWallpaperThumbnail(const
SkBitmap& wallpaper_thumb) {
On 2012/05/09 21:26:16, flackr wrote:
> You should probably change wallpaper_thumb to wallpaper_thumbnail too.

Done.

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

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager.h:179: // Update custom wallpaper to
selected layout and saves layout to Local
On 2012/05/09 21:26:16, flackr wrote:
> s/Update/Updates

Done.

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

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:659: // For wallpapers, save
the image without resize.
On 2012/05/09 21:26:16, flackr wrote:
> s/resize/resizing

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1285: kThumbnailWidth,
kThumbnailHeight);
On 2012/05/09 21:26:16, flackr wrote:
> Will this be the correct aspect ratio?

No. It just resizes the wallpaper to 128x80. Ideally, the thumbnail should be a
preview (with correct user selected wallpaper layout). I plan to submit a
separate CL for it and it is tracked in issue 127569.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1323: // TODO(bshe): We
should save the original file unchanged instead of re-encode
On 2012/05/09 21:26:16, flackr wrote:
> s/re-encode/re-encoding

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1324: // it and save it.
On 2012/05/09 21:26:16, flackr wrote:
> s/it and save/and saving

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/chr...
chrome/browser/chromeos/login/user_manager_impl.cc:1373: // TODO(bshe): we
probably need to save wallpaper_path instead of index.
On 2012/05/09 21:26:16, flackr wrote:
> s/we/We

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:98:
void SetWallpaperOptionsHandler::FileSelected(const FilePath& path, int index,
On 2012/05/09 21:26:16, flackr wrote:
> One parameter per line when wrapping decl.

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:145:
int index = layout;
On 2012/05/09 21:26:16, flackr wrote:
> Should be able to use:
> FundamentalValue selected_value(static_cast<int>(layout));

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc:185:
return;
On 2012/05/09 21:26:16, flackr wrote:
> Either we expect this case or don't return for NOTREACHED.

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:70:
// Redraw the wallpaper with specified wallpaper layout.
On 2012/05/09 21:26:16, flackr wrote:
> s/Redraw/Redraws

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
File chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc
(right):

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc:61:
path.find_first_of("?") == std::string::npos)
On 2012/05/09 21:26:16, flackr wrote:
> Not having a ? doesn't mean it's definitely not a custom wallpaper path, even
> though the convention is that we always put the ? in. Can you move the check
for
> ? to change the substr?

Done.

https://chromiumcodereview.appspot.com/10375010/diff/13007/chrome/browser/ui/...
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc:64:
path.find_first_of("?") - strlen(kCustomWallpaperPrefix));
On 2012/05/09 21:26:16, flackr wrote:
> std::string::substr can take just start position, this will be much cleaner.

Done.

[flackr, 2012-05-11 03:52:57]

LGTM with caveat.

The operation of setting a wallpaper (especially a custom wallpaper) seems
complex enough to merit testing coverage. Can you file a bug to write some
wallpaper tests?

[bshe, 2012-05-11 18:06:47]

friendly ping

+ben for patchset 4(delta 3)
chrome/browser/chromeos/login/user_manager.h
chrome/browser/chromeos/login/user_manager_impl.h
chrome/browser/chromeos/login/user_manager_impl.cc
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc

+jhawkins for patchset 5 (delta 4 against delta 2)
chrome/browser/resources/options2/chromeos/set_wallpaper_options.js
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.cc
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
chrome/browser/ui/webui/options2/chromeos/wallpaper_thumbnail_source2.cc

[James Hawkins, 2012-05-11 18:13:23]

LGTM

http://codereview.chromium.org/10375010/diff/16023/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h
(right):

http://codereview.chromium.org/10375010/diff/16023/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/set_wallpaper_options_handler2.h:25:
virtual void SetCustomWallpaperThumbnail() = 0;
nit: Add protected destructor.

[Ben Goodger (Google), 2012-05-14 21:21:04]

lgtm

[commit-bot: I haz the power, 2012-05-15 18:34:35]

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

[commit-bot: I haz the power, 2012-05-15 21:41:43]

Change committed as 137258