Make wallpaper picker manifest and thumbnails available when offline.

Make wallpaper picker manifest and thumbnails available when offline.

This CL did the following:

1. Save/update manifest in chrome.storage.local when user successfully
requested latest manifest from server. If user failed to get manifest
from server(offline or server error), fallback to the manifest saved
in chrome.storage.local last time.

2. Lazily saves all requested thumbnails to thumbnails directory. And
load from that directory when user open wallpaper picker next time. Note
that thumbnails are shared across user session. So after one user saved
thumbnails, all other users will directly use the thumbnails that saved
in thumbnail directory.


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

[bshe, 2012-11-23 20:00:12]

Hi Rob.

This CL should make wallpaper picker manifest and thumbnails available when
offline.

Could you please take a look?

Thanks!

Biao

[bshe, 2012-11-26 22:14:38]

+miket for extensions/*

Thanks!

[flackr, 2012-11-27 22:04:36]

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:456:
SetError("Failed to create wallpaper thumbnails directory.");
Not localized, is this displayed to the user?

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:540:
SetError("Failed to save thumbnail");
ditto.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:549: const
std::string& file_name) {
Seems like you should be able to share this saving and loading file
functionality with the wallpaper saving / loading code. Can you use some local
anonymous functions to do the core work?

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:571: this));
return; ?

Won't this proceed to post a success message otherwise?

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
(right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:57:
var blob = new Blob([data]);
Combine these two lines, you only use data here.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:62:
self.appendChild(imageEl);
The list of thumbnails grow as we fetch them? We should consider using a
placeholder "loading" image, reserving space so the thumbnails don't shift.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:130:
this.onLoadManifestFailed_.bind(this));
If we're offline won't it take a while to fail to download the manifest? We may
want to either show the local manifest and then update with the new one when
it's available or detect the connection state.

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
File chrome/common/extensions/api/wallpaper_private.json (right):

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
chrome/common/extensions/api/wallpaper_private.json:141: "description":
"Wallpaper url."
Just out of convention I think the url "key" should be first. We're setting the
wallpaper thumbnail with key "url" to be "data" or thumbnail[key] = url.

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
chrome/common/extensions/api/wallpaper_private.json:147: "parameters": [],
Saving the thumbnail can also fail, you may want to add this parameter to the
callback so that the test can accurately detect what the failure is.

[miket_OOO, 2012-11-28 00:17:19]

extensions LGTM but please see comments.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:438: if (args_ ==
NULL || !args_->GetString(0, &url) || url.empty()) {
You ought to be able to use EXTENSION_FUNCTION_VALIDATE here, and then the
validation will be performed automatically and consistently with the IDL
declaration. (I'm assuming that the same rules apply even though this is in
chromeos/.)

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:488: this));
no need to wrap (same with below)

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:571: this));
On 2012/11/27 22:04:36, flackr wrote:
> return; ?
> 
> Won't this proceed to post a success message otherwise?

Good catch.

[bshe, 2012-11-28 18:54:25]

Thanks for review.

I have addressed all comments. And I also improved the error message returned
from api function call.

PTAL. thanks!

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:438: if (args_ ==
NULL || !args_->GetString(0, &url) || url.empty()) {
On 2012/11/28 00:17:19, miket wrote:
> You ought to be able to use EXTENSION_FUNCTION_VALIDATE here, and then the
> validation will be performed automatically and consistently with the IDL
> declaration. (I'm assuming that the same rules apply even though this is in
> chromeos/.)

Done.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:456:
SetError("Failed to create wallpaper thumbnails directory.");
It is not displayed to user. The js console and chrome logs may display the
message. But for users, they shouldn't be able to invoke the js console. So it
is probably no need to localize?

On 2012/11/27 22:04:36, flackr wrote:
> Not localized, is this displayed to the user?

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:488: this));
On 2012/11/28 00:17:19, miket wrote:
> no need to wrap (same with below)

Done.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:540:
SetError("Failed to save thumbnail");
ditto.

On 2012/11/27 22:04:36, flackr wrote:
> ditto.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:549: const
std::string& file_name) {
On 2012/11/27 22:04:36, flackr wrote:
> Seems like you should be able to share this saving and loading file
> functionality with the wallpaper saving / loading code. Can you use some local
> anonymous functions to do the core work?

Done.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:571: this));
right. I must accidentally replaced the return with the PostTask.

Done.

On 2012/11/27 22:04:36, flackr wrote:
> return; ?
> 
> Won't this proceed to post a success message otherwise?

https://codereview.chromium.org/11348215/diff/19/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:571: this));
On 2012/11/28 00:17:19, miket wrote:
> On 2012/11/27 22:04:36, flackr wrote:
> > return; ?
> > 
> > Won't this proceed to post a success message otherwise?
> 
> Good catch.

Done.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
(right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:57:
var blob = new Blob([data]);
On 2012/11/27 22:04:36, flackr wrote:
> Combine these two lines, you only use data here.

Done.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:62:
self.appendChild(imageEl);
The img tag is added to a div. And the div has width/height equal to the
thumbnail image. So the thumbnails shouldn't shift as it loading.

Add a placeholder image sounds great. We just reserve the space currently. Can
we keep it this way right now though? Since I dont have the resource and it may
take a while to get it. Added a todo.

On 2012/11/27 22:04:36, flackr wrote:
> The list of thumbnails grow as we fetch them? We should consider using a
> placeholder "loading" image, reserving space so the thumbnails don't shift.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:130:
this.onLoadManifestFailed_.bind(this));
Good point. I added navigator.onLine to detect the connection state.

On 2012/11/27 22:04:36, flackr wrote:
> If we're offline won't it take a while to fail to download the manifest? We
may
> want to either show the local manifest and then update with the new one when
> it's available or detect the connection state.

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
File chrome/common/extensions/api/wallpaper_private.json (right):

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
chrome/common/extensions/api/wallpaper_private.json:141: "description":
"Wallpaper url."
On 2012/11/27 22:04:36, flackr wrote:
> Just out of convention I think the url "key" should be first. We're setting
the
> wallpaper thumbnail with key "url" to be "data" or thumbnail[key] = url.

Done.

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
chrome/common/extensions/api/wallpaper_private.json:147: "parameters": [],
I believe the convention for extension function is using SetError("error
string") in the function implementation, and use chrome.runtime.lastError to get
the error string in javascript. miket@ could you confirm?

On 2012/11/27 22:04:36, flackr wrote:
> Saving the thumbnail can also fail, you may want to add this parameter to the
> callback so that the test can accurately detect what the failure is.

[flackr, 2012-12-03 19:02:29]

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
(right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:57:
var blob = new Blob([data]);
On 2012/11/28 18:54:26, bshe wrote:
> On 2012/11/27 22:04:36, flackr wrote:
> > Combine these two lines, you only use data here.
> 
> Done.

This doesn't look done.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:62:
self.appendChild(imageEl);
On 2012/11/28 18:54:26, bshe wrote:
> The img tag is added to a div. And the div has width/height equal to the
> thumbnail image. So the thumbnails shouldn't shift as it loading.
> 
> Add a placeholder image sounds great. We just reserve the space currently. Can
> we keep it this way right now though? Since I dont have the resource and it
may
> take a while to get it. Added a todo.
> 
> On 2012/11/27 22:04:36, flackr wrote:
> > The list of thumbnails grow as we fetch them? We should consider using a
> > placeholder "loading" image, reserving space so the thumbnails don't shift.
> 

Yes that sounds good to me.

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
File chrome/common/extensions/api/wallpaper_private.json (right):

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
chrome/common/extensions/api/wallpaper_private.json:147: "parameters": [],
On 2012/11/28 18:54:26, bshe wrote:
> I believe the convention for extension function is using SetError("error
> string") in the function implementation, and use chrome.runtime.lastError to
get
> the error string in javascript. miket@ could you confirm?
> 
> On 2012/11/27 22:04:36, flackr wrote:
> > Saving the thumbnail can also fail, you may want to add this parameter to
the
> > callback so that the test can accurately detect what the failure is.
> 

But this is an asynchronous call, you wouldn't know when to check
chrome.runtime.lastError unless it failed immediately.

[bshe, 2012-12-03 19:39:32]

Done. Thanks for review.

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
(right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:57:
var blob = new Blob([data]);
Sorry. I changed the one at line 38 but somehow forgot to change here.

Now done for real.

On 2012/12/03 19:02:30, flackr wrote:
> On 2012/11/28 18:54:26, bshe wrote:
> > On 2012/11/27 22:04:36, flackr wrote:
> > > Combine these two lines, you only use data here.
> > 
> > Done.
> 
> This doesn't look done.

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
File chrome/common/extensions/api/wallpaper_private.json (right):

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
chrome/common/extensions/api/wallpaper_private.json:147: "parameters": [],
Maybe I understand you wrong. But you should be able to check
chrome.runtime.lastError in the callback function of the api call? When you
request it from the callback, you should be able to get the error string that
you have set in the SetError. And also, the js console displays the string as a
js error. See comment 11 at https://chromiumcodereview.appspot.com/11092078



On 2012/12/03 19:02:30, flackr wrote:
> On 2012/11/28 18:54:26, bshe wrote:
> > I believe the convention for extension function is using SetError("error
> > string") in the function implementation, and use chrome.runtime.lastError to
> get
> > the error string in javascript. miket@ could you confirm?
> > 
> > On 2012/11/27 22:04:36, flackr wrote:
> > > Saving the thumbnail can also fail, you may want to add this parameter to
> the
> > > callback so that the test can accurately detect what the failure is.
> > 
> 
> But this is an asynchronous call, you wouldn't know when to check
> chrome.runtime.lastError unless it failed immediately.

[flackr, 2012-12-03 20:03:45]

LGTM

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:130:
this.onLoadManifestFailed_.bind(this));
On 2012/11/28 18:54:26, bshe wrote:
> Good point. I added navigator.onLine to detect the connection state.
> 
> On 2012/11/27 22:04:36, flackr wrote:
> > If we're offline won't it take a while to fail to download the manifest? We
> may
> > want to either show the local manifest and then update with the new one when
> > it's available or detect the connection state.
> 

I think in the long run loading the offline manifest first and replacing with
the online one when available will give a better user experience (it may be slow
to download) but this can be done later.

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
File chrome/common/extensions/api/wallpaper_private.json (right):

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
chrome/common/extensions/api/wallpaper_private.json:147: "parameters": [],
On 2012/12/03 19:39:32, bshe wrote:
> Maybe I understand you wrong. But you should be able to check
> chrome.runtime.lastError in the callback function of the api call? When you
> request it from the callback, you should be able to get the error string that
> you have set in the SetError. And also, the js console displays the string as
a
> js error. See comment 11 at https://chromiumcodereview.appspot.com/11092078
> 
> 
> 
> On 2012/12/03 19:02:30, flackr wrote:
> > On 2012/11/28 18:54:26, bshe wrote:
> > > I believe the convention for extension function is using SetError("error
> > > string") in the function implementation, and use chrome.runtime.lastError
to
> > get
> > > the error string in javascript. miket@ could you confirm?
> > > 
> > > On 2012/11/27 22:04:36, flackr wrote:
> > > > Saving the thumbnail can also fail, you may want to add this parameter
to
> > the
> > > > callback so that the test can accurately detect what the failure is.
> > > 
> > 
> > But this is an asynchronous call, you wouldn't know when to check
> > chrome.runtime.lastError unless it failed immediately.
> 

Ah, I follow. So in theory the success parameter in getThumbail may be
unnecessary although it seems that SetError is meant for unexpected errors like
being unable to save thumbnails whereas thumbnails would regularly be expected
to not be on the computer.

[bshe, 2012-12-03 22:18:00]

Thanks for review!

+jhawkins for owners (just added a new chrome path for wallpaper thumbnails):

chrome/common/chrome_paths.cc
chrome/common/chrome_paths.h

Thanks!

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://codereview.chromium.org/11348215/diff/19/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:130:
this.onLoadManifestFailed_.bind(this));
Make sense. Added a TODO.

On 2012/12/03 20:03:46, flackr wrote:
> On 2012/11/28 18:54:26, bshe wrote:
> > Good point. I added navigator.onLine to detect the connection state.
> > 
> > On 2012/11/27 22:04:36, flackr wrote:
> > > If we're offline won't it take a while to fail to download the manifest?
We
> > may
> > > want to either show the local manifest and then update with the new one
when
> > > it's available or detect the connection state.
> > 
> 
> I think in the long run loading the offline manifest first and replacing with
> the online one when available will give a better user experience (it may be
slow
> to download) but this can be done later.

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
File chrome/common/extensions/api/wallpaper_private.json (right):

https://codereview.chromium.org/11348215/diff/19/chrome/common/extensions/api...
chrome/common/extensions/api/wallpaper_private.json:147: "parameters": [],
Right. Sorry for confusion. For the case thumbnails regularly be expected to not
be on the computer, I want to avoid to use SetError since it usually output
error message in js console and in chrome log message. I am hoping to avoid to
show these error messages when it happens.

I can probably remove the success parameter and use the existence of data
parameter to infer the success state. If data is undefined, we didn't find the
thumbnail on the computer.

On 2012/12/03 20:03:46, flackr wrote:
> On 2012/12/03 19:39:32, bshe wrote:
> > Maybe I understand you wrong. But you should be able to check
> > chrome.runtime.lastError in the callback function of the api call? When you
> > request it from the callback, you should be able to get the error string
that
> > you have set in the SetError. And also, the js console displays the string
as
> a
> > js error. See comment 11 at https://chromiumcodereview.appspot.com/11092078
> > 
> > 
> > 
> > On 2012/12/03 19:02:30, flackr wrote:
> > > On 2012/11/28 18:54:26, bshe wrote:
> > > > I believe the convention for extension function is using SetError("error
> > > > string") in the function implementation, and use
chrome.runtime.lastError
> to
> > > get
> > > > the error string in javascript. miket@ could you confirm?
> > > > 
> > > > On 2012/11/27 22:04:36, flackr wrote:
> > > > > Saving the thumbnail can also fail, you may want to add this parameter
> to
> > > the
> > > > > callback so that the test can accurately detect what the failure is.
> > > > 
> > > 
> > > But this is an asynchronous call, you wouldn't know when to check
> > > chrome.runtime.lastError unless it failed immediately.
> > 
> 
> Ah, I follow. So in theory the success parameter in getThumbail may be
> unnecessary although it seems that SetError is meant for unexpected errors
like
> being unable to save thumbnails whereas thumbnails would regularly be expected
> to not be on the computer.

[James Hawkins, 2012-12-04 00:38:21]

lgtm

[commit-bot: I haz the power, 2012-12-04 16:23:04]

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

[commit-bot: I haz the power, 2012-12-04 18:50:57]

Change committed as 171011

[Nico, 2012-12-04 19:34:44]

https://chromiumcodereview.appspot.com/11348215/diff/6015/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://chromiumcodereview.appspot.com/11348215/diff/6015/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:93:
(file_util::ReadFileToString(file_path, data) != -1);
ReadFileToString returns a bool, not an int. I sent
https://codereview.chromium.org/11419315/ to fix this.