Loads local resources from current locale subtree if available, if not it fal...

Loads local resources from current locale subtree if available, if not it falls back to extension subtree.

We look for ext_root/foo/bar.js under ext_root/_locales/fr/foo/bar.js if current locale is fr. If there is no fr specific resource we load ext_root/foo/bar.js instead.

Lots of small refactoring to replace FilePath with ExtensionResource.


BUG=12131
TEST=See unittest for sample tree.

[Aaron Boodman, 2009-10-01 03:17:01]

GetResourcePath() is used in several places on the UI thread. We cannot call
file_util::PathExists inside it.

But I see the desire to centralize dispatch to the right file in one location.

What if we renamed Extension::GetResourcePath() -> GetResource() and had it
return a new ExtensionResource object which was an opaque thing. That thing
could only be used by a new util function in extension_file_util like
GetPathForExtensionResource(), which would do the actual resolution to the
possibly localized resource.

This would ensure that all future code doesn't mess up an accidentally use
Extension::GetResourcePath() when it should actually be doing localization *and*
would avoid doing filesystem poking on the UI thread.

[Nebojša Ćirić, 2009-10-01 15:48:58]

I was afraid something like this would happen since it looked too simple :).

Ok, so create ExtensionResource class:

class ExtensionResource {
...
private:
 FilePath original;
 FilePath localized;
 friend extension_file_util::GetPathForExtensionResource();
};

then pass ExtensionResource around (refactor all the callers), and hope that
callers would be aware that GetPathForExtensionResource may stat the file since
it's in extension_file_util.

Sounds good?

On 2009/10/01 03:17:01, Aaron Boodman wrote:
> GetResourcePath() is used in several places on the UI thread. We cannot call
> file_util::PathExists inside it.
> 
> But I see the desire to centralize dispatch to the right file in one location.
> 
> What if we renamed Extension::GetResourcePath() -> GetResource() and had it
> return a new ExtensionResource object which was an opaque thing. That thing
> could only be used by a new util function in extension_file_util like
> GetPathForExtensionResource(), which would do the actual resolution to the
> possibly localized resource.
> 
> This would ensure that all future code doesn't mess up an accidentally use
> Extension::GetResourcePath() when it should actually be doing localization
*and*
> would avoid doing filesystem poking on the UI thread.

[Nebojša Ćirić, 2009-10-05 22:28:55]

Ready for another pass.

On 2009/10/01 15:48:58, Nebojša Ćirić wrote:
> I was afraid something like this would happen since it looked too simple :).
> 
> Ok, so create ExtensionResource class:
> 
> class ExtensionResource {
> ...
> private:
>  FilePath original;
>  FilePath localized;
>  friend extension_file_util::GetPathForExtensionResource();
> };
> 
> then pass ExtensionResource around (refactor all the callers), and hope that
> callers would be aware that GetPathForExtensionResource may stat the file
since
> it's in extension_file_util.
> 
> Sounds good?
> 
> On 2009/10/01 03:17:01, Aaron Boodman wrote:
> > GetResourcePath() is used in several places on the UI thread. We cannot call
> > file_util::PathExists inside it.
> > 
> > But I see the desire to centralize dispatch to the right file in one
location.
> > 
> > What if we renamed Extension::GetResourcePath() -> GetResource() and had it
> > return a new ExtensionResource object which was an opaque thing. That thing
> > could only be used by a new util function in extension_file_util like
> > GetPathForExtensionResource(), which would do the actual resolution to the
> > possibly localized resource.
> > 
> > This would ensure that all future code doesn't mess up an accidentally use
> > Extension::GetResourcePath() when it should actually be doing localization
> *and*
> > would avoid doing filesystem poking on the UI thread.

[Aaron Boodman, 2009-10-06 01:54:41]

http://codereview.chromium.org/256022/diff/6001/6026
File chrome/browser/net/file_reader.h (right):

http://codereview.chromium.org/256022/diff/6001/6026#newcode24
Line 24: FileReader(const ExtensionResource& resource, Callback* callback);
You cannot refer to things in chrome/* from base/*. This will fail on checkin.

Looking at the changes to ImageTrackingLoader, it's the same problem. I think we
should create a simple FilePathProvider interface in base/ that
ExtensionResource implements.

It would just be:

FilePathProvider
  virtual ~FilePathProvider()
  virtual const FilePath& ProvideFilePath();

FilePath could also be modified to implement this by just returning itself.

That way we just need to modify ImageTrackingLoader and FileReader to accept
FilePathProvider instead of FilePath and everything should just work.

http://codereview.chromium.org/256022/diff/6001/6029
File chrome/common/extensions/extension_resource.h (right):

http://codereview.chromium.org/256022/diff/6001/6029#newcode16
Line 16: // Class that holds all posible paths for an extension resource
Comment nit. How about:

Represents a resource inside an extension. For example, an image, or a
JavaScript file. This is more complicated than just a simple FilePath because
extension resources can come from multiple physical file locations depending on
locale.

http://codereview.chromium.org/256022/diff/6001/6029#newcode26
Line 26: const FilePath& default_relative_path,
Why are default_relative_path and l10n_relative_path needed? 

It seems like this class should only contain enough information to answer the
question GetFilePath().

http://codereview.chromium.org/256022/diff/6001/6029#newcode27
Line 27: const FilePath& l10n_relative_path);
If I understand things correctly, the path to any given resource could from
three locations (in descending priority):

a) <extension-root>/<chrome-current-locale>/foo.gif
b) <extension-root>/<extension-default-locale>/foo.gif
c) <extension-root>/foo.gif

It doesn't seem like this code is handling b). I can't remember what we decide
here. Did we decide not to do b)?

http://codereview.chromium.org/256022/diff/6001/6029#newcode31
Line 31: ExtensionResource& operator=(const ExtensionResource& resource);
We usually discourage operator overloading. Is this necessary?

http://codereview.chromium.org/256022/diff/6001/6029#newcode47
Line 47: enum PathFollowed {
Seeing the extension_file_util+friend in action, I think it is a bit over
complicated. I think this can just be:

class ExtensionResource : public FilePathProvider // see other comment
 public:
  ExtensionResource(FilePath extension_root, FilePath relative_path);

  // Might hit filesystem. Do not call on UI thread!
  virtual const FilePath& ProvideFilePath();

 private:
  // Whatever state is necessary to compute GetFilePath().

Question about the state in this class:

[Nebojša Ćirić, 2009-10-06 16:37:57]

I just have 2 questions (agree on everything else):

1. how is chrome/browser/net/ the same as base/? Try bots usually complain about
deps across packages.

2. Is there a way to access current extension object from a static method (like
GetResource)? If I can do that, then I can access default_locale from the
extension object, and fall back properly - current locale -> default locale ->
extension root.

Also, it looks like you had additional question about the state in the class (at
the end), but was either cut or you forgot to type it :)

Cira

On 2009/10/06 01:54:41, Aaron Boodman wrote:
> http://codereview.chromium.org/256022/diff/6001/6026
> File chrome/browser/net/file_reader.h (right):
> 
> http://codereview.chromium.org/256022/diff/6001/6026#newcode24
> Line 24: FileReader(const ExtensionResource& resource, Callback* callback);
> You cannot refer to things in chrome/* from base/*. This will fail on checkin.
> 
> Looking at the changes to ImageTrackingLoader, it's the same problem. I think
we
> should create a simple FilePathProvider interface in base/ that
> ExtensionResource implements.
> 
> It would just be:
> 
> FilePathProvider
>   virtual ~FilePathProvider()
>   virtual const FilePath& ProvideFilePath();
> 
> FilePath could also be modified to implement this by just returning itself.
> 
> That way we just need to modify ImageTrackingLoader and FileReader to accept
> FilePathProvider instead of FilePath and everything should just work.
> 
> http://codereview.chromium.org/256022/diff/6001/6029
> File chrome/common/extensions/extension_resource.h (right):
> 
> http://codereview.chromium.org/256022/diff/6001/6029#newcode16
> Line 16: // Class that holds all posible paths for an extension resource
> Comment nit. How about:
> 
> Represents a resource inside an extension. For example, an image, or a
> JavaScript file. This is more complicated than just a simple FilePath because
> extension resources can come from multiple physical file locations depending
on
> locale.
> 
> http://codereview.chromium.org/256022/diff/6001/6029#newcode26
> Line 26: const FilePath& default_relative_path,
> Why are default_relative_path and l10n_relative_path needed? 
> 
> It seems like this class should only contain enough information to answer the
> question GetFilePath().
> 
> http://codereview.chromium.org/256022/diff/6001/6029#newcode27
> Line 27: const FilePath& l10n_relative_path);
> If I understand things correctly, the path to any given resource could from
> three locations (in descending priority):
> 
> a) <extension-root>/<chrome-current-locale>/foo.gif
> b) <extension-root>/<extension-default-locale>/foo.gif
> c) <extension-root>/foo.gif
> 
> It doesn't seem like this code is handling b). I can't remember what we decide
> here. Did we decide not to do b)?
> 
> http://codereview.chromium.org/256022/diff/6001/6029#newcode31
> Line 31: ExtensionResource& operator=(const ExtensionResource& resource);
> We usually discourage operator overloading. Is this necessary?
> 
> http://codereview.chromium.org/256022/diff/6001/6029#newcode47
> Line 47: enum PathFollowed {
> Seeing the extension_file_util+friend in action, I think it is a bit over
> complicated. I think this can just be:
> 
> class ExtensionResource : public FilePathProvider // see other comment
>  public:
>   ExtensionResource(FilePath extension_root, FilePath relative_path);
> 
>   // Might hit filesystem. Do not call on UI thread!
>   virtual const FilePath& ProvideFilePath();
> 
>  private:
>   // Whatever state is necessary to compute GetFilePath().
> 
> Question about the state in this class:

[Aaron Boodman, 2009-10-06 16:59:32]

On 2009/10/06 16:37:57, Nebojša Ćirić wrote:
> I just have 2 questions (agree on everything else):
> 
> 1. how is chrome/browser/net/ the same as base/? Try bots usually complain
about
> deps across packages.

You are right! I thought file_reader was in base/.

Let me re-evaluate my suggestion.

[Aaron Boodman, 2009-10-06 17:10:12]

On 2009/10/06 16:37:57, Nebojša Ćirić wrote:
> I just have 2 questions (agree on everything else):
> 
> 1. how is chrome/browser/net/ the same as base/? Try bots usually complain
about
> deps across packages.

Thanks for pointing this out, my mistake.

Ok, new suggestion.

I think that your changes to ImageTrackingLoader and FileReader are good, but
since they can now only be used by the ExtensionSystem, I think they should be
moved into browser/extensions.

We can add a comment to each of them to consider abstracting out a
FilePathProvider and moving back to base or chrome/common if other subsystems
want to use them.

> 2. Is there a way to access current extension object from a static method
(like
> GetResource)? If I can do that, then I can access default_locale from the
> extension object, and fall back properly - current locale -> default locale ->
> extension root.

No, but there is no reason that you can't copy the extension's default locale
into ExtensionResource.

> Also, it looks like you had additional question about the state in the class
(at
> the end), but was either cut or you forgot to type it :)

My question/comment was that it seemed like there was the wrong set of state in
the class. I think that the following should be sufficient:

- extension root path
- requested relative path
- extension default locale
- resolved full path (starts out empty, used as a cache)

I wasn't sure what the other bool, enum, and path were needed for.

Hit me up on IRC if you want to talk about this more.

[Nebojša Ćirić, 2009-10-06 22:35:02]

All done.

On 2009/10/06 17:10:12, Aaron Boodman wrote:
> On 2009/10/06 16:37:57, Nebojša Ćirić wrote:
> > I just have 2 questions (agree on everything else):
> > 
> > 1. how is chrome/browser/net/ the same as base/? Try bots usually complain
> about
> > deps across packages.
> 
> Thanks for pointing this out, my mistake.
> 
> Ok, new suggestion.
> 
> I think that your changes to ImageTrackingLoader and FileReader are good, but
> since they can now only be used by the ExtensionSystem, I think they should be
> moved into browser/extensions.
> 
> We can add a comment to each of them to consider abstracting out a
> FilePathProvider and moving back to base or chrome/common if other subsystems
> want to use them.
> 
> > 2. Is there a way to access current extension object from a static method
> (like
> > GetResource)? If I can do that, then I can access default_locale from the
> > extension object, and fall back properly - current locale -> default locale
->
> > extension root.
> 
> No, but there is no reason that you can't copy the extension's default locale
> into ExtensionResource.
> 
> > Also, it looks like you had additional question about the state in the class
> (at
> > the end), but was either cut or you forgot to type it :)
> 
> My question/comment was that it seemed like there was the wrong set of state
in
> the class. I think that the following should be sufficient:
> 
> - extension root path
> - requested relative path
> - extension default locale
> - resolved full path (starts out empty, used as a cache)
> 
> I wasn't sure what the other bool, enum, and path were needed for.
> 
> Hit me up on IRC if you want to talk about this more.

[Aaron Boodman, 2009-10-06 23:09:05]

http://codereview.chromium.org/256022/diff/3003/9081
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/256022/diff/3003/9081#newcode128
Line 128: extension_->GetIconPath(Extension::EXTENSION_ICON_LARGE)
Nit: this line should be indented two more (and same with one below).

http://codereview.chromium.org/256022/diff/3003/9084
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/256022/diff/3003/9084#newcode105
Line 105: resource_.relative_path().value());
Nice: convenient having that relative_path().

http://codereview.chromium.org/256022/diff/3003/9075
File chrome/browser/extensions/extension_l10n_util_unittest.cc (right):

http://codereview.chromium.org/256022/diff/3003/9075#newcode147
Line 147: bool ComparePaths(const FilePath& path, const FilePath& result) {
Maybe PathsAreEqual(const FilePath& path1, const FilePath& path2) ?

http://codereview.chromium.org/256022/diff/3003/9075#newcode148
Line 148: FilePath::StringType result_slash = result.value();
Odd name. path1_str ?

http://codereview.chromium.org/256022/diff/3003/9098
File chrome/common/extensions/extension_resource.cc (right):

http://codereview.chromium.org/256022/diff/3003/9098#newcode31
Line 31: const FilePath& ExtensionResource::ResolveResourcePath() const {
Sorry for the name nitting, but now that we don't have the interface, how about
just GetFilePath(). The one below could be CombinePathsSafely() maybe?

http://codereview.chromium.org/256022/diff/3003/9098#newcode49
Line 49: if (file_util::PathExists(full_resource_path_)) {
No need to do this second PathExists() check. If it doesn't exist, it will just
behave like the current code.

http://codereview.chromium.org/256022/diff/3003/9099
File chrome/common/extensions/extension_resource.h (right):

http://codereview.chromium.org/256022/diff/3003/9099#newcode16
Line 16: ExtensionResource();
Do we need the default constructor? Seems invalid to have an extension resource
with no path.

http://codereview.chromium.org/256022/diff/3003/9099#newcode21
Line 21: ExtensionResource(const ExtensionResource& resource);
Do we need the copy constructor? If we do, I think the compiler will generate it
if you leave this out, right?

http://codereview.chromium.org/256022/diff/3003/9099#newcode24
Line 24: // Might hit filesystem. Do not call on UI thread!
Can you put some stars and capital letters around this :).

http://codereview.chromium.org/256022/diff/3003/9099#newcode52
Line 52: bool is_valid_;
If default constructor goes away, no need for this.

[Nebojša Ćirić, 2009-10-07 00:40:24]

http://codereview.chromium.org/256022/diff/3003/9081
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/256022/diff/3003/9081#newcode128
Line 128: extension_->GetIconPath(Extension::EXTENSION_ICON_LARGE)
On 2009/10/06 23:09:05, Aaron Boodman wrote:
> Nit: this line should be indented two more (and same with one below).

Hm, I always (at least in google3) indented 2 spaces after = and 4 for function
parameter.

Done.

http://codereview.chromium.org/256022/diff/3003/9075
File chrome/browser/extensions/extension_l10n_util_unittest.cc (right):

http://codereview.chromium.org/256022/diff/3003/9075#newcode147
Line 147: bool ComparePaths(const FilePath& path, const FilePath& result) {
On 2009/10/06 23:09:05, Aaron Boodman wrote:
> Maybe PathsAreEqual(const FilePath& path1, const FilePath& path2) ?

Done.

http://codereview.chromium.org/256022/diff/3003/9075#newcode148
Line 148: FilePath::StringType result_slash = result.value();
On 2009/10/06 23:09:05, Aaron Boodman wrote:
> Odd name. path1_str ?

Done.

http://codereview.chromium.org/256022/diff/3003/9098
File chrome/common/extensions/extension_resource.cc (right):

http://codereview.chromium.org/256022/diff/3003/9098#newcode31
Line 31: const FilePath& ExtensionResource::ResolveResourcePath() const {
On 2009/10/06 23:09:05, Aaron Boodman wrote:
> Sorry for the name nitting, but now that we don't have the interface, how
about
> just GetFilePath(). The one below could be CombinePathsSafely() maybe?

Done.

http://codereview.chromium.org/256022/diff/3003/9098#newcode49
Line 49: if (file_util::PathExists(full_resource_path_)) {
On 2009/10/06 23:09:05, Aaron Boodman wrote:
> No need to do this second PathExists() check. If it doesn't exist, it will
just
> behave like the current code.

Done.

http://codereview.chromium.org/256022/diff/3003/9099
File chrome/common/extensions/extension_resource.h (right):

http://codereview.chromium.org/256022/diff/3003/9099#newcode16
Line 16: ExtensionResource();
We keep instances of the ExtensionResource around.

On 2009/10/06 23:09:05, Aaron Boodman wrote:
> Do we need the default constructor? Seems invalid to have an extension
resource
> with no path.

http://codereview.chromium.org/256022/diff/3003/9099#newcode16
Line 16: ExtensionResource();
We do need default constructor (we keep instances not pointers to instances
around so compiler complains about missing default constructor).

On 2009/10/06 23:09:05, Aaron Boodman wrote:
> Do we need the default constructor? Seems invalid to have an extension
resource
> with no path.

http://codereview.chromium.org/256022/diff/3003/9099#newcode21
Line 21: ExtensionResource(const ExtensionResource& resource);
On 2009/10/06 23:09:05, Aaron Boodman wrote:
> Do we need the copy constructor? If we do, I think the compiler will generate
it
> if you leave this out, right?

Done.

http://codereview.chromium.org/256022/diff/3003/9099#newcode24
Line 24: // Might hit filesystem. Do not call on UI thread!
On 2009/10/06 23:09:05, Aaron Boodman wrote:
> Can you put some stars and capital letters around this :).

Done.

http://codereview.chromium.org/256022/diff/3003/9099#newcode52
Line 52: bool is_valid_;
is_valid is not really necessary so I've removed it. I've added check if
extension_root or relative_path are empty (which is equivalent but more
readable).

On 2009/10/06 23:09:05, Aaron Boodman wrote:
> If default constructor goes away, no need for this.

[Aaron Boodman, 2009-10-07 17:47:27]

lgtm

http://codereview.chromium.org/256022/diff/3003/9081
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/256022/diff/3003/9081#newcode128
Line 128: extension_->GetIconPath(Extension::EXTENSION_ICON_LARGE)
On 2009/10/07 00:40:24, Nebojša Ćirić wrote:
> On 2009/10/06 23:09:05, Aaron Boodman wrote:
> > Nit: this line should be indented two more (and same with one below).
> 
> Hm, I always (at least in google3) indented 2 spaces after = and 4 for
function
> parameter.
> 
> Done.
> 

Oh, maybe I am wrong then. This is how I've always done it in Chromium, but to
be honest, the google3 indent rules are a little baroque in my opinion. I
wouldn't be surprised if I misunderstood them.

http://codereview.chromium.org/256022/diff/3003/9099
File chrome/common/extensions/extension_resource.h (right):

http://codereview.chromium.org/256022/diff/3003/9099#newcode24
Line 24: // Might hit filesystem. Do not call on UI thread!
On 2009/10/07 00:40:24, Nebojša Ćirić wrote:
> On 2009/10/06 23:09:05, Aaron Boodman wrote:
> > Can you put some stars and capital letters around this :).
> 
> Done.

Perfect, thanks :)

http://codereview.chromium.org/256022/diff/11002/12004
File chrome/browser/extensions/extension_l10n_util_unittest.cc (right):

http://codereview.chromium.org/256022/diff/11002/12004#newcode147
Line 147: bool PathsAreEqual(const FilePath& path, const FilePath& result) {
You didn't change this second argument name to path2.