Minimize dependency of user scripts....

Minimize dependency of user scripts.

And made some minor lint fixes and code refactoring on the way, based on CR comments of previous attempt.

BUG=none
TEST=Make sure that the extension resources can still be properly localized and that they also load correctly when they are not localized.


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

[Paweł Hajdan Jr., 2009-10-12 15:57:53]

Drive-by.

I added a comment about the temp file because it's easier to debug flakiness
when each test has only one, fixed flow.

http://codereview.chromium.org/267051/diff/2001/2011
File chrome/browser/extensions/extension_l10n_util_unittest.cc (right):

http://codereview.chromium.org/267051/diff/2001/2011#newcode169
Line 169: if (file_util::PathExists(inexistent))
How about ASSERT_FALSE(file_util::PathExists(inexistent))?

The file is in a temp directory. It really shouldn't exist.

http://codereview.chromium.org/267051/diff/2001/2008
File net/base/net_util.h (right):

http://codereview.chromium.org/267051/diff/2001/2008#newcode17
Line 17: #include "build/build_config.h"
Ooops. Without it you won't have OS_WIN above. Please don't move this header.

[Aaron Boodman, 2009-10-12 15:58:54]

Seems OK to me. You should ping darin or brettw directly and make sure they see
this.

http://codereview.chromium.org/267051/diff/2001/2008
File net/base/net_util.h (right):

http://codereview.chromium.org/267051/diff/2001/2008#newcode53
Line 53: // URL encoding and path separator issues.
We should update this comment now that this has moved so far away from the
extension system. What this is really trying to do:

Combine base_path and relative_path, and ensure the result is still a child of
base_path.

http://codereview.chromium.org/267051/diff/2001/2008#newcode54
Line 54: FilePath CombinePathsSafely(const FilePath& extension_path,
We should get Darin's opinion on putting this here.

At the very least, you should remove the references to 'extensions' in the
comments as base/ knows nothing about the extensions system.

This may make more sense someplace like browser/common or app/. Would either of
those places address your dependency issues?

[MAD, 2009-10-12 16:58:01]

Changes based on initial feedback...

And added Brett and Darin to reviewers to validate addition of a new function in
net_util...

Thanks!

BYE
MAD

http://codereview.chromium.org/267051/diff/2001/2011
File chrome/browser/extensions/extension_l10n_util_unittest.cc (right):

http://codereview.chromium.org/267051/diff/2001/2011#newcode169
Line 169: if (file_util::PathExists(inexistent))
I have bitten by this in other projects where tests would get interrupted half
way there and the temp files didn't get deleted and the next time the tests ran,
they failed...

So I prefer to leave it as is if it is OK with everyone. :-)

http://codereview.chromium.org/267051/diff/2001/2008
File net/base/net_util.h (right):

http://codereview.chromium.org/267051/diff/2001/2008#newcode17
Line 17: #include "build/build_config.h"
On 2009/10/12 15:57:54, Paweł Hajdan Jr. wrote:
> Ooops. Without it you won't have OS_WIN above. Please don't move this header.

Done.
Good catch, I noticed that in the source file and moved it back with a NOLINT,
but forgot about the header... Thanks! :-)

http://codereview.chromium.org/267051/diff/2001/2008#newcode53
Line 53: // URL encoding and path separator issues.
On 2009/10/12 15:58:54, Aaron Boodman wrote:
> We should update this comment now that this has moved so far away from the
> extension system. What this is really trying to do:
> 
> Combine base_path and relative_path, and ensure the result is still a child of
> base_path.

Thanks, you're right... I thought I had removed all the references to extension,
but have missed this one...
Done.

http://codereview.chromium.org/267051/diff/2001/2008#newcode54
Line 54: FilePath CombinePathsSafely(const FilePath& extension_path,
On 2009/10/12 15:58:54, Aaron Boodman wrote:
> We should get Darin's opinion on putting this here.
> 
> At the very least, you should remove the references to 'extensions' in the
> comments as base/ knows nothing about the extensions system.
> 
> This may make more sense someplace like browser/common or app/. Would either
of
> those places address your dependency issues?

I'll double check with them, but I think it makes sense here, at least to me, it
seems very similar to other functions in this namespace... no?

[Paweł Hajdan Jr., 2009-10-12 17:05:56]

http://codereview.chromium.org/267051/diff/2001/2011
File chrome/browser/extensions/extension_l10n_util_unittest.cc (right):

http://codereview.chromium.org/267051/diff/2001/2011#newcode169
Line 169: if (file_util::PathExists(inexistent))
ScopedTempDir should be different for each test run. But if you prefer the
current version, it's quite ok for me.

[Nebojša Ćirić, 2009-10-12 17:16:37]

http://codereview.chromium.org/267051/diff/4001/4009
File chrome/browser/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/267051/diff/4001/4009#newcode159
Line 159: .Append(relative_resource_path);
New line here?

http://codereview.chromium.org/267051/diff/4001/4010
File chrome/browser/extensions/extension_l10n_util.h (right):

http://codereview.chromium.org/267051/diff/4001/4010#newcode59
Line 59: // relative path as is otherwise.
I would say, after comma:

returns the given relative path otherwise.

http://codereview.chromium.org/267051/diff/4001/4005
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/267051/diff/4001/4005#newcode441
Line 441: extension_path, FilePath().AppendASCII(trimmed_path));
I don't think you can call GetL10nRelativePath here since it calls PathExist(),
and you can do that only on file thread.

[MAD, 2009-10-12 17:41:03]

Press Pause on your Code Review player...

And wait for the next revision (which won't come today, it's actually a Holiday
up here and I shouldn't even be working. ;-)

But I thought I could get you guys to confirm that what I was trying (on and off
during the weekend) made sense... And it seems it doesn't quite satisfy the
requirements, based on the last comments from cira...

So back to the drawing board... Suggestions are welcome :-)...

BYE
MAD

http://codereview.chromium.org/267051/diff/4001/4009
File chrome/browser/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/267051/diff/4001/4009#newcode159
Line 159: .Append(relative_resource_path);
On 2009/10/12 17:16:37, Nebojša Ćirić wrote:
> New line here?

Done.

http://codereview.chromium.org/267051/diff/4001/4010
File chrome/browser/extensions/extension_l10n_util.h (right):

http://codereview.chromium.org/267051/diff/4001/4010#newcode59
Line 59: // relative path as is otherwise.
On 2009/10/12 17:16:37, Nebojša Ćirić wrote:
> I would say, after comma:
> 
> returns the given relative path otherwise.

Done.

http://codereview.chromium.org/267051/diff/4001/4005
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/267051/diff/4001/4005#newcode441
Line 441: extension_path, FilePath().AppendASCII(trimmed_path));
On 2009/10/12 17:16:37, Nebojša Ćirić wrote:
> I don't think you can call GetL10nRelativePath here since it calls
PathExist(),
> and you can do that only on file thread.

D'ho!

This changes everything... Sorry about that, I missed this subtlety... Back to
the drawing board then... :-(

[Aaron Boodman, 2009-10-12 17:46:56]

http://codereview.chromium.org/267051/diff/4001/4005
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/267051/diff/4001/4005#newcode441
Line 441: extension_path, FilePath().AppendASCII(trimmed_path));
On 2009/10/12 17:16:37, Nebojša Ćirić wrote:
> I don't think you can call GetL10nRelativePath here since it calls
PathExist(),
> and you can do that only on file thread.

Nice catch.

[brettw, 2009-10-12 19:35:48]

> http://codereview.chromium.org/267051/diff/2001/2008#newcode53
> Line 53: // URL encoding and path separator issues.
> We should update this comment now that this has moved so far away from the
> extension system. What this is really trying to do:
> 
> Combine base_path and relative_path, and ensure the result is still a child of
> base_path.

Why *has* this moved so far from the extensions system?

Brett

[brettw, 2009-10-13 02:57:06]

To clarify my previous comment (I'll be out most of Tuesday): I don't see what
this has to do with net at all. It doesn't take any net types nor use any net
services. It isn't called by other parts of net, and it isn't used by any
consumer of the network stack other than Chrome. Therefore, it should definitely
not be in net_util.

I'm also kind of disturbed by the implementation of CombinePathsSafely. Why does
it round-trip through GURL? This seems both unnecessary and error-prone.

[Aaron Boodman, 2009-10-13 07:16:20]

On 2009/10/13 02:57:06, brettw wrote:
> To clarify my previous comment (I'll be out most of Tuesday): I don't see what
> this has to do with net at all. It doesn't take any net types nor use any net
> services. It isn't called by other parts of net, and it isn't used by any
> consumer of the network stack other than Chrome. Therefore, it should
definitely
> not be in net_util.

Makes sense.

> I'm also kind of disturbed by the implementation of CombinePathsSafely. Why
does
> it round-trip through GURL? This seems both unnecessary and error-prone.

I had to do some spelunking to figure this out.

This function got refactored and in the process its signature changed and it
makes less sense now.

The point used to be that we want to take arbitrary input in the form of a GURL
(chrome-xtension://<id>/foo/bar/baz) and convert it into a path below an
extension's root.

Due to encoding issues, it was recommended that the safest way to do this was to
convert the extension root to a file URL, replace the path, then convert back to
a path.

In the refactor though, the code got changed to convert the arbitrary input
directly into a FilePath (using AppendASCII()), which is then passed into here.

Anyway, I think that we still want the roundtrip through GURL, but we should
change it back to use the string input.

I can't remember why this function had to move out of extensions/ in the first
place. Some deps thing. Cira, do you remember?

[MAD, 2009-10-14 22:27:19]

Salut,

   here's another attempt at resolving the issues identified in the previous
version's code review...

BYE
MAD

[MAD, 2009-10-15 22:30:51]

Ping?
Thanks!

BYE
MAD

On Wed, Oct 14, 2009 at 6:27 PM, <mad@chromium.org> wrote:

> Salut,
>
>   here's another attempt at resolving the issues identified in the previous
> version's code review...
>
> BYE
> MAD
>
>
>
> http://codereview.chromium.org/267051
>

[Nebojša Ćirić, 2009-10-16 04:50:35]

I'll take a look at it on Monday. I am at Unicode conference Wed-Fri this
week...

On Thu, Oct 15, 2009 at 3:30 PM, Marc-Andre Decoste <mad@chromium.org>wrote:

> Ping?
> Thanks!
>
> BYE
> MAD
>
>
> On Wed, Oct 14, 2009 at 6:27 PM, <mad@chromium.org> wrote:
>
>> Salut,
>>
>>   here's another attempt at resolving the issues identified in the
>> previous
>> version's code review...
>>
>> BYE
>> MAD
>>
>>
>>
>> http://codereview.chromium.org/267051
>>
>
>

[Aaron Boodman, 2009-10-16 05:24:53]

The extension parts look good to me. Defer to brettw on FilePath change, but it
also looks nice imo.

[Nebojša Ćirić, 2009-10-16 22:37:40]

lgtm

So, by removing ExtensionResource from UserScript::File you got rid of deps. on
app/l10n_util?

If that's true, it looks good to me too (see nit below).

http://codereview.chromium.org/267051/diff/10001/11004
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/267051/diff/10001/11004#newcode190
Line 190: const FilePath& path = resource.GetFilePath();
Not a big deal, but if you had a static version of std::string GetFilePath(root,
relative, *full_path); you could avoid creating local ExtensionResource object
just for that one call...

[MAD, 2009-10-19 20:00:05]

Added the static method requested by Nebojša...

Brett, are you OK with the FilePath changes you had requested???

Thanks!

BYE
MAD

http://codereview.chromium.org/267051/diff/10001/11004
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/267051/diff/10001/11004#newcode190
Line 190: const FilePath& path = resource.GetFilePath();
On 2009/10/16 22:37:40, Nebojša Ćirić wrote:
> Not a big deal, but if you had a static version of std::string
GetFilePath(root,
> relative, *full_path); you could avoid creating local ExtensionResource object
> just for that one call...

Done.

[brettw, 2009-10-19 20:39:20]

I'm checking the logic right now on the FilePath function. The unit test is very
nice, thanks.

I would like to see a unit test example where there's a "." for a directory, if
only to show that we don't handle them.

[Nebojša Ćirić, 2009-10-19 21:20:44]

http://codereview.chromium.org/267051/diff/16001/16008
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/267051/diff/16001/16008#newcode141
Line 141: const FilePath& path = resource.GetFilePath();
Use static GetFilePath() here?

http://codereview.chromium.org/267051/diff/16001/16008#newcode175
Line 175: resource.extension_root(), resource.relative_path(), url));
Do you need ExtensionResource here? Why not use script_dir and file.BaseName()
directly, since extension_root() and relative_path() methods are not l10n
sensitive.

http://codereview.chromium.org/267051/diff/16001/16010
File chrome/common/extensions/extension_resource.cc (right):

http://codereview.chromium.org/267051/diff/16001/16010#newcode45
Line 45: &full_path) &&
Nit (no need to reply):
Could "&full_path) &&" fit on previous line?

[MAD, 2009-10-19 21:41:06]

Adressed Nebojša's comments (thanks for the good tips ;-)

And also added support for "." in the path as hinted by Brett (thanks for that
too) and updated unit test as well...

How about that?

BYE
MAD

http://codereview.chromium.org/267051/diff/16001/16008
File chrome/browser/extensions/user_script_master.cc (right):

http://codereview.chromium.org/267051/diff/16001/16008#newcode141
Line 141: const FilePath& path = resource.GetFilePath();
On 2009/10/19 21:20:44, Nebojša Ćirić wrote:
> Use static GetFilePath() here?

Done.

http://codereview.chromium.org/267051/diff/16001/16008#newcode175
Line 175: resource.extension_root(), resource.relative_path(), url));
On 2009/10/19 21:20:44, Nebojša Ćirić wrote:
> Do you need ExtensionResource here? Why not use script_dir and file.BaseName()
> directly, since extension_root() and relative_path() methods are not l10n
> sensitive.

Done.

http://codereview.chromium.org/267051/diff/16001/16010
File chrome/common/extensions/extension_resource.cc (right):

http://codereview.chromium.org/267051/diff/16001/16010#newcode45
Line 45: &full_path) &&
On 2009/10/19 21:20:44, Nebojša Ćirić wrote:
> Nit (no need to reply):
> Could "&full_path) &&" fit on previous line?

Done.

[brettw, 2009-10-19 23:47:24]

Maybe I'm misunderstanding something, but the old GURL code if given
  "/foo/bar" + "baz" = "/foo/baz"
The new code will just append, so you'll get
  "/foo/bar/baz"
This kind of makes sense, but doesn't at all if you have a filename
  FilePath("c:\foo\bar.html").ResolveRelative("hello")
will generate "c:\foo\bar.html\hello"

Is this really what we want? I suspect since we don't know whether something is
a directory or not means that we can't do any better here. The rules for URLs
mean that there's always a trailing slash at the end when it's a directory.

(This is one reason why we shouldn't use the URL resolution code here, I think.)

Since we can't do this "right" without touching the disk, I think probably a
different name would make it more clear, and we should have a big warning above
the function about this behavior. I'm thinking something like "AppendAndResolve"
or something else beginning with "Append" to imply that this just appends and
then fixes up ".."

[MAD, 2009-10-20 00:14:36]

The current usage has to be made with a folder as the path we append to, so we
are OK.

I changed the name of the method as suggested and added comments to make it even
more obvious.

OK?

Thanks again!

BYE
MAD

[brettw, 2009-10-20 06:06:19]

LGTM, thanks for the patience.

http://codereview.chromium.org/267051/diff/19003/19005
File base/file_path.h (right):

http://codereview.chromium.org/267051/diff/19003/19005#newcode185
Line 185: // Note that if the current path ends with a file name, we won't try
to
Can you put a blank line separating these two paragraphs to make it easier to
read?

[MAD, 2009-10-20 13:43:28]

Thank YOU for YOUR patience :-)

Will commit as soon as I rebuilt and re-ran the tests after a sync...

BYE
MAD

http://codereview.chromium.org/267051/diff/19003/19005
File base/file_path.h (right):

http://codereview.chromium.org/267051/diff/19003/19005#newcode185
Line 185: // Note that if the current path ends with a file name, we won't try
to
On 2009/10/20 06:06:19, brettw wrote:
> Can you put a blank line separating these two paragraphs to make it easier to
> read?

Done.

[Mark Mentovai, 2009-10-23 21:29:40]

The file_path part of this change needs to be backed out.

http://codereview.chromium.org/267051/diff/19003/19004
File base/file_path.cc (right):

http://codereview.chromium.org/267051/diff/19003/19004#newcode161
Line 161: bool FilePath::AppendAndResolveRelative(const FilePath& relative_path,
This is wrong and we need to remove it.

We can't resolve .. without referencing the disk.  (Think symbolic links.)

We can't reference the disk in FilePath.

If you need to resolve .., and you need to hit the disk, you need something in
file_util, not file_path.

[Aaron Boodman, 2009-10-23 21:58:41]

On Fri, Oct 23, 2009 at 2:29 PM,  <mark@chromium.org> wrote:
>
> The file_path part of this change needs to be backed out.
>
>
> http://codereview.chromium.org/267051/diff/19003/19004
> File base/file_path.cc (right):
>
> http://codereview.chromium.org/267051/diff/19003/19004#newcode161
> Line 161: bool FilePath::AppendAndResolveRelative(const FilePath&
> relative_path,
> This is wrong and we need to remove it.
>
> We can't resolve .. without referencing the disk. =A0(Think symbolic
> links.)

We want unresolved symbolic links in this case I think. Maybe
"resolve" is the wrong name?

- a

[MAD, 2009-10-25 02:57:54]

Salut,

   OK, I could rename it... and I was thinking that doing both the append
and the relative resolution in the same method, might  be a bit too much...
So here is what I would propose...

   We remove the Append call from the method and let the callers do it on
their own. And then, rename the method to "ResolveParentReferences" which is
what we are doing.... This is why we have nothing to do if
"ReferencesParent" returns false...

   Deal?

Thanks!

BYE
MAD

On Fri, Oct 23, 2009 at 5:58 PM, Aaron Boodman <aa@chromium.org> wrote:

> On Fri, Oct 23, 2009 at 2:29 PM,  <mark@chromium.org> wrote:
> >
> > The file_path part of this change needs to be backed out.
> >
> >
> > http://codereview.chromium.org/267051/diff/19003/19004
> > File base/file_path.cc (right):
> >
> > http://codereview.chromium.org/267051/diff/19003/19004#newcode161
> > Line 161: bool FilePath::AppendAndResolveRelative(const FilePath&
> > relative_path,
> > This is wrong and we need to remove it.
> >
> > We can't resolve .. without referencing the disk.  (Think symbolic
> > links.)
>
> We want unresolved symbolic links in this case I think. Maybe
> "resolve" is the wrong name?
>
> - a
>