Miscellaneous changes to Extension class in prep for user scripts

Parse more user script info out of the manifest and expose
it on the Extension class.

Removed Extension::CopyToValue() because it was only being 
used in unit tests.

Centralize functions for creation of absolute URLs and
paths to extension resources in Extension class and move
corresponding unit tests.

Various other minor nitpickery.

[Erik does not do reviews, 2009-01-20 21:32:03]

http://codereview.chromium.org/18352/diff/30/31
File chrome/browser/extensions/extension.cc (right):

http://codereview.chromium.org/18352/diff/30/31#newcode14
Line 14: const wchar_t* Extension::kFormatVersionKey = L"format_version";
please alphabetize

http://codereview.chromium.org/18352/diff/30/31#newcode26
Line 26: // cross-platform.
alphabetize

http://codereview.chromium.org/18352/diff/30/31#newcode58
Line 58: extern const char kExtensionURLScheme[];
add a comment about where this is defined

http://codereview.chromium.org/18352/diff/30/31#newcode67
Line 67: if (!StartsWithASCII(ret_val.spec(), extension_url.spec(), false))
Does this need to be here?  It seems like this is an invariant of GURL.  Maybe a
DCHECK instead?

http://codereview.chromium.org/18352/diff/30/31#newcode96
Line 96: if (!net::FileURLToFilePath(file_url, &ret_val))
It looks like this doesn't actually do what we'd like it to do here (solve the
encoding issue).  The code says deprecated, and it just uses ToWStringHack.

http://codereview.chromium.org/18352/diff/30/31#newcode123
Line 123: ReplaceSubstringsAfterOffset(&ret_val, 0, "*", s2);
I don't see how this works.  The first one replaces all instances of *, so
unless s1 itself had a * in it, the second call will do nothing.

http://codereview.chromium.org/18352/diff/30/31#newcode133
Line 133: if (path_str[0] >= 'a' && path_str[0] <= 'z')
don't you also need to verify that the second character is ':'?  Otherwise, this
could be a relative path.

http://codereview.chromium.org/18352/diff/30/31#newcode222
Line 222: if (!matches->GetString(j, &match)) {
Can you also verify that the match parses reasonably here?

http://codereview.chromium.org/18352/diff/30/32
File chrome/browser/extensions/extension.h (right):

http://codereview.chromium.org/18352/diff/30/32#newcode28
Line 28: // Keys used in JSON representation of extensions.
Since this is growing, I think we should be sorted alphabetically.  I understand
that right now you're putting it in parse order, but I think that's less useful
for someone trying to review the code to see what keys are here.

http://codereview.chromium.org/18352/diff/30/32#newcode38
Line 38: // Error messages returned from InitFromValue().
The same comment here about alphabetical order.

http://codereview.chromium.org/18352/diff/30/32#newcode58
Line 58: // NOTE: Static so that it can be used from multiple threads.
Maybe there should be a cover version of this as an instance method.

http://codereview.chromium.org/18352/diff/30/32#newcode66
Line 66: // NOTE: Static so that it can be used from multiple threads.
same here

http://codereview.chromium.org/18352/diff/30/37
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/18352/diff/30/37#newcode66
Line 66: printf("%s\n", error.c_str());
Did you mean to check this in?

[Aaron Boodman, 2009-01-20 23:11:04]

Ready for another look. I'll fix the ReplaceSubstrings() thing separately in a
different CL that I'll check in before this one.

http://codereview.chromium.org/18352/diff/30/31
File chrome/browser/extensions/extension.cc (right):

http://codereview.chromium.org/18352/diff/30/31#newcode14
Line 14: const wchar_t* Extension::kFormatVersionKey = L"format_version";
On 2009/01/20 21:32:03, Erik Kay wrote:
> please alphabetize

Done.

http://codereview.chromium.org/18352/diff/30/31#newcode26
Line 26: // cross-platform.
On 2009/01/20 21:32:03, Erik Kay wrote:
> alphabetize

Done.

http://codereview.chromium.org/18352/diff/30/31#newcode67
Line 67: if (!StartsWithASCII(ret_val.spec(), extension_url.spec(), false))
On 2009/01/20 21:32:03, Erik Kay wrote:
> Does this need to be here?  It seems like this is an invariant of GURL.  Maybe
a
> DCHECK instead?

Good point, done.

http://codereview.chromium.org/18352/diff/30/31#newcode96
Line 96: if (!net::FileURLToFilePath(file_url, &ret_val))
On 2009/01/20 21:32:03, Erik Kay wrote:
> It looks like this doesn't actually do what we'd like it to do here (solve the
> encoding issue).  The code says deprecated, and it just uses ToWStringHack.

The function it delegates to, in net_util_win.cc does some fancy stuff for
Windows. The other platforms will have to eventually do corresponding fancy
stuff.

http://codereview.chromium.org/18352/diff/30/31#newcode123
Line 123: ReplaceSubstringsAfterOffset(&ret_val, 0, "*", s2);
On 2009/01/20 21:32:03, Erik Kay wrote:
> I don't see how this works.  The first one replaces all instances of *, so
> unless s1 itself had a * in it, the second call will do nothing.

Err, good point. Despite the name, I was thinking that
ReplaceSubstringsAfterOffset only replaced one instance, and this fails in just
the right way to still pass my unit tests.

I will create a separate CL to add a utility function to string_util to replace
only the first occurrence and submit that, then fix this, before submitting this
CL.

http://codereview.chromium.org/18352/diff/30/31#newcode133
Line 133: if (path_str[0] >= 'a' && path_str[0] <= 'z')
On 2009/01/20 21:32:03, Erik Kay wrote:
> don't you also need to verify that the second character is ':'?  Otherwise,
this
> could be a relative path.

Yeah I was assuming the input path was absolute. I added a DCHECK for that.

http://codereview.chromium.org/18352/diff/30/31#newcode222
Line 222: if (!matches->GetString(j, &match)) {
On 2009/01/20 21:32:03, Erik Kay wrote:
> Can you also verify that the match parses reasonably here?

Any match pattern is currently valid. Future work will impose structure and I
will add validation then.

http://codereview.chromium.org/18352/diff/30/32
File chrome/browser/extensions/extension.h (right):

http://codereview.chromium.org/18352/diff/30/32#newcode28
Line 28: // Keys used in JSON representation of extensions.
On 2009/01/20 21:32:03, Erik Kay wrote:
> Since this is growing, I think we should be sorted alphabetically.  I
understand
> that right now you're putting it in parse order, but I think that's less
useful
> for someone trying to review the code to see what keys are here.

Great point. Done.

http://codereview.chromium.org/18352/diff/30/32#newcode38
Line 38: // Error messages returned from InitFromValue().
On 2009/01/20 21:32:03, Erik Kay wrote:
> The same comment here about alphabetical order.

Done.

http://codereview.chromium.org/18352/diff/30/32#newcode58
Line 58: // NOTE: Static so that it can be used from multiple threads.
On 2009/01/20 21:32:03, Erik Kay wrote:
> Maybe there should be a cover version of this as an instance method.

I'd like to leave this until I see if it's necessary.

http://codereview.chromium.org/18352/diff/30/32#newcode66
Line 66: // NOTE: Static so that it can be used from multiple threads.
On 2009/01/20 21:32:03, Erik Kay wrote:
> same here

Same here.

http://codereview.chromium.org/18352/diff/30/37
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/18352/diff/30/37#newcode66
Line 66: printf("%s\n", error.c_str());
On 2009/01/20 21:32:03, Erik Kay wrote:
> Did you mean to check this in?

No fixed. I was desperately trying to find some way to see this output. This
didn't work either :(

[Erik does not do reviews, 2009-01-20 23:22:27]

LGTM

http://codereview.chromium.org/18352/diff/30/37
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/18352/diff/30/37#newcode66
Line 66: printf("%s\n", error.c_str());
On 2009/01/20 23:11:04, Aaron Boodman wrote:
> On 2009/01/20 21:32:03, Erik Kay wrote:
> > Did you mean to check this in?
> 
> No fixed. I was desperately trying to find some way to see this output. This
> didn't work either :(

strange.  Is the LOG(WARNING) not working still the case?  I recently checked in
a fix where unit tests should all be initializing logging properly now.

http://codereview.chromium.org/18352/diff/60/242
File chrome/browser/extensions/extension.cc (right):

http://codereview.chromium.org/18352/diff/60/242#newcode133
Line 133: DCHECK(path_str.size() >= 2 && path_str[1] == L':');
Instead use path.IsAbsolute().  It also handles UNC paths.