Hook up custom wallpaper code path to new wallpaper picker.

Hook up custom wallpaper code path to new wallpaper picker.

Add setCustomWallpaper private api function to hook up custom
wallpaper to new Wallpaper picker

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

[bshe, 2012-08-29 17:52:51]

Hi Mihai.

Could you please take a look at this CL? I added a new wallpaper private
extension function to hook up the custom wallpaper code path to the new
wallpaper picker ui.

+ivankr for
chrome/browser/chromeos/login/*
I made SetCustomWallpaper a public function for the extension api implementation
to use.

+flackr for
chrome/browser/resources/chromeos/wallpaper_manager/*

Thanks!

[flackr, 2012-08-29 18:43:45]

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:22:
this.initDom_();
Initialize "loadedFile_".

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:256:
var self = this;
Add TODO to handle onerror.

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:272:
if (this.loadedFile_) {
I think you should guarantee that loadedFile_ is set before calling this
function and perhaps pass it in as a required parameter.

Also, can we change loadedFile to be more descriptive. Perhaps
customWallpaperData_.

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:282:
this.refreshWallpaper_();
i.e. if (this.loadedFile_)
        this.refreshWallpaper_(this.loadedFile_);

[Ivan Korotkov, 2012-08-30 14:07:36]

chromeos LGTM

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_private_api.h (right):

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_private_api.h:35: class
WallpaperDecoder;
Forward declarations go after includes.

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_private_api.h:42: virtual void
OnFail() = 0;
OnFailure?

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:257:
reader.onload = function(e) {
Use addEventListener

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:269:
this.dialogDom_.querySelector('#set-wallpaper-layout');
Could be just $('set-wallpaper-layout');

[bshe, 2012-08-30 19:22:40]

Thanks for review!
+dgozman for $ vs querySelector in a comment.

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_private_api.h (right):

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_private_api.h:35: class
WallpaperDecoder;
Looks like this is a nested class. Not sure if I should forward declare it after
includes.

On 2012/08/30 14:07:36, Ivan Korotkov wrote:
> Forward declarations go after includes.

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_private_api.h:42: virtual void
OnFail() = 0;
On 2012/08/30 14:07:36, Ivan Korotkov wrote:
> OnFailure?

Done.

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:22:
this.initDom_();
On 2012/08/29 18:43:45, flackr wrote:
> Initialize "loadedFile_".

Done.

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:256:
var self = this;
On 2012/08/29 18:43:45, flackr wrote:
> Add TODO to handle onerror.

Done.

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:257:
reader.onload = function(e) {
Looks like some other places use the same .onload/.onloadend pattern.
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/resources/f...
Should I keep it consistent or do you still prefer to use addEventListener?
On 2012/08/30 14:07:36, Ivan Korotkov wrote:
> Use addEventListener

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:269:
this.dialogDom_.querySelector('#set-wallpaper-layout');
I followed file manager js coding style. Not sure why $ is not used in there.
Happy to change it to $('') pattern if wallpaper manager do not apply the
restriction file manager may have. +dgozman

On 2012/08/30 14:07:36, Ivan Korotkov wrote:
> Could be just $('set-wallpaper-layout');

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:272:
if (this.loadedFile_) {
On 2012/08/29 18:43:45, flackr wrote:
> I think you should guarantee that loadedFile_ is set before calling this
> function and perhaps pass it in as a required parameter.
> 
> Also, can we change loadedFile to be more descriptive. Perhaps
> customWallpaperData_.

Done.

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:282:
this.refreshWallpaper_();
On 2012/08/29 18:43:45, flackr wrote:
> i.e. if (this.loadedFile_)
>         this.refreshWallpaper_(this.loadedFile_);

Done.

[flackr, 2012-08-30 21:18:25]

I agree with ivankr's suggestions: using addEventListener and $(''). I don't
think there's any need to be consistent with file manager and these are
preferable.

https://chromiumcodereview.appspot.com/10890038/diff/11002/chrome/browser/res...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://chromiumcodereview.appspot.com/10890038/diff/11002/chrome/browser/res...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:261:

Is it possible that this will be null/undefined after FileReader's onload event
fires?

https://chromiumcodereview.appspot.com/10890038/diff/11002/chrome/browser/res...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:268:

Can you add a comment on what customWallpaper is? i.e. type ArrayBuffer, it's
the raw wallpaper file data.

[dgozman, 2012-08-31 09:20:08]

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:269:
this.dialogDom_.querySelector('#set-wallpaper-layout');
On 2012/08/30 19:22:40, bshe wrote:
> I followed file manager js coding style. Not sure why $ is not used in there.
> Happy to change it to $('') pattern if wallpaper manager do not apply the
> restriction file manager may have. +dgozman
> 
> On 2012/08/30 14:07:36, Ivan Korotkov wrote:
> > Could be just $('set-wallpaper-layout');
> 

Well, we just don't like it. It can be also confused with jQuery, which is not
used.

[Nikita (slow), 2012-08-31 10:33:01]

On 2012/08/31 09:20:08, dgozman wrote:
>
https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
> File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
> (right):
> 
>
https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:269:
> this.dialogDom_.querySelector('#set-wallpaper-layout');
> On 2012/08/30 19:22:40, bshe wrote:
> > I followed file manager js coding style. Not sure why $ is not used in
there.
> > Happy to change it to $('') pattern if wallpaper manager do not apply the
> > restriction file manager may have. +dgozman
> > 
> > On 2012/08/30 14:07:36, Ivan Korotkov wrote:
> > > Could be just $('set-wallpaper-layout');
> > 
> 
> Well, we just don't like it. It can be also confused with jQuery, which is not
> used.

But Chrome JS code uses $('') very extensively so we should follow Chrome WebUI
code style.

[Nikita (slow), 2012-08-31 10:36:49]

On 2012/08/31 10:33:01, Nikita Kostylev wrote:
> On 2012/08/31 09:20:08, dgozman wrote:
> >
>
https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
> > File
> chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
> > (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10890038/diff/2005/chrome/browser/reso...
> >
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:269:
> > this.dialogDom_.querySelector('#set-wallpaper-layout');
> > On 2012/08/30 19:22:40, bshe wrote:
> > > I followed file manager js coding style. Not sure why $ is not used in
> there.
> > > Happy to change it to $('') pattern if wallpaper manager do not apply the
> > > restriction file manager may have. +dgozman
> > > 
> > > On 2012/08/30 14:07:36, Ivan Korotkov wrote:
> > > > Could be just $('set-wallpaper-layout');
> > > 
> > 
> > Well, we just don't like it. It can be also confused with jQuery, which is
not
> > used.
> 
> But Chrome JS code uses $('') very extensively so we should follow Chrome
WebUI
> code style.

See also "$ in the console" part of
https://plus.google.com/u/0/113127438179392830442/posts/Bo1zdF4X9mp

[bshe, 2012-08-31 19:46:57]

Thanks for suggestions and explanation. 

mihaip@ friendly ping?

On 2012/08/30 21:18:25, flackr wrote:
> I agree with ivankr's suggestions: using addEventListener and $(''). I don't
> think there's any need to be consistent with file manager and these are
> preferable.

Done.

> 
>
https://chromiumcodereview.appspot.com/10890038/diff/11002/chrome/browser/res...
> File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
> (right):
> 
>
https://chromiumcodereview.appspot.com/10890038/diff/11002/chrome/browser/res...
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:261:
> 
> Is it possible that this will be null/undefined after FileReader's onload
event
> fires?

Looks like the onload is only called when success. But onloadend is called after
success or failure. So it can't be null. Removed the check.

> 
>
https://chromiumcodereview.appspot.com/10890038/diff/11002/chrome/browser/res...
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:268:
> 
> Can you add a comment on what customWallpaper is? i.e. type ArrayBuffer, it's
> the raw wallpaper file data.

Done.

[bshe, 2012-09-04 17:29:08]

mihaip@ friendly ping in case it is buried in your inbox. Thanks!

On 2012/08/31 19:46:57, bshe wrote:
> Thanks for suggestions and explanation. 
> 
> mihaip@ friendly ping?
> 
> On 2012/08/30 21:18:25, flackr wrote:
> > I agree with ivankr's suggestions: using addEventListener and $(''). I don't
> > think there's any need to be consistent with file manager and these are
> > preferable.
> 
> Done.
> 
> > 
> >
>
https://chromiumcodereview.appspot.com/10890038/diff/11002/chrome/browser/res...
> > File
> chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
> > (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10890038/diff/11002/chrome/browser/res...
> >
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:261:
> > 
> > Is it possible that this will be null/undefined after FileReader's onload
> event
> > fires?
> 
> Looks like the onload is only called when success. But onloadend is called
after
> success or failure. So it can't be null. Removed the check.
> 
> > 
> >
>
https://chromiumcodereview.appspot.com/10890038/diff/11002/chrome/browser/res...
> >
>
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:268:
> > 
> > Can you add a comment on what customWallpaper is? i.e. type ArrayBuffer,
it's
> > the raw wallpaper file data.
> 
> Done.

[bshe, 2012-09-05 15:40:36]

+ nikita for Owners:

chrome/browser/resources/chromeos/wallpaper_manager/*

[Nikita (slow), 2012-09-05 17:43:08]

lgtm

you should add yourself as an owner in
chrome/browser/resources/chromeos/wallpaper_manager/*

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:255:
//TODO(bshe): Handle file error.
nit: space between // and TODO

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:303:
if (show) {
Could simplify:
show > showCustom

$('category-container').hidden = showCustom;
$('custom-container').hidden = !showCustom;

[Mihai Parparita -not on Chrome, 2012-09-05 18:20:13]

LGTM

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:116: bool
WallpaperFunctionBase::RunImpl() {
You can just leave this as unimplemented.

[bshe, 2012-09-05 22:39:46]

thanks!

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:116: bool
WallpaperFunctionBase::RunImpl() {
On 2012/09/05 18:20:14, Mihai Parparita wrote:
> You can just leave this as unimplemented.

Done.

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
(right):

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:255:
//TODO(bshe): Handle file error.
On 2012/09/05 17:43:08, Nikita Kostylev wrote:
> nit: space between // and TODO

Done.

http://codereview.chromium.org/10890038/diff/14013/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:303:
if (show) {
On 2012/09/05 17:43:08, Nikita Kostylev wrote:
> Could simplify:
> show > showCustom
> 
> $('category-container').hidden = showCustom;
> $('custom-container').hidden = !showCustom;

Done.

[flackr, 2012-09-06 12:36:06]

LGTM

[commit-bot: I haz the power, 2012-09-06 14:02:04]

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

[commit-bot: I haz the power, 2012-09-06 16:19:09]

Failed to apply patch for
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:
While running patch -p1 --forward --force;
patching file
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js
Hunk #4 succeeded at 178 (offset 18 lines).
Hunk #5 FAILED at 169.
Hunk #6 succeeded at 201 (offset 18 lines).
Hunk #7 succeeded at 222 (offset 18 lines).
Hunk #8 succeeded at 238 (offset 18 lines).
Hunk #9 succeeded at 264 (offset 18 lines).
1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js.rej