Convert wallpaper picker to v2 app

Convert wallpaper picker to v2 app


TBR=jhawkins
BUG=155090
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170388

[bshe, 2012-10-10 19:14:31]

Hi Mihai.

Could you please take a look at this CL? This should launch wallpaper picker as
a platform app.

The only thing doesn't work is chrome.windows.* apis. It looks like there is
something different when using it in extension verse in platform app. I have
sent email to the discussion mail list. I wont land this CL until I figure out
how to fix the chrome.windows issue. But at the same time, it would be really
nice to get some early feed back from you.

Thanks!

[Mihai Parparita -not on Chrome, 2012-10-12 00:48:47]

https://codereview.chromium.org/11028121/diff/11/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/wallpaper_manager_util.cc (right):

https://codereview.chromium.org/11028121/diff/11/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_manager_util.cc:40:
params.override_url = GURL(url);
This should not be necessary, launching of v2 apps doesn't use URLs (it just
sends an onLaunched event).

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/main.js (right):

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/main.js:13:
window.addEventListener('close',
This won't work either. Once http://crbug.com/130465 (in the next week
hopefully), you should be able to listen for a close event.

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

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:36:
xhr.open('GET', this.dataItem.baseURL + ThumbnailSuffix, true);
What is the base URL in this case? We generally are OK with loading of local
images (they don't have issues when offline).

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:43:
imageEl.src = window.URL.createObjectURL(xhr.response);
This will leak the URL, you'll need to call revokeObjectURL once the window is
closing.

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/manifest.json (right):

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/manifest.json:22:
"persistent": false
This shouldn't be necessary (all app background pages are not persistent).

[bshe, 2012-10-12 16:28:30]

Thanks for review.

I did the following in this patch:
1. address your comments
2. remove chrome://wallpapers url completely since we no longer need it
3. fix an issue associated with register two event handlers for xhr when
requesting manifest(in wallpaper_manager.js file).

https://codereview.chromium.org/11028121/diff/11/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/wallpaper_manager_util.cc (right):

https://codereview.chromium.org/11028121/diff/11/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/wallpaper_manager_util.cc:40:
params.override_url = GURL(url);
On 2012/10/12 00:48:47, Mihai Parparita wrote:
> This should not be necessary, launching of v2 apps doesn't use URLs (it just
> sends an onLaunched event).

Done.

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/js/main.js (right):

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/main.js:13:
window.addEventListener('close',
Thanks for info. I will wait for 130465 and the chrome.windows.* api before I
land this CL. Should I block my issue on 130465 though?

On 2012/10/12 00:48:47, Mihai Parparita wrote:
> This won't work either. Once http://crbug.com/130465 (in the next week
> hopefully), you should be able to listen for a close event.

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

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:36:
xhr.open('GET', this.dataItem.baseURL + ThumbnailSuffix, true);
The base URL is a online url.
https://storage.googleapis.com/chromeos-wallpaper-public/timestamp-file-name.png

We want to push new wallpapers to users as soon as possible. If we include them
as local iamges, users will have to wait for chrome update to get the new
wallpapers.

On 2012/10/12 00:48:47, Mihai Parparita wrote:
> What is the base URL in this case? We generally are OK with loading of local
> images (they don't have issues when offline).

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:43:
imageEl.src = window.URL.createObjectURL(xhr.response);
I saw examples of revoke object after load. After test on my desktop, it seems
work fine. We just need to display the thumbnail and wont do any manipulation on
the URL object after load. So it is probably fine to just revoke them after
load?

On 2012/10/12 00:48:47, Mihai Parparita wrote:
> This will leak the URL, you'll need to call revokeObjectURL once the window is
> closing.

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/wallpaper_manager/manifest.json (right):

https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/wallpaper_manager/manifest.json:22:
"persistent": false
On 2012/10/12 00:48:47, Mihai Parparita wrote:
> This shouldn't be necessary (all app background pages are not persistent).

Done.

[Mihai Parparita -not on Chrome, 2012-10-19 00:38:05]

It sounds like this is blocked on a few other things. Let me know when I
should look at it again.

Mihai

On Fri, Oct 12, 2012 at 9:28 AM, <bshe@chromium.org> wrote:

> Thanks for review.
>
> I did the following in this patch:
> 1. address your comments
> 2. remove chrome://wallpapers url completely since we no longer need it
> 3. fix an issue associated with register two event handlers for xhr when
> requesting manifest(in wallpaper_manager.js file).
>
>
>
>
>
> https://codereview.chromium.**org/11028121/diff/11/chrome/**
>
browser/chromeos/extensions/**wallpaper_manager_util.cc<https://codereview.chromium.org/11028121/diff/11/chrome/browser/chromeos/extensions/wallpaper_manager_util.cc>
> File chrome/browser/chromeos/**extensions/wallpaper_manager_**util.cc
> (right):
>
> https://codereview.chromium.**org/11028121/diff/11/chrome/**
>
browser/chromeos/extensions/**wallpaper_manager_util.cc#**newcode40<https://codereview.chromium.org/11028121/diff/11/chrome/browser/chromeos/extensions/wallpaper_manager_util.cc#newcode40>
> chrome/browser/chromeos/**extensions/wallpaper_manager_**util.cc:40:
> params.override_url = GURL(url);
> On 2012/10/12 00:48:47, Mihai Parparita wrote:
>
>> This should not be necessary, launching of v2 apps doesn't use URLs
>>
> (it just
>
>> sends an onLaunched event).
>>
>
> Done.
>
>
> https://codereview.chromium.**org/11028121/diff/11/chrome/**
>
browser/resources/chromeos/**wallpaper_manager/js/main.js<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/main.js>
> File chrome/browser/resources/**chromeos/wallpaper_manager/js/**main.js
> (right):
>
> https://codereview.chromium.**org/11028121/diff/11/chrome/**
>
browser/resources/chromeos/**wallpaper_manager/js/main.js#**newcode13<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/main.js#newcode13>
> chrome/browser/resources/**chromeos/wallpaper_manager/js/**main.js:13:
> window.addEventListener('**close',
> Thanks for info. I will wait for 130465 and the chrome.windows.* api
> before I land this CL. Should I block my issue on 130465 though?
>
>
> On 2012/10/12 00:48:47, Mihai Parparita wrote:
>
>> This won't work either. Once http://crbug.com/130465 (in the next week
>> hopefully), you should be able to listen for a close event.
>>
>
> https://codereview.chromium.**org/11028121/diff/11/chrome/**
> browser/resources/chromeos/**wallpaper_manager/js/**
>
wallpaper_images_grid.js<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js>
> File
> chrome/browser/resources/**chromeos/wallpaper_manager/js/**
> wallpaper_images_grid.js
> (right):
>
> https://codereview.chromium.**org/11028121/diff/11/chrome/**
> browser/resources/chromeos/**wallpaper_manager/js/**
>
wallpaper_images_grid.js#**newcode36<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js#newcode36>
> chrome/browser/resources/**chromeos/wallpaper_manager/js/**
> wallpaper_images_grid.js:36:
> xhr.open('GET', this.dataItem.baseURL + ThumbnailSuffix, true);
> The base URL is a online url.
> https://storage.googleapis.**com/chromeos-wallpaper-public/**
>
timestamp-file-name.png<https://storage.googleapis.com/chromeos-wallpaper-public/timestamp-file-name.png>
>
> We want to push new wallpapers to users as soon as possible. If we
> include them as local iamges, users will have to wait for chrome update
> to get the new wallpapers.
>
>
> On 2012/10/12 00:48:47, Mihai Parparita wrote:
>
>> What is the base URL in this case? We generally are OK with loading of
>>
> local
>
>> images (they don't have issues when offline).
>>
>
> https://codereview.chromium.**org/11028121/diff/11/chrome/**
> browser/resources/chromeos/**wallpaper_manager/js/**
>
wallpaper_images_grid.js#**newcode43<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js#newcode43>
> chrome/browser/resources/**chromeos/wallpaper_manager/js/**
> wallpaper_images_grid.js:43:
> imageEl.src = window.URL.createObjectURL(**xhr.response);
> I saw examples of revoke object after load. After test on my desktop, it
> seems work fine. We just need to display the thumbnail and wont do any
> manipulation on the URL object after load. So it is probably fine to
> just revoke them after load?
>
>
> On 2012/10/12 00:48:47, Mihai Parparita wrote:
>
>> This will leak the URL, you'll need to call revokeObjectURL once the
>>
> window is
>
>> closing.
>>
>
> https://codereview.chromium.**org/11028121/diff/11/chrome/**
>
browser/resources/chromeos/**wallpaper_manager/manifest.**json<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/manifest.json>
> File chrome/browser/resources/**chromeos/wallpaper_manager/**manifest.json
> (right):
>
> https://codereview.chromium.**org/11028121/diff/11/chrome/**
>
browser/resources/chromeos/**wallpaper_manager/manifest.**json#newcode22<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/manifest.json#newcode22>
> chrome/browser/resources/**chromeos/wallpaper_manager/**manifest.json:22:
> "persistent": false
> On 2012/10/12 00:48:47, Mihai Parparita wrote:
>
>> This shouldn't be necessary (all app background pages are not
>>
> persistent).
>
> Done.
>
>
https://codereview.chromium.**org/11028121/<https://codereview.chromium.org/1...
>

[bshe, 2012-10-19 02:50:33]

It was blocked on two things. And one of them already fixed (the close event)
and the other may take some time. Probably will be m25. So this CL may block for
some time. I will definitely let you know once it is ready. Thanks!

On 2012/10/19 00:38:05, Mihai Parparita wrote:
> It sounds like this is blocked on a few other things. Let me know when I
> should look at it again.
> 
> Mihai
> 
> On Fri, Oct 12, 2012 at 9:28 AM, <mailto:bshe@chromium.org> wrote:
> 
> > Thanks for review.
> >
> > I did the following in this patch:
> > 1. address your comments
> > 2. remove chrome://wallpapers url completely since we no longer need it
> > 3. fix an issue associated with register two event handlers for xhr when
> > requesting manifest(in wallpaper_manager.js file).
> >
> >
> >
> >
> >
> > https://codereview.chromium.**org/11028121/diff/11/chrome/**
> >
>
browser/chromeos/extensions/**wallpaper_manager_util.cc<https://codereview.chromium.org/11028121/diff/11/chrome/browser/chromeos/extensions/wallpaper_manager_util.cc>
> > File chrome/browser/chromeos/**extensions/wallpaper_manager_**util.cc
> > (right):
> >
> > https://codereview.chromium.**org/11028121/diff/11/chrome/**
> >
>
browser/chromeos/extensions/**wallpaper_manager_util.cc#**newcode40<https://codereview.chromium.org/11028121/diff/11/chrome/browser/chromeos/extensions/wallpaper_manager_util.cc#newcode40>
> > chrome/browser/chromeos/**extensions/wallpaper_manager_**util.cc:40:
> > params.override_url = GURL(url);
> > On 2012/10/12 00:48:47, Mihai Parparita wrote:
> >
> >> This should not be necessary, launching of v2 apps doesn't use URLs
> >>
> > (it just
> >
> >> sends an onLaunched event).
> >>
> >
> > Done.
> >
> >
> > https://codereview.chromium.**org/11028121/diff/11/chrome/**
> >
>
browser/resources/chromeos/**wallpaper_manager/js/main.js<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/main.js>
> > File chrome/browser/resources/**chromeos/wallpaper_manager/js/**main.js
> > (right):
> >
> > https://codereview.chromium.**org/11028121/diff/11/chrome/**
> >
>
browser/resources/chromeos/**wallpaper_manager/js/main.js#**newcode13<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/main.js#newcode13>
> > chrome/browser/resources/**chromeos/wallpaper_manager/js/**main.js:13:
> > window.addEventListener('**close',
> > Thanks for info. I will wait for 130465 and the chrome.windows.* api
> > before I land this CL. Should I block my issue on 130465 though?
> >
> >
> > On 2012/10/12 00:48:47, Mihai Parparita wrote:
> >
> >> This won't work either. Once http://crbug.com/130465 (in the next week
> >> hopefully), you should be able to listen for a close event.
> >>
> >
> > https://codereview.chromium.**org/11028121/diff/11/chrome/**
> > browser/resources/chromeos/**wallpaper_manager/js/**
> >
>
wallpaper_images_grid.js<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js>
> > File
> > chrome/browser/resources/**chromeos/wallpaper_manager/js/**
> > wallpaper_images_grid.js
> > (right):
> >
> > https://codereview.chromium.**org/11028121/diff/11/chrome/**
> > browser/resources/chromeos/**wallpaper_manager/js/**
> >
>
wallpaper_images_grid.js#**newcode36<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js#newcode36>
> > chrome/browser/resources/**chromeos/wallpaper_manager/js/**
> > wallpaper_images_grid.js:36:
> > xhr.open('GET', this.dataItem.baseURL + ThumbnailSuffix, true);
> > The base URL is a online url.
> > https://storage.googleapis.**com/chromeos-wallpaper-public/**
> >
>
timestamp-file-name.png<https://storage.googleapis.com/chromeos-wallpaper-public/timestamp-file-name.png>
> >
> > We want to push new wallpapers to users as soon as possible. If we
> > include them as local iamges, users will have to wait for chrome update
> > to get the new wallpapers.
> >
> >
> > On 2012/10/12 00:48:47, Mihai Parparita wrote:
> >
> >> What is the base URL in this case? We generally are OK with loading of
> >>
> > local
> >
> >> images (they don't have issues when offline).
> >>
> >
> > https://codereview.chromium.**org/11028121/diff/11/chrome/**
> > browser/resources/chromeos/**wallpaper_manager/js/**
> >
>
wallpaper_images_grid.js#**newcode43<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js#newcode43>
> > chrome/browser/resources/**chromeos/wallpaper_manager/js/**
> > wallpaper_images_grid.js:43:
> > imageEl.src = window.URL.createObjectURL(**xhr.response);
> > I saw examples of revoke object after load. After test on my desktop, it
> > seems work fine. We just need to display the thumbnail and wont do any
> > manipulation on the URL object after load. So it is probably fine to
> > just revoke them after load?
> >
> >
> > On 2012/10/12 00:48:47, Mihai Parparita wrote:
> >
> >> This will leak the URL, you'll need to call revokeObjectURL once the
> >>
> > window is
> >
> >> closing.
> >>
> >
> > https://codereview.chromium.**org/11028121/diff/11/chrome/**
> >
>
browser/resources/chromeos/**wallpaper_manager/manifest.**json<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/manifest.json>
> > File chrome/browser/resources/**chromeos/wallpaper_manager/**manifest.json
> > (right):
> >
> > https://codereview.chromium.**org/11028121/diff/11/chrome/**
> >
>
browser/resources/chromeos/**wallpaper_manager/manifest.**json#newcode22<https://codereview.chromium.org/11028121/diff/11/chrome/browser/resources/chromeos/wallpaper_manager/manifest.json#newcode22>
> > chrome/browser/resources/**chromeos/wallpaper_manager/**manifest.json:22:
> > "persistent": false
> > On 2012/10/12 00:48:47, Mihai Parparita wrote:
> >
> >> This shouldn't be necessary (all app background pages are not
> >>
> > persistent).
> >
> > Done.
> >
> >
>
https://codereview.chromium.**org/11028121/%3Chttps://codereview.chromium.org...>
> >

[bshe, 2012-11-12 20:55:21]

This cl was blocked on two issues. Now the two issues has been fixed on TOT. I
have verified it works on TOT.

could you please take a look? Thanks!

flackr@ for resources/chromeos/wallpaper_manager

[miket_OOO, 2012-11-12 21:39:46]

LGTM. Exciting that we're getting close!

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
(right):

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:49:
//TODO(bshe): Add error handleing.
Nit: "handling"

Do you really think this TODO will ever be resolved if it's checked in like
this? If yes, proceed.

If no, then please either handle the error, or just remove the TODO.

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

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:92:
// Other components may want to use it (i.e. screen saver).
e.g., not i.e.

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:107:
self.butterBar_.showError_(str('connectionFailed'));
What does the cast do here?

[flackr, 2012-11-12 22:00:34]

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_manager_util.cc (right):

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_manager_util.cc:35:
NEW_FOREGROUND_TAB);
We seem to ignore these parameters for v2 apps anyways, but isn't the mode you
want technically NEW_WINDOW?

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js
(right):

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:12:
wallpaperPickerWindow.focus();
This may be a legitimate case for minimizing the other windows again.

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

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:107:
self.butterBar_.showError_(str('connectionFailed'));
On 2012/11/12 21:39:46, miket wrote:
> What does the cast do here?

str is a function which returns the localized string defined on line 51, not a
cast.

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:115:
return;
Is there a reason for unrolling the for loop into a nested block with very
similar logic?

https://codereview.chromium.org/11028121/diff/16014/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/11028121/diff/16014/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:337: "extension_types":
["extension", "packaged_app", "platform_app"],
Do we need "extension" and "packaged_app" anymore?

[bshe, 2012-11-12 22:24:28]

miket@ thanks for fast review. your comments are address in patchset 8

flackr@ in progress to address yours

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
File
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js
(right):

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_images_grid.js:49:
//TODO(bshe): Add error handleing.
I guess I am not sure how should we proceed when there is an error. Should we
just display a stub image or should we just skip the thumbnail or should we
display some kind of error message? I am more lean towards skip the
thumbnail(remove the TODO) for now. Later if we found this is not what we want,
I will fix it then.


On 2012/11/12 21:39:46, miket wrote:
> Nit: "handling"
> 
> Do you really think this TODO will ever be resolved if it's checked in like
> this? If yes, proceed.
> 
> If no, then please either handle the error, or just remove the TODO.

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

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:92:
// Other components may want to use it (i.e. screen saver).
On 2012/11/12 21:39:46, miket wrote:
> e.g., not i.e.

Done.

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:107:
self.butterBar_.showError_(str('connectionFailed'));
you mean str('..')? It gets the localized string for us.

On 2012/11/12 21:39:46, miket wrote:
> What does the cast do here?

[miket_OOO, 2012-11-12 22:27:06]

> you mean str('..')? It gets the localized string for us.

Yes, sorry, I was out to lunch. I didn't recognize it.

On the TODO, either resolution's OK -- as long as the current code (without the
TODO) is sufficient. Sounds like it is.

[bshe, 2012-11-12 23:53:37]

flackr@ PTAL the latest patchset. Thanks!

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_manager_util.cc (right):

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_manager_util.cc:35:
NEW_FOREGROUND_TAB);
Right. The parameter seems ignored for v2 app. I dont remember why I use
FOREGROUDN_TAB when launch as an extension. But NEW_WINDOW seems more
descriptive. Change it to NEW_WINDOW.

On 2012/11/12 22:00:34, flackr wrote:
> We seem to ignore these parameters for v2 apps anyways, but isn't the mode you
> want technically NEW_WINDOW?

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js
(right):

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:12:
wallpaperPickerWindow.focus();
I remember we had a double minimizing issue when we try to minimizing other
windows on focus/relaunch.
But this seems to be fine with the new private api now. So adding it back again.

On 2012/11/12 22:00:34, flackr wrote:
> This may be a legitimate case for minimizing the other windows again.

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

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:115:
return;
We now use async xmlhttp request. I wrote a sync loop function for it. But it
looks like less readable. 

On 2012/11/12 22:00:34, flackr wrote:
> Is there a reason for unrolling the for loop into a nested block with very
> similar logic?

https://codereview.chromium.org/11028121/diff/16014/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/11028121/diff/16014/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:337: "extension_types":
["extension", "packaged_app", "platform_app"],
On 2012/11/12 22:00:34, flackr wrote:
> Do we need "extension" and "packaged_app" anymore?

Done.

[bshe, 2012-11-12 23:57:39]

+dgozman for OWNERS
component_extension_resources.grd

[flackr, 2012-11-13 02:19:06]

Looking good, just a couple comments.

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js
(right):

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:12:
wallpaperPickerWindow.focus();
On 2012/11/12 23:53:38, bshe wrote:
> I remember we had a double minimizing issue when we try to minimizing other
> windows on focus/relaunch.
> But this seems to be fine with the new private api now. So adding it back
again.
> 
> On 2012/11/12 22:00:34, flackr wrote:
> > This may be a legitimate case for minimizing the other windows again.
> 

We may want to change chrome.wallpaperPrivate.minimizeInactiveWindows to add to
the list of minimized windows rather than replace it to accomodate this.

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

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:115:
return;
On 2012/11/12 23:53:38, bshe wrote:
> We now use async xmlhttp request. I wrote a sync loop function for it. But it
> looks like less readable. 
> 
> On 2012/11/12 22:00:34, flackr wrote:
> > Is there a reason for unrolling the for loop into a nested block with very
> > similar logic?
> 

Ah of course, this can still be done asynchronously with a list of fallback
URL's to avoid repeating the code.

[dgozman, 2012-11-13 10:07:07]

component_extension_resources.grd LGTM

[bshe, 2012-11-13 15:24:45]

thanks!

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js
(right):

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:12:
wallpaperPickerWindow.focus();
Done. PTAL at the latest patchset.

On 2012/11/13 02:19:07, flackr wrote:
> On 2012/11/12 23:53:38, bshe wrote:
> > I remember we had a double minimizing issue when we try to minimizing other
> > windows on focus/relaunch.
> > But this seems to be fine with the new private api now. So adding it back
> again.
> > 
> > On 2012/11/12 22:00:34, flackr wrote:
> > > This may be a legitimate case for minimizing the other windows again.
> > 
> 
> We may want to change chrome.wallpaperPrivate.minimizeInactiveWindows to add
to
> the list of minimized windows rather than replace it to accomodate this.

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

https://codereview.chromium.org/11028121/diff/16014/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:115:
return;
Agree. I will keep it this way.

On 2012/11/13 02:19:07, flackr wrote:
> On 2012/11/12 23:53:38, bshe wrote:
> > We now use async xmlhttp request. I wrote a sync loop function for it. But
it
> > looks like less readable. 
> > 
> > On 2012/11/12 22:00:34, flackr wrote:
> > > Is there a reason for unrolling the for loop into a nested block with very
> > > similar logic?
> > 
> 
> Ah of course, this can still be done asynchronously with a list of fallback
> URL's to avoid repeating the code.

[flackr, 2012-11-13 16:18:29]

Converting to a v2 app is pretty much done here, if you want to file a bug and
change wallpaperPrivate's MinimizeWindows behavior separately to land this
sooner that sounds good to me too.

https://codereview.chromium.org/11028121/diff/24003/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://codereview.chromium.org/11028121/diff/24003/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:56: delete
g_window_state_manager;
Instead of deleting the previous window state manager, can we not just change
BuildWindowListAndMinimizeInactive to append to whatever list it has (which will
be empty the first time we call it and will have the previous list on subsequent
invocations)? We can change the name to MinimizeInactiveWindows to avoid the
confusion about "building" the list being used for appending.

https://codereview.chromium.org/11028121/diff/24003/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:61:
g_window_state_manager->AppendWindows(previous_minimized_windows);
This new behavior should be tested in wallpaper_private_unittest.cc.

https://codereview.chromium.org/11028121/diff/24003/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:119: // The topmost
window is at the front of the list.
If you minimize all the windows and then minimize again, the newer minimized
windows should be topmost, I think we should reverse the order when building the
list such that the topmost window is always at the end of the vector.

Can you update wallpaper_private_api_unittest.cc, add a test to check the
appropriate window is focused / on top after restoring?

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

https://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:91:
var asyncFetchManifestFromUrls = function(urls, func, callback) {
To make this a bit easier to read, perhaps you could have a successCallback
taking the response data and failureCallback showing an error. These can be
moved out to functions in the WallpaperManager.prototype.

https://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:104:
callback();
It looks like if there are no manifests we will no longer show the connection
failed message, is this by design?

https://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:108:
}
Need semicolon at end of line.

https://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:121:
loop.done();
As the code is currently, you'll call initDom twice, once by calling
loop.done().

https://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:131:
}
Semicolon at end of line.

[bshe, 2012-11-13 19:33:21]

flackr@ pre discussion offline
I will change the window state manager api in a follow up cl.
PTAL. Thanks

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

http://codereview.chromium.org/11028121/diff/24003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:56: delete
g_window_state_manager;
Reverted the change and will address them in a follow up CL.

On 2012/11/13 16:18:30, flackr wrote:
> Instead of deleting the previous window state manager, can we not just change
> BuildWindowListAndMinimizeInactive to append to whatever list it has (which
will
> be empty the first time we call it and will have the previous list on
subsequent
> invocations)? We can change the name to MinimizeInactiveWindows to avoid the
> confusion about "building" the list being used for appending.

http://codereview.chromium.org/11028121/diff/24003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:61:
g_window_state_manager->AppendWindows(previous_minimized_windows);
Reverted the change and will address them in a follow up CL.

On 2012/11/13 16:18:30, flackr wrote:
> This new behavior should be tested in wallpaper_private_unittest.cc.

http://codereview.chromium.org/11028121/diff/24003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:119: // The topmost
window is at the front of the list.
Reverted the change and will address them in a follow up CL.

On 2012/11/13 16:18:30, flackr wrote:
> If you minimize all the windows and then minimize again, the newer minimized
> windows should be topmost, I think we should reverse the order when building
the
> list such that the topmost window is always at the end of the vector.
> 
> Can you update wallpaper_private_api_unittest.cc, add a test to check the
> appropriate window is focused / on top after restoring?

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

http://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:91:
var asyncFetchManifestFromUrls = function(urls, func, callback) {
On 2012/11/13 16:18:30, flackr wrote:
> To make this a bit easier to read, perhaps you could have a successCallback
> taking the response data and failureCallback showing an error. These can be
> moved out to functions in the WallpaperManager.prototype.

Done.

http://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:104:
callback();
No. That was a mistake. Fixed it in new patch.

On 2012/11/13 16:18:30, flackr wrote:
> It looks like if there are no manifests we will no longer show the connection
> failed message, is this by design?

http://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:108:
}
On 2012/11/13 16:18:30, flackr wrote:
> Need semicolon at end of line.

Done.

http://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:121:
loop.done();
Should be fixed in the new patch.

On 2012/11/13 16:18:30, flackr wrote:
> As the code is currently, you'll call initDom twice, once by calling
> loop.done().

http://codereview.chromium.org/11028121/diff/24003/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:131:
}
On 2012/11/13 16:18:30, flackr wrote:
> Semicolon at end of line.

Done.

[flackr, 2012-11-13 22:22:31]

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

http://codereview.chromium.org/11028121/diff/20006/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:105:
xhr.addEventListener('load', function(e) {
Reading the XHR documentation it seems like 'load' is only on successful load.
Should you perhaps handle loadend or ready state change and state == this.DONE
instead? See http://www.w3.org/TR/XMLHttpRequest/#introduction. I think this
will only catch successful connections to the server that didn't return a 200
status code.

http://codereview.chromium.org/11028121/diff/20006/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:108:
if (xhr.status === 200) {
nit: braces not needed anymore on this if since they are both single line
blocks.

http://codereview.chromium.org/11028121/diff/20006/chrome/browser/resources/c...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:133:
this.butterBar_.showError_('Failed to parse manifest.');
This isn't a localized string, and the message is somewhat confusing. You could
fall back to the generic connection failed message since the only way you could
get into this state is if you loaded bad data which probably means your
connection is bad. Probably simplest / best to move the JSON parsing back to
fetchManifestAsync and have a JSON parsing error call onLoadManifestFailed.

[bshe, 2012-11-15 00:17:57]

flackr@

Hi Rob. While this is blocked on the double onLaunch event issue, the js code
comments you have are addressed in the latest patch. Could you please take a
look?

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

https://codereview.chromium.org/11028121/diff/20006/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:105:
xhr.addEventListener('load', function(e) {
Done. I used loadend event.

On 2012/11/13 22:22:31, flackr wrote:
> Reading the XHR documentation it seems like 'load' is only on successful load.
> Should you perhaps handle loadend or ready state change and state == this.DONE
> instead? See http://www.w3.org/TR/XMLHttpRequest/#introduction. I think this
> will only catch successful connections to the server that didn't return a 200
> status code.

https://codereview.chromium.org/11028121/diff/20006/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:108:
if (xhr.status === 200) {
On 2012/11/13 22:22:31, flackr wrote:
> nit: braces not needed anymore on this if since they are both single line
> blocks.

Done.

https://codereview.chromium.org/11028121/diff/20006/chrome/browser/resources/...
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js:133:
this.butterBar_.showError_('Failed to parse manifest.');
Done.

On 2012/11/13 22:22:31, flackr wrote:
> This isn't a localized string, and the message is somewhat confusing. You
could
> fall back to the generic connection failed message since the only way you
could
> get into this state is if you loaded bad data which probably means your
> connection is bad. Probably simplest / best to move the JSON parsing back to
> fetchManifestAsync and have a JSON parsing error call onLoadManifestFailed.

[flackr, 2012-11-15 15:53:52]

lgtm

[bshe, 2012-11-29 17:12:39]

+jhawkins for owners approval:

chrome/browser/browser_about_handler.cc
chrome/common/url_constants.h
chrome/common/url_constants.cc

[bshe, 2012-11-30 03:41:09]

tbring jhawkins for the trivial changes in the following three files (just
remove some unused variables)
chrome/browser/browser_about_handler.cc
chrome/common/url_constants.h
chrome/common/url_constants.cc
I will address your comment in a new CL if any. Thanks!

On 2012/11/29 17:12:39, bshe wrote:
> +jhawkins for owners approval:
> 
> chrome/browser/browser_about_handler.cc
> chrome/common/url_constants.h
> chrome/common/url_constants.cc

[James Hawkins, 2012-11-30 04:04:52]

lgtm