Give Chrome Web Store app an icon in its manifest file.

Give Chrome Web Store app an icon in its manifest file.

This fixes no icon being shown in the "The app is currently unreachable" page for the chrome web store.
Also added is another exception to ResourceRequestPolicy::CanRequestResource to make sure the error
page is always allowed to display the app's icon, even in a manifest v2 app that doesn't list its
icons in web_accessible_resources.

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

[Marijn Kruisselbrink, 2012-09-25 20:15:06]

I'm not sure if this is the best way to go about installing the webstore icons
in the right spot, but it works at least...
Also I haven't removed the special casing of the webstore in
ExtensionIconSource::LoadDefaultImage, while it should now be safe to do so I
think.

[Mihai Parparita -not on Chrome, 2012-09-25 20:23:19]

Seems like we should copy the 200% version too (as the 256x256 icon) so
that it won't appear pixelated in HiDPI cases.

Mihai

On Tue, Sep 25, 2012 at 1:15 PM, <mek@chromium.org> wrote:

> Reviewers: Aaron Boodman, sky,
>
> Message:
> I'm not sure if this is the best way to go about installing the webstore
> icons
> in the right spot, but it works at least...
> Also I haven't removed the special casing of the webstore in
> ExtensionIconSource::**LoadDefaultImage, while it should now be safe to
> do so I
> think.
>
> Description:
> Give Chrome Web Store app an icon in its manifest file.
>
> This fixes no icon being shown in the "The app is currently unreachable"
> page
> for the chrome web store.
> Also added is another exception to
ResourceRequestPolicy::**CanRequestResource
> to
> make sure the error
> page is always allowed to display the app's icon, even in a manifest v2
> app that
> doesn't list its
> icons in web_accessible_resources.
>
> BUG=135549
>
>
> Please review this at
http://codereview.chromium.**org/10985028/<http://codereview.chromium.org/109...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/browser/resources/**webstore_app/manifest.json
>   M chrome/chrome_resources.gyp
>   M chrome/renderer/extensions/**resource_request_policy.cc
>
>
> Index: chrome/browser/resources/**webstore_app/manifest.json
> diff --git a/chrome/browser/resources/**webstore_app/manifest.json
> b/chrome/browser/resources/**webstore_app/manifest.json
> index a08a6ea5ac7d88dbd505bc8b184563**e90b70d18d..**
> 56ba0b34e55cda5b15ea15e6b39fcb**72cac30487 100644
> --- a/chrome/browser/resources/**webstore_app/manifest.json
> +++ b/chrome/browser/resources/**webstore_app/manifest.json
> @@ -4,6 +4,8 @@
>    "version": "0.1",
>    "description": "Web Store",
>    "icons": {
> +    "16": "webstore_icon_16.png",
> +    "128": "webstore_icon.png"
>    },
>    "app": {
>      "launch": {
> Index: chrome/chrome_resources.gyp
> diff --git a/chrome/chrome_resources.gyp b/chrome/chrome_resources.gyp
> index 8018cebcdca6b6d5a4f857e15dad07**95a7ee42d2..**
> 14fcef7a1f5c11a8352a4232b6a969**81a59f53d8 100644
> --- a/chrome/chrome_resources.gyp
> +++ b/chrome/chrome_resources.gyp
> @@ -76,6 +76,22 @@
>              'browser/resources/extension_**resource/demo/library.js',
>            ],
>          },
> +        {
> +          'destination': '<(PRODUCT_DIR)/resources/web_**store',
> +          'conditions': [
> +            ['branding=="Chrome"', {
> +              'files': [
> +                'app/theme/default_100_**percent/google_chrome/**
> webstore_icon.png',
> +                'app/theme/default_100_**percent/google_chrome/**
> webstore_icon_16.png',
> +              ],
> +            }, { # else: 'branding!="Chrome"
> +              'files': [
> +                'app/theme/default_100_**percent/chromium/webstore_**
> icon.png',
> +                'app/theme/default_100_**percent/chromium/webstore_**
> icon_16.png',
> +              ],
> +            }],
> +          ],
> +        },
>        ]
>      },
>      {
> Index: chrome/renderer/extensions/**resource_request_policy.cc
> diff --git a/chrome/renderer/extensions/**resource_request_policy.cc
> b/chrome/renderer/extensions/**resource_request_policy.cc
> index c9973c8a5d075f6e235407b6010c84**54e147883a..**
> da83a32643ee9ef5b7be9ffa5f9e56**910a383878 100644
> --- a/chrome/renderer/extensions/**resource_request_policy.cc
> +++ b/chrome/renderer/extensions/**resource_request_policy.cc
> @@ -70,9 +70,12 @@ bool ResourceRequestPolicy::**CanRequestResource(
>          !extension->devtools_url().is_**empty();
>      bool transition_allowed =
>          !content::**PageTransitionIsWebTriggerable**(transition_type);
> +    // - unreachable web page error page (to allow showing the icon of the
> +    //   unreachable app on this page)
> +    bool is_error_page = frame_url == GURL(content::**
> kUnreachableWebDataURL);
>
>      if (!is_empty_origin && !is_own_resource &&
> -        !is_dev_tools && !transition_allowed) {
> +        !is_dev_tools && !transition_allowed && !is_error_page) {
>        std::string message = base::StringPrintf(
>            "Denying load of %s. Resources must be listed in the "
>            "web_accessible_resources manifest key in order to be loaded by
> "
>
>
>

[Aaron Boodman, 2012-09-25 21:44:12]

Seems reasonable to me, but can you do the same for
extension_misc::kChromeAppId, then remove all the special-case crap for this
from image_loading_tracker.cc?

[Marijn Kruisselbrink, 2012-09-27 20:47:49]

On 2012/09/25 21:44:12, Aaron Boodman wrote:
> Seems reasonable to me, but can you do the same for
> extension_misc::kChromeAppId, then remove all the special-case crap for this
> from image_loading_tracker.cc?
Do you happen to know where that special case crap is used/how I can test if
what I've done works?

Also what would be preferred; having all these icons be installed as separate
files, or extend ImageLoadingTracker::IsComponentExtensionResource to be able to
map web_store and chrome_app icons to resource id's in addition to what it
currently already does?

[Marijn Kruisselbrink, 2012-09-27 21:14:42]

On 2012/09/27 20:47:49, Marijn Kruisselbrink wrote:
> On 2012/09/25 21:44:12, Aaron Boodman wrote:
> > Seems reasonable to me, but can you do the same for
> > extension_misc::kChromeAppId, then remove all the special-case crap for this
> > from image_loading_tracker.cc?
> Do you happen to know where that special case crap is used/how I can test if
> what I've done works?
Actually, I think I've found this part.

> Also what would be preferred; having all these icons be installed as separate
> files, or extend ImageLoadingTracker::IsComponentExtensionResource to be able
to
> map web_store and chrome_app icons to resource id's in addition to what it
> currently already does?
And this mapping is actually used in two locations, in ImageLoadingTracker and
in ExtensionProtocolHandler::MaybeCreateJob, so I'd have to add to both places
(or factor out the logic to map from extension resource path to resource id out
to some other place).

Also the problem with using the existing resources is that for the webstore
icons IDR_WEBSTORE_ICON_16 (and all the others too) might actually be double
that size if the 200_percent theme bits are loaded. Not sure if that is actually
a problem?

[Marijn Kruisselbrink, 2012-09-27 22:14:23]

Sorry for keeping replying to myself :)
I've uploaded a new patch set, that should do most of these things.

On 2012/09/27 21:14:42, Marijn Kruisselbrink wrote:
> On 2012/09/27 20:47:49, Marijn Kruisselbrink wrote:
> > Also what would be preferred; having all these icons be installed as
separate
> > files, or extend ImageLoadingTracker::IsComponentExtensionResource to be
able
> to
> > map web_store and chrome_app icons to resource id's in addition to what it
> > currently already does?
> And this mapping is actually used in two locations, in ImageLoadingTracker and
> in ExtensionProtocolHandler::MaybeCreateJob, so I'd have to add to both places
> (or factor out the logic to map from extension resource path to resource id
out
> to some other place).
I've now moved this shared code to a new method in extension_file_util. I'm not
sure if that is the right place for it?

Also at the moment I hardcoded the paths/resource ids for the webstore and
chrome_app icons there; I'm not sure if there is an easy way around that, since
these icons don't have paths under chrome/browser/resources, so can't be in the
regular kComponentExtensionResources.

[Aaron Boodman, 2012-09-28 22:38:51]

http://codereview.chromium.org/10985028/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_protocols.cc (right):

http://codereview.chromium.org/10985028/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_protocols.cc:381: if
(extension_file_util::IsComponentExtensionResource(extension,
Is it possible to just put the icons here:

http://git.chromium.org/gitweb/?p=chromium.git;a=tree;f=chrome/browser/resour...

... and register them in the manifest as normal?

The extension system is HiDPI aware now (at least this part of it), so it should
properly handle getting the right one.

http://codereview.chromium.org/10985028/diff/5001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_file_util.h (right):

http://codereview.chromium.org/10985028/diff/5001/chrome/common/extensions/ex...
chrome/common/extensions/extension_file_util.h:135: int& resource_id);
Use pointers for output params in Chrome. (Technically we're supposed to prefer
const ref for input params too, but Extension* is so widely used in Chrome it's
not worth diverging from)

[Marijn Kruisselbrink, 2012-09-29 01:49:30]

On 2012/09/28 22:38:51, Aaron Boodman wrote:
>
http://codereview.chromium.org/10985028/diff/5001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_protocols.cc (right):
> 
>
http://codereview.chromium.org/10985028/diff/5001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_protocols.cc:381: if
> (extension_file_util::IsComponentExtensionResource(extension,
> Is it possible to just put the icons here:
> 
>
http://git.chromium.org/gitweb/?p=chromium.git;a=tree;f=chrome/browser/resour...
> 
> ... and register them in the manifest as normal?
That would be nice, yes; but what icon is used needs to depend on gyp defines
(chrome vs chromium), and I'm not sure if it is possible to do that? (somehow
make sure the right icon is copied into that dir before the resource file is
processed?)

> The extension system is HiDPI aware now (at least this part of it), so it
should
> properly handle getting the right one.
How does that work? It'll just know to get an icon twice the size (if the
manifest has one)? (so I should have a 256 size icon in the manifest too?
although this particular error page actually tries to get a 512 sized icon in
normal DPI mode already, there just aren't many apps that actually have one of
that size.

 
>
http://codereview.chromium.org/10985028/diff/5001/chrome/common/extensions/ex...
> File chrome/common/extensions/extension_file_util.h (right):
> 
>
http://codereview.chromium.org/10985028/diff/5001/chrome/common/extensions/ex...
> chrome/common/extensions/extension_file_util.h:135: int& resource_id);
> Use pointers for output params in Chrome. (Technically we're supposed to
prefer
> const ref for input params too, but Extension* is so widely used in Chrome
it's
> not worth diverging from)
Ah yes, not sure how that sneaked in. I guess because I copied it from
ImageLoadingTracker::IsComponentExtensionResource which also does it wrong.
Should I fix that to take a pointer instead of a reference too or is it not
worth the effort?

[Aaron Boodman, 2012-10-01 07:43:21]

On 2012/09/29 01:49:30, Marijn Kruisselbrink wrote:
> On 2012/09/28 22:38:51, Aaron Boodman wrote:
> >
>
http://codereview.chromium.org/10985028/diff/5001/chrome/browser/extensions/e...
> > File chrome/browser/extensions/extension_protocols.cc (right):
> > 
> >
>
http://codereview.chromium.org/10985028/diff/5001/chrome/browser/extensions/e...
> > chrome/browser/extensions/extension_protocols.cc:381: if
> > (extension_file_util::IsComponentExtensionResource(extension,
> > Is it possible to just put the icons here:
> > 
> >
>
http://git.chromium.org/gitweb/?p=chromium.git;a=tree;f=chrome/browser/resour...
> > 
> > ... and register them in the manifest as normal?
> That would be nice, yes; but what icon is used needs to depend on gyp defines
> (chrome vs chromium), and I'm not sure if it is possible to do that? (somehow
> make sure the right icon is copied into that dir before the resource file is
> processed?)

It seems that it must be possible, but I don't know how to do it off-hand. I
guess what you have in current patch is better. Simple and expedient.

> > The extension system is HiDPI aware now (at least this part of it), so it
> should
> > properly handle getting the right one.
> How does that work? It'll just know to get an icon twice the size (if the
> manifest has one)? (so I should have a 256 size icon in the manifest too?
> although this particular error page actually tries to get a 512 sized icon in
> normal DPI mode already, there just aren't many apps that actually have one of
> that size.

For the 'main icons' (the ones specified in the top-level icons key in the
manifest), yeah, it will just get the size greater or equal to the pixel size
needed.

> Ah yes, not sure how that sneaked in. I guess because I copied it from
> ImageLoadingTracker::IsComponentExtensionResource which also does it wrong.
> Should I fix that to take a pointer instead of a reference too or is it not
> worth the effort?

If it isn't too much work, sure.

[Aaron Boodman, 2012-10-01 07:47:35]

LGTM

[Marijn Kruisselbrink, 2012-10-01 17:04:39]

James: could you look at the chrome/browser/ui/webui/ and the
chrome/browser/resources/ parts?

[James Hawkins, 2012-10-01 21:38:05]

lgtm

[commit-bot: I haz the power, 2012-10-01 22:08:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10985028/12001

[commit-bot: I haz the power, 2012-10-02 00:37:22]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-02 00:41:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10985028/12001

[commit-bot: I haz the power, 2012-10-02 04:09:50]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-02 04:13:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10985028/12001

[commit-bot: I haz the power, 2012-10-02 08:27:00]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-10-03 14:52:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10985028/12001

[commit-bot: I haz the power, 2012-10-03 16:44:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10985028/32001

[commit-bot: I haz the power, 2012-10-03 19:42:59]

Change committed as 159950