New custom wallpaper picker UI

New custom wallpaper picker UI


This CL implement the following:
1. display all thumbnails of user previously selected custom wallpapers in a grid
2. when user click any thumbnail in the grid, sets wallpaper to the corresponding one with
layout gets from chrome.storage.local
3. implement UI for adding new custom wallpaper, mock:
https://docs.google.com/a/google.com/file/d/0B6x6iYCtKinEcEhwSjdEWUpRTFk/edit?usp=sharing


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

[bshe, 2013-02-22 05:36:22]

Hey Rob.

Could you please take a look at this CL? Thanks!

[bshe, 2013-02-26 14:57:46]

On 2013/02/22 05:36:22, bshe wrote:
> Hey Rob.
> 
> Could you please take a look at this CL? Thanks!

Hi Rob. Could you please take a look?

[flackr, 2013-02-26 17:16:01]

The wallpaper manager is getting rather complex and large and really needs
testing.

Can you give me (and update the CL description) with a rough idea what's going
on. It looks like you're saving multiple custom wallpapers and going to show a
list of them with saved layout settings?

[bshe, 2013-02-26 19:07:26]

Sorry. I forgot to add the description.

You are right. The main purpose of this CL is show a
list of custom wallpapers and allow users chose from them. So user can get back
to previous wallpaper they use if they are not satisfy with their new
wallpapers. Please see the issue description for details.

I will add a function to allow user remove specific wallpaper in the grid from
context menu (proposed by UX) in a follow up CL.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
(right):

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:42:

This element is the grid element responsible for open "choose wallpaper" overlay
(https://docs.google.com/a/google.com/file/d/0B6x6iYCtKinEcEhwSjdEWUpRTFk/edit...).

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:252:
if (this.currentWallpaper_ &&
the currentWallpaper is either a url contains HightResolutionSuffix or a custom
wallpaper file name (e.g. 13006377367586070, a string converted from an integer
value represent time).

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:326:
*/
Most of the code in this function is moved from onThumbnailClicked_. I only add
support for custom wallpaper.

[flackr, 2013-02-26 21:37:34]

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:252:
if (this.currentWallpaper_ &&
On 2013/02/26 19:07:26, bshe wrote:
> the currentWallpaper is either a url contains HightResolutionSuffix or a
custom
> wallpaper file name (e.g. 13006377367586070, a string converted from an
integer
> value represent time).

Great, I was going to comment on this. Can you add this in a comment so it's
clear why this is a safe check. Initially I thought it was just checked against
the custom wallpaper's filename.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:269:
if (url.indexOf(self.currentWallpaper_) != -1)
nit: I believe you need braces for a multi-line logic block.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:271:
OnlineCategoriesOffset;
You should break when you've found the category.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:330:
if (selectedItem.source == 'ONLINE')
nit: constants for source values.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:331:
wallpaperURL += HighResolutionSuffix;
TODO: Use non high resolution wallpaper when appropriate?

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:386:
!this.wallpaperGrid_.inProgramSelection) {
pardon my ignorance, but what does inProgramSelection mean?

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:535:
* @param {string=} opt_fileName The file name used to save the new selected
When is the filename optional / not passed?

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:621:
layout: 'CENTER_CROPPED',
So this is later replaced with the last used layout when you set the wallpaper?

[bshe, 2013-02-27 03:16:38]

Done. Thanks for review.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:252:
if (this.currentWallpaper_ &&
On 2013/02/26 21:37:35, flackr wrote:
> On 2013/02/26 19:07:26, bshe wrote:
> > the currentWallpaper is either a url contains HightResolutionSuffix or a
> custom
> > wallpaper file name (e.g. 13006377367586070, a string converted from an
> integer
> > value represent time).
> 
> Great, I was going to comment on this. Can you add this in a comment so it's
> clear why this is a safe check. Initially I thought it was just checked
against
> the custom wallpaper's filename.

Done.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:269:
if (url.indexOf(self.currentWallpaper_) != -1)
On 2013/02/26 21:37:35, flackr wrote:
> nit: I believe you need braces for a multi-line logic block.

Done.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:271:
OnlineCategoriesOffset;
On 2013/02/26 21:37:35, flackr wrote:
> You should break when you've found the category.

Done.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:330:
if (selectedItem.source == 'ONLINE')
On 2013/02/26 21:37:35, flackr wrote:
> nit: constants for source values.

Done.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:331:
wallpaperURL += HighResolutionSuffix;
There are changes in the design on how to fetch wallpaper from server. As you
probably remember, we originally plan to download high and low resolution
wallpapers for each devices. But later we decided to just download a high
resolution wallpaper and resize it in chrome to get small resolution wallpaper.
The resize quality is comparable to server and we can reduce the bandwidth
usage. So a TODO is probably not necessary here?

On 2013/02/26 21:37:35, flackr wrote:
> TODO: Use non high resolution wallpaper when appropriate?

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:386:
!this.wallpaperGrid_.inProgramSelection) {
Sorry for confusion. When the current thumbnail selection change is triggered by
program, not user's interaction, this inProgramSelection is true. Please see
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/resources/c...
for more detail.

When the wallpaper picker first open, we will automatically select the thumbnail
of current wallpaper. It triggers a selection change. But we dont want to call
api function to set wallpaper for the selection change. So inProgramSelection is
set to true to prevent us from doing that.

On 2013/02/26 21:37:35, flackr wrote:
> pardon my ignorance, but what does inProgramSelection mean?

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:535:
* @param {string=} opt_fileName The file name used to save the new selected
When custom wallpaper layout change finished, this function will be called
without a fileName (in function onWallpaperLayoutChanged_). As for layout
change, we dont save a new file into file system. So the fileName is null.

On 2013/02/26 21:37:35, flackr wrote:
> When is the filename optional / not passed?

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:621:
layout: 'CENTER_CROPPED',
Right. It will be replaced by the value in local storage when set. The reason I
use CENTER_CROPPED layout here is to avoid wait for fetching layout from local
storage, hence speed up the process of displaying thumbnail grid.

On 2013/02/26 21:37:35, flackr wrote:
> So this is later replaced with the last used layout when you set the
wallpaper?

[flackr, 2013-02-27 14:39:52]

Are custom wallpapers saved in a public folder where other users can see them in
their custom list?

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:331:
wallpaperURL += HighResolutionSuffix;
In light of that, I'm surprised we have a high resolution suffix. Are there only
high res images available online? If we're okay with downloading the extra large
image on devices which only need a low res image we should remove the suffix
altogether / add it early on when we fetch the manifest if we need to keep it
for backwards compatibility. We should discuss this.

On 2013/02/27 03:16:38, bshe wrote:
> There are changes in the design on how to fetch wallpaper from server. As you
> probably remember, we originally plan to download high and low resolution
> wallpapers for each devices. But later we decided to just download a high
> resolution wallpaper and resize it in chrome to get small resolution
wallpaper.
> The resize quality is comparable to server and we can reduce the bandwidth
> usage. So a TODO is probably not necessary here?
> 
> On 2013/02/26 21:37:35, flackr wrote:
> > TODO: Use non high resolution wallpaper when appropriate?
>

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:535:
* @param {string=} opt_fileName The file name used to save the new selected
Comment this please.

On 2013/02/27 03:16:38, bshe wrote:
> When custom wallpaper layout change finished, this function will be called
> without a fileName (in function onWallpaperLayoutChanged_). As for layout
> change, we dont save a new file into file system. So the fileName is null.
> 
> On 2013/02/26 21:37:35, flackr wrote:
> > When is the filename optional / not passed?
>

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:621:
layout: 'CENTER_CROPPED',
Please add comment to this effect.

On 2013/02/27 03:16:38, bshe wrote:
> Right. It will be replaced by the value in local storage when set. The reason
I
> use CENTER_CROPPED layout here is to avoid wait for fetching layout from local
> storage, hence speed up the process of displaying thumbnail grid.
> 
> On 2013/02/26 21:37:35, flackr wrote:
> > So this is later replaced with the last used layout when you set the
> wallpaper?
>

[bshe, 2013-03-01 17:43:56]

Hey Rob.

I have completely transit custom wallpapers to FileSystem as you suggested. Now
only one custom wallpaper is saved in the shared directory.

Could you please take a look?

Thanks!

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:331:
wallpaperURL += HighResolutionSuffix;
Discussed offline. We should investigate removing the high resolution suffix
from the server and client.

On 2013/02/27 14:39:52, flackr wrote:
> In light of that, I'm surprised we have a high resolution suffix. Are there
only
> high res images available online? If we're okay with downloading the extra
large
> image on devices which only need a low res image we should remove the suffix
> altogether / add it early on when we fetch the manifest if we need to keep it
> for backwards compatibility. We should discuss this.
> 
> On 2013/02/27 03:16:38, bshe wrote:
> > There are changes in the design on how to fetch wallpaper from server. As
you
> > probably remember, we originally plan to download high and low resolution
> > wallpapers for each devices. But later we decided to just download a high
> > resolution wallpaper and resize it in chrome to get small resolution
> wallpaper.
> > The resize quality is comparable to server and we can reduce the bandwidth
> > usage. So a TODO is probably not necessary here?
> > 
> > On 2013/02/26 21:37:35, flackr wrote:
> > > TODO: Use non high resolution wallpaper when appropriate?
> >

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:535:
* @param {string=} opt_fileName The file name used to save the new selected
On 2013/02/27 14:39:52, flackr wrote:
> Comment this please.
> 
> On 2013/02/27 03:16:38, bshe wrote:
> > When custom wallpaper layout change finished, this function will be called
> > without a fileName (in function onWallpaperLayoutChanged_). As for layout
> > change, we dont save a new file into file system. So the fileName is null.
> > 
> > On 2013/02/26 21:37:35, flackr wrote:
> > > When is the filename optional / not passed?
> > 
> 

Done.

https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:621:
layout: 'CENTER_CROPPED',
On 2013/02/27 14:39:52, flackr wrote:
> Please add comment to this effect.
> 
> On 2013/02/27 03:16:38, bshe wrote:
> > Right. It will be replaced by the value in local storage when set. The
reason
> I
> > use CENTER_CROPPED layout here is to avoid wait for fetching layout from
local
> > storage, hence speed up the process of displaying thumbnail grid.
> > 
> > On 2013/02/26 21:37:35, flackr wrote:
> > > So this is later replaced with the last used layout when you set the
> > wallpaper?
> > 
> 

Done.

[bshe, 2013-03-01 17:45:32]

There are more clean up to do. But the main part is here. I may separate these
clean ups in a follow up CL since this is already a large CL. Thanks!

On 2013/03/01 17:43:56, bshe wrote:
> Hey Rob.
> 
> I have completely transit custom wallpapers to FileSystem as you suggested.
Now
> only one custom wallpaper is saved in the shared directory.
> 
> Could you please take a look?
> 
> Thanks!
> 
>
https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
> File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
> (right):
> 
>
https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:331:
> wallpaperURL += HighResolutionSuffix;
> Discussed offline. We should investigate removing the high resolution suffix
> from the server and client.
> 
> On 2013/02/27 14:39:52, flackr wrote:
> > In light of that, I'm surprised we have a high resolution suffix. Are there
> only
> > high res images available online? If we're okay with downloading the extra
> large
> > image on devices which only need a low res image we should remove the suffix
> > altogether / add it early on when we fetch the manifest if we need to keep
it
> > for backwards compatibility. We should discuss this.
> > 
> > On 2013/02/27 03:16:38, bshe wrote:
> > > There are changes in the design on how to fetch wallpaper from server. As
> you
> > > probably remember, we originally plan to download high and low resolution
> > > wallpapers for each devices. But later we decided to just download a high
> > > resolution wallpaper and resize it in chrome to get small resolution
> > wallpaper.
> > > The resize quality is comparable to server and we can reduce the bandwidth
> > > usage. So a TODO is probably not necessary here?
> > > 
> > > On 2013/02/26 21:37:35, flackr wrote:
> > > > TODO: Use non high resolution wallpaper when appropriate?
> > >
> 
>
https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:535:
> * @param {string=} opt_fileName The file name used to save the new selected
> On 2013/02/27 14:39:52, flackr wrote:
> > Comment this please.
> > 
> > On 2013/02/27 03:16:38, bshe wrote:
> > > When custom wallpaper layout change finished, this function will be called
> > > without a fileName (in function onWallpaperLayoutChanged_). As for layout
> > > change, we dont save a new file into file system. So the fileName is null.
> > > 
> > > On 2013/02/26 21:37:35, flackr wrote:
> > > > When is the filename optional / not passed?
> > > 
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/12334030/diff/5001/chrome/browser/resources/c...
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:621:
> layout: 'CENTER_CROPPED',
> On 2013/02/27 14:39:52, flackr wrote:
> > Please add comment to this effect.
> > 
> > On 2013/02/27 03:16:38, bshe wrote:
> > > Right. It will be replaced by the value in local storage when set. The
> reason
> > I
> > > use CENTER_CROPPED layout here is to avoid wait for fetching layout from
> local
> > > storage, hence speed up the process of displaying thumbnail grid.
> > > 
> > > On 2013/02/26 21:37:35, flackr wrote:
> > > > So this is later replaced with the last used layout when you set the
> > > wallpaper?
> > > 
> > 
> 
> Done.

[bshe, 2013-03-06 15:44:33]

friendly ping?

Also add more owners for the following:
+miket for extension/*
+nkostylev for login/*

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:634:
fileWriter.write(file);
duplicate the selected file before we try to set it as wallpaper in the event
handler for writeend. The duplicated file will be deleted if setCustomWallpaper
failed.

[flackr, 2013-03-06 16:22:11]

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:609:
fileWriter.write(blob);
What happens when we run out of quota? We should bump the oldest wallpapers out
right? If you want to simplify things for now you could request unlimited quota
though I imagine having a limit is the right thing to do long-term.

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/manifest.json (right):

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/manifest.json:15:
"unlimitedStorage",
Is unlimitedStorage necessary to get the 100MB you request?

[Nikita (slow), 2013-03-06 16:51:17]

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:381: return
std::string(reinterpret_cast<const char*>(data->front()),
Is this the only way to return resized Wallpaper?
I would expect this to be an output parameter with array of char* and
ResizeWallpaper returning bool.

[flackr, 2013-03-06 18:22:24]

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:381: return
std::string(reinterpret_cast<const char*>(data->front()),
On 2013/03/06 16:51:17, Nikita Kostylev wrote:
> Is this the only way to return resized Wallpaper?
> I would expect this to be an output parameter with array of char* and
> ResizeWallpaper returning bool.

But we don't know in advance the size of the resized image. I could see an
output vector.

[bshe, 2013-03-07 05:34:35]

addressed all comments.

flackr@ and nkostylev@ could you please take another look?

Thanks!

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:381: return
std::string(reinterpret_cast<const char*>(data->front()),
I changed the signature according to nikita's suggestion. Also, I use ref
counted bytes in the hope of reduce unnecessary copy.

On 2013/03/06 18:22:24, flackr wrote:
> On 2013/03/06 16:51:17, Nikita Kostylev wrote:
> > Is this the only way to return resized Wallpaper?
> > I would expect this to be an output parameter with array of char* and
> > ResizeWallpaper returning bool.
> 
> But we don't know in advance the size of the resized image. I could see an
> output vector.

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:609:
fileWriter.write(blob);
Added a simple function to remove the oldest file for now. We may need better
approach.

On 2013/03/06 16:22:11, flackr wrote:
> What happens when we run out of quota? We should bump the oldest wallpapers
out
> right? If you want to simplify things for now you could request unlimited
quota
> though I imagine having a limit is the right thing to do long-term.

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/manifest.json (right):

https://codereview.chromium.org/12334030/diff/21001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/manifest.json:15:
"unlimitedStorage",
talked offline. I think so.

On 2013/03/06 16:22:11, flackr wrote:
> Is unlimitedStorage necessary to get the 100MB you request?

[Nikita (slow), 2013-03-07 11:03:12]

lgtm

[flackr, 2013-03-07 14:46:47]

Looks good, just a few comments.

I'm not sure adding new wallpapers to the front makes sense from a UX
perspective. I'd expect the wallpaper to go where the + button was, at the end
of the list.

Can you file a bug to add tests for the wallpaper manager?

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:399: &data)) {
If the image is smaller than the preferred width and height will the code know
to use the original image even though this doesn't return anything?

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:448:
this.removeOldestWallpaper_();
This won't retry saving the wallpaper though right? This should remove enough
wallpapers to get the necessary space to add the new one but you can split this
out into a new bug / leave a TODO.

[bshe, 2013-03-07 16:13:05]

Thanks for review!

I have created bugs to track test and quota. For the wallpaper order issue, it
is not specified where should the new thumbnail be added in the mock. I assumed
it should insert to the beginning because it is the most nature for me.  I will
double check with UX folks and get their input on this. It will also be in a
separate CL if we want to reverse the order.

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/wallpaper_manager.cc:399: &data)) {
If it is smaller than preferred size, we wont try to resize and save that
wallpaper. And we will automatically fallback to use the original wallpaper if
we can't find wallpaper at requested resolution.

On 2013/03/07 14:46:47, flackr wrote:
> If the image is smaller than the preferred width and height will the code know
> to use the original image even though this doesn't return anything?

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:448:
this.removeOldestWallpaper_();
Right. If we retry to save wallpaper here, it may enter to an infinite loop if
somehow removeOldestWallpaper_ failed. I  imagine there are more extra
complexity associated with it. So definitely create a new bug.

Instead of removing oldest ones automatically, we may want to ask user to delete
wallpapers themselves. Oldest may not be a very good hint for deleting? Anyway,
bug filed: https://code.google.com/p/chromium/issues/detail?id=180890

On 2013/03/07 14:46:47, flackr wrote:
> This won't retry saving the wallpaper though right? This should remove enough
> wallpapers to get the necessary space to add the new one but you can split
this
> out into a new bug / leave a TODO.

[flackr, 2013-03-07 18:03:17]

lgtm

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/12334030/diff/38001/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:448:
this.removeOldestWallpaper_();
Thanks. Please add TODO referencing bug.

On 2013/03/07 16:13:05, bshe wrote:
> Right. If we retry to save wallpaper here, it may enter to an infinite loop if
> somehow removeOldestWallpaper_ failed. I  imagine there are more extra
> complexity associated with it. So definitely create a new bug.
> 
> Instead of removing oldest ones automatically, we may want to ask user to
delete
> wallpapers themselves. Oldest may not be a very good hint for deleting?
Anyway,
> bug filed: https://code.google.com/p/chromium/issues/detail?id=180890
> 
> On 2013/03/07 14:46:47, flackr wrote:
> > This won't retry saving the wallpaper though right? This should remove
enough
> > wallpapers to get the necessary space to add the new one but you can split
> this
> > out into a new bug / leave a TODO.
>

[miket_OOO, 2013-03-07 18:44:32]

> +miket for extension/*

LGTM

[commit-bot: I haz the power, 2013-03-08 03:59:12]

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

[commit-bot: I haz the power, 2013-03-08 07:17:02]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-03-10 01:02:29]

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

[commit-bot: I haz the power, 2013-03-10 03:49:30]

Change committed as 187184