Normalize extension action icon paths before adding them to icon set

Normalize extension action icon paths before adding them to icon set

TEST=manual (extension with page action default icon with absolute path is
             loaded on debug build).

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

[tbarzic, 2012-09-20 00:24:42]

this dcheck is not particularly important for page actions case, since we don't
use GetIconSizeFromPath (so there should not be problems on Release builds)

The alternative to this fix would be to ensure that the path is relative before
each call to Add, which is imho more error prone..

[not at google - send to devlin, 2012-09-20 03:26:20]

> The alternative to this fix would be to ensure that the path is relative
before
> each call to Add, which is imho more error prone..

I think it would be better though; push the normalisation into the model so that
classes that use Extension don't need to worry about it... e.g. what if more
than ExtensionIconSet needs to look at icons? The paths should be consistent.

Also: it seems like we should be passing FilePaths around rather than strings so
that we can check things like !IsAbsolute.

[tbarzic, 2012-09-20 04:58:46]

On 2012/09/20 03:26:20, kalman wrote:
> > The alternative to this fix would be to ensure that the path is relative
> before
> > each call to Add, which is imho more error prone..
> 
> I think it would be better though; push the normalisation into the model so
that
> classes that use Extension don't need to worry about it... e.g. what if more
> than ExtensionIconSet needs to look at icons? The paths should be consistent.
> 

Ok then, done it that way..

> Also: it seems like we should be passing FilePaths around rather than strings
so
> that we can check things like !IsAbsolute.

yeah, I've been thinking about doing that (in very near future :) )

[not at google - send to devlin, 2012-09-20 06:00:55]

http://codereview.chromium.org/10957008/diff/4003/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10957008/diff/4003/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:159: if (icon_path[0] == '/')
should be "while" not "if"?

also seems like this should go before the icon_path.empty check since we would
want "/" and "//" etc to trigger the invalid icon path error.

http://codereview.chromium.org/10957008/diff/4003/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:861: if (path[0] == '/')
ditto while?

actually it seems like this (and same below) isn't compatible with the check
above:

if (!(*iter)->GetAsString(&path) || path.empty()) {
  // error
}

i.e. the path.empty() check. As I said above, trimming the absolute paths out of
the icon might still leave it empty and we'd want to show a warning.

how about adding a method like NormalizeAndValidatePath method which takes a
pointer, so this would be

if (!(*iter)->GetAsString(&path) || !NormalizeAndValidatePath(path)) {
  // error
}

and use elsewhere.

or something like that.

[tbarzic, 2012-09-20 06:45:06]

http://codereview.chromium.org/10957008/diff/4003/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10957008/diff/4003/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:159: if (icon_path[0] == '/')
On 2012/09/20 06:00:55, kalman wrote:
> should be "while" not "if"?
> 
> also seems like this should go before the icon_path.empty check since we would
> want "/" and "//" etc to trigger the invalid icon path error.

Done.

http://codereview.chromium.org/10957008/diff/4003/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:861: if (path[0] == '/')
On 2012/09/20 06:00:55, kalman wrote:
> ditto while?
> 
> actually it seems like this (and same below) isn't compatible with the check
> above:
> 
> if (!(*iter)->GetAsString(&path) || path.empty()) {
>   // error
> }
> 
> i.e. the path.empty() check. As I said above, trimming the absolute paths out
of
> the icon might still leave it empty and we'd want to show a warning.
> 
> how about adding a method like NormalizeAndValidatePath method which takes a
> pointer, so this would be
> 
> if (!(*iter)->GetAsString(&path) || !NormalizeAndValidatePath(path)) {
>   // error
> }
> 
> and use elsewhere.
> 
> or something like that.

Yeah, good point.. Done.

[not at google - send to devlin, 2012-09-20 07:06:31]

lgtm

[commit-bot: I haz the power, 2012-09-20 18:44:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10957008/7002

[commit-bot: I haz the power, 2012-09-20 21:32:38]

Change committed as 157845