Added new fileBrowserPrivate and fileHandler extension APIs

New fileBrowserPrivate and fileHandler APIs added + plus magic needed to safely hand over access to local file system elements from content extension to 3rd party extension.

As agreed with aa@ and asargent@, this new API defines following event:

chrome.fileHandler.onExecute.addListener(function(id, file_entries) {
}

This event is invoked when user selects files in ChromeOS file browser.

The extension needs to register itself as file content hanlder with following manifest changes:
...
"file_browser_actions": [
{
"id" : "ActionIdentifier",
"default_title" : "Action title",
"default_icon" : "icon.png",
"file_filters" : [ "filesystem:*.jpeg", ... ]
}
...
],
...



BUG=chromium-os:11996
TEST=ExtensionApiTest.FileBrowserTest, ExtensionManifestTest.FileBrowserActions 
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81869

[zel, 2011-03-31 03:57:07]

Let's get an early start on this review since it's pretty big. I am about 95%
done here, and will complete the missing items by EOD tomorrow.

[ericu, 2011-03-31 21:31:05]

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:410: return
file_url.append(MakeFileHash(file_url, extension_id));
So this will look like
filesystem:chrome-extension://{id}/local/path/to/file.html{hash} ?

I'm not clear on what the part that got taken out looked like.  What was the
original origin?

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.h (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:22: protected:
Duplicate label protected.

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:59: // context of
given extension.
How?  What is the HMAC intended to validate?

Adam Barth has been doing all the security screening for the filesystem URL
stuff; you might run this by him.

http://codereview.chromium.org/6749021/diff/19004/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4520: "description": "Represents
information about avaialble browser tasks.",
typo: available

http://codereview.chromium.org/6749021/diff/19004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4567: "description": "The list
of avalable tasks that can be performed on all of the passed file paths."
typo: available

http://codereview.chromium.org/6749021/diff/19004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4597: "description": "True of
task executution was successfully initiated."
typo: execution

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:681: if (resp[0].error)
{
Won't this throw if !response?  Is that what you want?

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:686: // Get locak file
system instance.
typo: local

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:698:
fs.root.getDirectory(file_path, {create: true},
I think we don't want to create the directory here.  If I ask for /foo.txt, and
it's not there yet [or has been deleted], that's going to make a funny
directory.

[zel, 2011-04-01 03:01:35]

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:410: return
file_url.append(MakeFileHash(file_url, extension_id));
On 2011/03/31 21:31:06, ericu wrote:
> So this will look like
> filesystem:chrome-extension://{id}/local/path/to/file.html{hash} ?
> 
> I'm not clear on what the part that got taken out looked like.  What was the
> original origin?

Yes, what I took out was the ID of the extension which initiated this API call
and replace it with the destination extension. Hash code ensures that the
destination extension is the one that can read this file url at the end.

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.h (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:22: protected:
On 2011/03/31 21:31:06, ericu wrote:
> Duplicate label protected.

Done.

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:59: // context of
given extension.
On 2011/03/31 21:31:06, ericu wrote:
> How?  What is the HMAC intended to validate?
> 
> Adam Barth has been doing all the security screening for the filesystem URL
> stuff; you might run this by him.

I've added Adam and implemented the HMAC at this point. Please see the comments
in the newly uploaded file now.

http://codereview.chromium.org/6749021/diff/19004/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4520: "description": "Represents
information about avaialble browser tasks.",
On 2011/03/31 21:31:06, ericu wrote:
> typo: available

Done.

http://codereview.chromium.org/6749021/diff/19004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4567: "description": "The list
of avalable tasks that can be performed on all of the passed file paths."
On 2011/03/31 21:31:06, ericu wrote:
> typo: available

Done.

http://codereview.chromium.org/6749021/diff/19004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4597: "description": "True of
task executution was successfully initiated."
On 2011/03/31 21:31:06, ericu wrote:
> typo: execution

Done.

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:681: if (resp[0].error)
{
On 2011/03/31 21:31:06, ericu wrote:
> Won't this throw if !response?  Is that what you want?

I am not sure that we want to throw here since this is callback and there would
be no one to catch this.

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:698:
fs.root.getDirectory(file_path, {create: true},
On 2011/03/31 21:31:06, ericu wrote:
> I think we don't want to create the directory here.  If I ask for /foo.txt,
and
> it's not there yet [or has been deleted], that's going to make a funny
> directory.

Whooops! Good catch. Done.

[zel, 2011-04-01 03:01:56]

+abarth

[ericu, 2011-04-02 01:16:05]

I'll leave most of the reviewing to security and extension folks at this point,
but here are a few little nits.

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:410: return
file_url.append(MakeFileHash(file_url, extension_id));
On 2011/04/01 03:01:35, zel wrote:
> On 2011/03/31 21:31:06, ericu wrote:
> > So this will look like
> > filesystem:chrome-extension://{id}/local/path/to/file.html{hash} ?
> > 
> > I'm not clear on what the part that got taken out looked like.  What was the
> > original origin?
> 
> Yes, what I took out was the ID of the extension which initiated this API call
> and replace it with the destination extension. Hash code ensures that the
> destination extension is the one that can read this file url at the end.  

OK, that makes sense.  Would be nice to see a little more detail in the comments
for MakeSafeFileUrl.  Perhaps "remove the id of the file browser extension and
replace it with that of the handler extension we're invoking" or something like
that.

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:681: if (resp[0].error)
{
On 2011/04/01 03:01:35, zel wrote:
> On 2011/03/31 21:31:06, ericu wrote:
> > Won't this throw if !response?  Is that what you want?
> 
> I am not sure that we want to throw here since this is callback and there
would
> be no one to catch this. 

Then you don't want to set resp to [] if !response.  Line 682 will dereference
null and throw.

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.h (right):

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:56: // context of
targeted extension. It does that by replacing soruce extension
typo: source

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:57: // id within
the utl and attaching a hash to the url. The hash is used as
typo: url

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:62: // extension
only during the lifetime of the current browser process.
It's still not at all clear what the hash is intended to guarantee.  What secret
piece of information is being transmitted and verified, or what changes are you
preventing people from making?

Ah, I see from the code what's happening, but the comments here should still
explain it.  What I was missing is the fact that the hash uses a secret, and
acts as a signature on the whole URL [including extension ID], so only we can
create a valid URL and no extension can use another extension's URL.

[asargent_no_longer_on_chrome, 2011-04-04 18:21:25]

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/tab_contents...
File chrome/browser/tab_contents/context_menu_utils.h (right):

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/tab_contents...
chrome/browser/tab_contents/context_menu_utils.h:28: private:
nit: I think you need a space before the "private:" line

http://codereview.chromium.org/6749021/diff/30004/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6749021/diff/30004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4556: "description": "An
DOMFileSystem instance for local file system access. null if the caller has no
appropriate permissions."
nit: "An" -> "A"

http://codereview.chromium.org/6749021/diff/30004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4564: "description": "Requests
access to local file system",
nit: looks like this description was cut-and-pasted from requestLocalFileSystem
above, and isn't quite accurate. 

Also, it would be great if you could add a description of what a task is either
here or in the description for the FileBrowserTask type above. When I first saw
the term "task", I was thinking it meant something like a running process, not
an available action (or intent) as you described in your email).

[zel, 2011-04-06 00:06:32]

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:410: return
file_url.append(MakeFileHash(file_url, extension_id));
On 2011/04/02 01:16:05, ericu wrote:
> On 2011/04/01 03:01:35, zel wrote:
> > On 2011/03/31 21:31:06, ericu wrote:
> > > So this will look like
> > > filesystem:chrome-extension://{id}/local/path/to/file.html{hash} ?
> > > 
> > > I'm not clear on what the part that got taken out looked like.  What was
the
> > > original origin?
> > 
> > Yes, what I took out was the ID of the extension which initiated this API
call
> > and replace it with the destination extension. Hash code ensures that the
> > destination extension is the one that can read this file url at the end.  
> 
> OK, that makes sense.  Would be nice to see a little more detail in the
comments
> for MakeSafeFileUrl.  Perhaps "remove the id of the file browser extension and
> replace it with that of the handler extension we're invoking" or something
like
> that.

Done.

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:681: if (resp[0].error)
{
On 2011/04/02 01:16:05, ericu wrote:
> On 2011/04/01 03:01:35, zel wrote:
> > On 2011/03/31 21:31:06, ericu wrote:
> > > Won't this throw if !response?  Is that what you want?
> > 
> > I am not sure that we want to throw here since this is callback and there
> would
> > be no one to catch this. 
> 
> Then you don't want to set resp to [] if !response.  Line 682 will dereference
> null and throw.

Done.

http://codereview.chromium.org/6749021/diff/19004/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:686: // Get locak file
system instance.
On 2011/03/31 21:31:06, ericu wrote:
> typo: local

Done.

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.h (right):

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:56: // context of
targeted extension. It does that by replacing soruce extension
On 2011/04/02 01:16:05, ericu wrote:
> typo: source

Done.

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:57: // id within
the utl and attaching a hash to the url. The hash is used as
On 2011/04/02 01:16:05, ericu wrote:
> typo: url

Done.

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:62: // extension
only during the lifetime of the current browser process.
On 2011/04/02 01:16:05, ericu wrote:
> It's still not at all clear what the hash is intended to guarantee.  What
secret
> piece of information is being transmitted and verified, or what changes are
you
> preventing people from making?
> 
> Ah, I see from the code what's happening, but the comments here should still
> explain it.  What I was missing is the fact that the hash uses a secret, and
> acts as a signature on the whole URL [including extension ID], so only we can
> create a valid URL and no extension can use another extension's URL.

Done.

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/tab_contents...
File chrome/browser/tab_contents/context_menu_utils.h (right):

http://codereview.chromium.org/6749021/diff/30004/chrome/browser/tab_contents...
chrome/browser/tab_contents/context_menu_utils.h:28: private:
On 2011/04/04 18:21:25, Antony Sargent wrote:
> nit: I think you need a space before the "private:" line

Done.

http://codereview.chromium.org/6749021/diff/30004/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6749021/diff/30004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4556: "description": "An
DOMFileSystem instance for local file system access. null if the caller has no
appropriate permissions."
On 2011/04/04 18:21:25, Antony Sargent wrote:
> nit: "An" -> "A"

Done.

http://codereview.chromium.org/6749021/diff/30004/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4564: "description": "Requests
access to local file system",
On 2011/04/04 18:21:25, Antony Sargent wrote:
> nit: looks like this description was cut-and-pasted from
requestLocalFileSystem
> above, and isn't quite accurate. 
> 
> Also, it would be great if you could add a description of what a task is
either
> here or in the description for the FileBrowserTask type above. When I first
saw
> the term "task", I was thinking it meant something like a running process, not
> an available action (or intent) as you described in your email). 

Done.

[abarth-chromium, 2011-04-06 02:17:32]

This patch is too large for my puny little brain.  From what I can tell, the
approach is ok, but you really shouldn't be using string manipulation functions
to manipulate URLs.  GURL has many nice URL manipulation functions that will
help you avoid bugs.

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:218: 256));
Should 256 be a named constant?

Why are we using an AES key for a SHA1 HMAC?

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:316:
std::string::size_type pos = file_url_.find('#');
You're storing something magical in the fragment of the URL?  Why are you using
string manipulation functions on URLs?  You should use GURL to find the fragment
for you.

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:563:
std::string::size_type pos = origin_file_url.find("/local/");
What about a URL like the following:

filesystem:http://example.com/temporary/foo/bar/local/baz/taco.jpg

?

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:573: file_url =
file_url.append("#");
What if the file_url already has a fragment?  You shouldn't use string
manipulations on URLs.  You should use GURL.

http://codereview.chromium.org/6749021/diff/40001/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/6749021/diff/40001/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:101: console.log(error);
Do we want to land this code with console.log in it?

[abarth-chromium, 2011-04-06 02:46:36]

BTW, how does this interact with directories (i.e., if the URL is the URL of a
directory)?

[ericu, 2011-04-06 03:50:08]

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.h (right):

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:59: // this
routine attached to it. The hash ensures that generated filesystem:
The sentence that starts with "Then it generates" is probably missing a word or
something--I can't read it.

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:60: // url can be
safely used within the context of the handler extension and
What the hash does is allow us to validate that the URL, including the domain,
hasn't been changed, and was generated by this code, not an attacker, since it
contains a randomly-generated secret.  Please say that instead of "ensures that
the url can be used safely" or the like.

[abarth-chromium, 2011-04-06 03:58:41]

Is there some reason we're using crypto instead of just keeping a
hashtable of these URL / origin mappings in memory?  (Sorry if this
has been discussed already.)

Adam


On Tue, Apr 5, 2011 at 7:46 PM,  <abarth@chromium.org> wrote:
> BTW, how does this interact with directories (i.e., if the URL is the URL of
> a
> directory)?
>
> http://codereview.chromium.org/6749021/
>

[zel, 2011-04-06 04:33:17]

The DirectoryEntry will work to a certain degree. The caller extension
should be able to mess with its sub-Entries and even read their blob
content. It won't be able to refer to any one of them directly by their
fileschema: URL - i.e. chrome.fileSystem.resolveFileSystemURL() call will
fail for sub-Entry URLs since ext won't have the proper entry point hash for
them.



On Tue, Apr 5, 2011 at 7:46 PM, <abarth@chromium.org> wrote:

> BTW, how does this interact with directories (i.e., if the URL is the URL
> of a
> directory)?
>
>
> http://codereview.chromium.org/6749021/
>

[abarth-chromium, 2011-04-06 04:50:09]

What sort of directory traversal exists in DirectoryEntry?  Can I get
parent directory entries?  Can I get child entries?  Does this
mechanism impose any constraints on the future evolution of
DirectoryEntry?

Adam


On Tue, Apr 5, 2011 at 9:33 PM, Zelidrag Hornung <zelidrag@chromium.org> wrote:
> The DirectoryEntry will work to a certain degree. The caller extension
> should be able to mess with its sub-Entries and even read their blob
> content. It won't be able to refer to any one of them directly by their
> fileschema: URL - i.e. chrome.fileSystem.resolveFileSystemURL() call will
> fail for sub-Entry URLs since ext won't have the proper entry point hash for
> them.
>
>
> On Tue, Apr 5, 2011 at 7:46 PM, <abarth@chromium.org> wrote:
>>
>> BTW, how does this interact with directories (i.e., if the URL is the URL
>> of a
>> directory)?
>>
>> http://codereview.chromium.org/6749021/
>
>

[zelidrag1, 2011-04-06 05:14:26]

Yes, there are mechanism for directory traversal for the entire exposed file
system instance. The lack of explicit file access rights
in ChildProcessSecurityPolicy will prevent file content retrieval though.

I think that I need to wire my local file system provider path lookups with
permissions that are set in ChildProcessSecurityPolicy to ensure that
traversals by non-component extension are scoped down to selected entities
and prevent walking the tree upward.

If that is in place, then ChildProcessSecurityPolicy can then simply be that
in-memory access map that you were talking about and we can maybe eliminate
the need for this hash business altogether?


On Tue, Apr 5, 2011 at 9:49 PM, Adam Barth <abarth@chromium.org> wrote:

> What sort of directory traversal exists in DirectoryEntry?  Can I get
> parent directory entries?  Can I get child entries?  Does this
> mechanism impose any constraints on the future evolution of
> DirectoryEntry?
>
> Adam
>
>
> On Tue, Apr 5, 2011 at 9:33 PM, Zelidrag Hornung <zelidrag@chromium.org>
> wrote:
> > The DirectoryEntry will work to a certain degree. The caller extension
> > should be able to mess with its sub-Entries and even read their blob
> > content. It won't be able to refer to any one of them directly by their
> > fileschema: URL - i.e. chrome.fileSystem.resolveFileSystemURL() call will
> > fail for sub-Entry URLs since ext won't have the proper entry point hash
> for
> > them.
> >
> >
> > On Tue, Apr 5, 2011 at 7:46 PM, <abarth@chromium.org> wrote:
> >>
> >> BTW, how does this interact with directories (i.e., if the URL is the
> URL
> >> of a
> >> directory)?
> >>
> >> http://codereview.chromium.org/6749021/
> >
> >
>

[zel, 2011-04-06 05:25:17]

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:218: 256));
On 2011/04/06 02:17:32, abarth wrote:
> Should 256 be a named constant?
> 
> Why are we using an AES key for a SHA1 HMAC?

Added constant kAESKeyLengthInBits.

symetric_key_nss.cc:20 only implements AES, that's why. Since that random AES
key is longer than SHA1 hash, it's still a good as secret key for this purpose.

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:316:
std::string::size_type pos = file_url_.find('#');
On 2011/04/06 02:17:32, abarth wrote:
> You're storing something magical in the fragment of the URL?  Why are you
using
> string manipulation functions on URLs?  You should use GURL to find the
fragment
> for you.

I tried GURL. It does not like filesystem: and does not really find anything
useful here. I tried to use it as much as I could here.

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:563:
std::string::size_type pos = origin_file_url.find("/local/");
On 2011/04/06 02:17:32, abarth wrote:
> What about a URL like the following:
> 
> filesystem:http://example.com/temporary/foo/bar/local/baz/taco.jpg
> 
> ?

I used GURL as much as I could. This class is not very happy with fileschema:
parsing business.

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:573: file_url =
file_url.append("#");
On 2011/04/06 02:17:32, abarth wrote:
> What if the file_url already has a fragment?  You shouldn't use string
> manipulations on URLs.  You should use GURL.

Same as above.

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.h (right):

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:59: // this
routine attached to it. The hash ensures that generated filesystem:
On 2011/04/06 03:50:09, ericu wrote:
> The sentence that starts with "Then it generates" is probably missing a word
or
> something--I can't read it.

Done.

http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:60: // url can be
safely used within the context of the handler extension and
On 2011/04/06 03:50:09, ericu wrote:
> What the hash does is allow us to validate that the URL, including the domain,
> hasn't been changed, and was generated by this code, not an attacker, since it
> contains a randomly-generated secret.  Please say that instead of "ensures
that
> the url can be used safely" or the like.

Done.

http://codereview.chromium.org/6749021/diff/40001/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/6749021/diff/40001/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:101: console.log(error);
this was here just for my debugging, it's been removed now. done.

[abarth-chromium, 2011-04-06 05:50:26]

On Tue, Apr 5, 2011 at 10:14 PM, Zelidrag Hornung <zelidrag@google.com> wrote:
> Yes, there are mechanism for directory traversal for the entire exposed file
> system instance. The lack of explicit file access rights
> in ChildProcessSecurityPolicy will prevent file content retrieval though.

That's not the job of ChildProcessSecurityPolicy.  It enforces access
control on a per-process basis, which is useful for stopping
compromised renderers from attacking the operating system.  It's not
fine-grained enough to provide web-level security (like we need here).

> I think that I need to wire my local file system provider path lookups with
> permissions that are set in ChildProcessSecurityPolicy to ensure that
> traversals by non-component extension are scoped down to selected entities
> and prevent walking the tree upward.

We probably need some integration with ChildProcessSecurityPolicy to
deal with the compromised renderer case, but that's different from
what you're doing in this patch.

> If that is in place, then ChildProcessSecurityPolicy can then simply be that
> in-memory access map that you were talking about and we can maybe eliminate
> the need for this hash business altogether?

Nope.  ChildProcessSecurityPolicy has a map from process ID to
privileges.  You need a map from origin to privileges, which is
finer-grained.

Adam


> On Tue, Apr 5, 2011 at 9:49 PM, Adam Barth <abarth@chromium.org> wrote:
>>
>> What sort of directory traversal exists in DirectoryEntry?  Can I get
>> parent directory entries?  Can I get child entries?  Does this
>> mechanism impose any constraints on the future evolution of
>> DirectoryEntry?
>>
>> Adam
>>
>>
>> On Tue, Apr 5, 2011 at 9:33 PM, Zelidrag Hornung <zelidrag@chromium.org>
>> wrote:
>> > The DirectoryEntry will work to a certain degree. The caller extension
>> > should be able to mess with its sub-Entries and even read their blob
>> > content. It won't be able to refer to any one of them directly by their
>> > fileschema: URL - i.e. chrome.fileSystem.resolveFileSystemURL() call
>> > will
>> > fail for sub-Entry URLs since ext won't have the proper entry point hash
>> > for
>> > them.
>> >
>> >
>> > On Tue, Apr 5, 2011 at 7:46 PM, <abarth@chromium.org> wrote:
>> >>
>> >> BTW, how does this interact with directories (i.e., if the URL is the
>> >> URL
>> >> of a
>> >> directory)?
>> >>
>> >> http://codereview.chromium.org/6749021/
>> >
>> >
>
>

[abarth-chromium, 2011-04-06 05:53:22]

On Tue, Apr 5, 2011 at 10:25 PM,  <zelidrag@chromium.org> wrote:
>
>
http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_file_browser_private_api.cc
> (right):
>
>
http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:218:
> 256));
> On 2011/04/06 02:17:32, abarth wrote:
>>
>> Should 256 be a named constant?
>
>> Why are we using an AES key for a SHA1 HMAC?
>
> Added constant kAESKeyLengthInBits.
>
> symetric_key_nss.cc:20 only implements AES, that's why. Since that
> random AES key is longer than SHA1 hash, it's still a good as secret key
> for this purpose.

Sounds like we should create a new SHA1 HMAC key class.  We shouldn't
be using AES keys for SHA1-HMAC.

>
http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:316:
> std::string::size_type pos = file_url_.find('#');
> On 2011/04/06 02:17:32, abarth wrote:
>>
>> You're storing something magical in the fragment of the URL?  Why are
>
> you using
>>
>> string manipulation functions on URLs?  You should use GURL to find
>
> the fragment
>>
>> for you.
>
> I tried GURL. It does not like filesystem: and does not really find
> anything useful here. I tried to use it as much as I could here.

Maybe we need to teach GURL how to handle filesystem URLs?  Maybe we
need a FilesystemURL class that understands how to crack open
filesystem URLs and feed the guts of the URL to GURL?  Doing this
working with string manipulation is error prone and going to lead to
security bugs.

>
http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:563:
> std::string::size_type pos = origin_file_url.find("/local/");
> On 2011/04/06 02:17:32, abarth wrote:
>>
>> What about a URL like the following:
>
>> filesystem:http://example.com/temporary/foo/bar/local/baz/taco.jpg
>
>> ?
>
> I used GURL as much as I could. This class is not very happy with
> fileschema: parsing business.

That just means we need to make the infrastructure smarter, not that
we should revert to stone knives and bears skins.

>
http://codereview.chromium.org/6749021/diff/40001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:573:
> file_url = file_url.append("#");
> On 2011/04/06 02:17:32, abarth wrote:
>>
>> What if the file_url already has a fragment?  You shouldn't use string
>> manipulations on URLs.  You should use GURL.
>
> Same as above.

Regardless, this code still has a bug, right?

Adam

[ericu, 2011-04-06 17:32:30]

On Tue, Apr 5, 2011 at 10:14 PM, Zelidrag Hornung <zelidrag@google.com> wrote:
> Yes, there are mechanism for directory traversal for the entire exposed file
> system instance. The lack of explicit file access rights
> in ChildProcessSecurityPolicy will prevent file content retrieval though.
> I think that I need to wire my local file system provider path lookups with
> permissions that are set in ChildProcessSecurityPolicy to ensure that
> traversals by non-component extension are scoped down to selected entities
> and prevent walking the tree upward.

This sounds like what we discussed: a file that's been given to an
extension via context menu should be an orphan.  You shouldn't be able
to see its path, list its parent directory, etc.  That's going to take
a bit more code than you've got here, especially if you want to allow
these handlers to access files in the sandbox as well as the rest of
the disk.  I'm not really sure what you want to do about directories,
though--my guess would be that you'd want to allow access to all the
contents of the directory all the way down, and the directory itself
[perhaps to rename it?], but not its parent, path, or siblings.

> If that is in place, then ChildProcessSecurityPolicy can then simply be that
> in-memory access map that you were talking about and we can maybe eliminate
> the need for this hash business altogether?
>
> On Tue, Apr 5, 2011 at 9:49 PM, Adam Barth <abarth@chromium.org> wrote:
>>
>> What sort of directory traversal exists in DirectoryEntry?  Can I get
>> parent directory entries?  Can I get child entries?  Does this
>> mechanism impose any constraints on the future evolution of
>> DirectoryEntry?
>>
>> Adam
>>
>>
>> On Tue, Apr 5, 2011 at 9:33 PM, Zelidrag Hornung <zelidrag@chromium.org>
>> wrote:
>> > The DirectoryEntry will work to a certain degree. The caller extension
>> > should be able to mess with its sub-Entries and even read their blob
>> > content. It won't be able to refer to any one of them directly by their
>> > fileschema: URL - i.e. chrome.fileSystem.resolveFileSystemURL() call
>> > will
>> > fail for sub-Entry URLs since ext won't have the proper entry point hash
>> > for
>> > them.
>> >
>> >
>> > On Tue, Apr 5, 2011 at 7:46 PM, <abarth@chromium.org> wrote:
>> >>
>> >> BTW, how does this interact with directories (i.e., if the URL is the
>> >> URL
>> >> of a
>> >> directory)?
>> >>
>> >> http://codereview.chromium.org/6749021/
>> >
>> >
>
>

[zel, 2011-04-06 21:28:29]

PTAL

I have completely removed hash business and went with Adam's suggestion to keep
the track of permissions in memory instead. This also solves file system upward
traversal problems.

[ericu, 2011-04-07 01:40:58]

The new security model looks like it deals with directories properly.

It looks to me like the receiving extension will always get the path of the
file, but that it'll be the virtual path, rooted at the mount point.  Is that
right, and is that OK?

http://codereview.chromium.org/6749021/diff/46043/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/46043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:195: class
FileHashGenerator {
I thought you'd completely removed the hash stuff...is this dead code?

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:130: const FilePath&
virtual_path) {
s/virtual_path/unused, here.

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.h (right):

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:36: const FilePath& virtual_path)
{}
Should this ever get called?  How about a NOTREACHED?

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:69: return 
std::vector<FilePath>();
Same here.  Perhaps these shouldn't really live on the superclass, but should
instead be on your specific subclass?

[zel, 2011-04-07 02:54:42]

Yes, the handler extension will always get the full filesystem URL, even though
it might not be able to navigate upward from it. I see no big issues with that
since the root of its real system file path is still masked there.

http://codereview.chromium.org/6749021/diff/46043/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/46043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:195: class
FileHashGenerator {
On 2011/04/07 01:40:58, ericu wrote:
> I thought you'd completely removed the hash stuff...is this dead code?

Yes, it's removed now.

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:130: const FilePath&
virtual_path) {
On 2011/04/07 01:40:58, ericu wrote:
> s/virtual_path/unused, here.

Done.

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.h (right):

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:36: const FilePath& virtual_path)
{}
On 2011/04/07 01:40:58, ericu wrote:
> Should this ever get called?  How about a NOTREACHED?

Done.

http://codereview.chromium.org/6749021/diff/46043/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.h:69: return 
std::vector<FilePath>();
On 2011/04/07 01:40:58, ericu wrote:
> Same here.  Perhaps these shouldn't really live on the superclass, but should
> instead be on your specific subclass?

I've added NOTREACHED() here. Done.

[abarth-chromium, 2011-04-07 23:09:20]

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:37: const char
kInvalidFileUrl[] = "Invalid file URL";
Does these error messages need to be localized?

[zel, 2011-04-07 23:11:00]

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:37: const char
kInvalidFileUrl[] = "Invalid file URL";
On 2011/04/07 23:09:20, abarth wrote:
> Does these error messages need to be localized?

From what I've seen in other API examples, we didn't localize any extension API
error message.

[abarth-chromium, 2011-04-07 23:14:56]

My basic opinion is that this patch is too large to review accurately.  I'm sure
there are bugs here, but I doubt that I'm going to find them by staring at the
code anymore.

In general, the code quality could be improve by better separation of concerns. 
For example, there's still a bunch of printf manipulation of file URLs. 
extension_file_browser_private_api.cc is very unlikely to be the correct place
to perform those manipulations.  Instead, they should likely be abstracted into
a lower-layer class that understand how to manipulate filesystem URLs.

Short of asking you to break this patch into smaller, reviewable pieces, I'm not
sure there's much more I can contribute to the quality of this change. 
Obviously, you need tests, but it's unclear to me whether you've designed your
code with testing in mind given that the tests are coming 10 iterations into the
patch.

If you submitted this patch to WebKit, it would be unlikely to be accepted in
this form.  However, I understand that Chromium is a different project from
WebKit and there are different aesthetics.  Please don't let me foreign
aesthetic get in the way of making progress here.

[zel, 2011-04-07 23:23:45]

I can address URL construction concerns if someone here can point me which
class would be an appropriate place for that purpose. Other than EntryBase
implementation (on WK side), I didn't see anything else that constructs file
URLs on chromium side.

The extension API tests are going to be added to this CL by EOD.

Btw, I see no good way how to split this into different CLs and still have
this feature working across extension API surface.


On Thu, Apr 7, 2011 at 4:14 PM, <abarth@chromium.org> wrote:

> My basic opinion is that this patch is too large to review accurately.  I'm
> sure
> there are bugs here, but I doubt that I'm going to find them by staring at
> the
> code anymore.
>
> In general, the code quality could be improve by better separation of
> concerns.
> For example, there's still a bunch of printf manipulation of file URLs.
> extension_file_browser_private_api.cc is very unlikely to be the correct
> place
> to perform those manipulations.  Instead, they should likely be abstracted
> into
> a lower-layer class that understand how to manipulate filesystem URLs.
>
> Short of asking you to break this patch into smaller, reviewable pieces,
> I'm not
> sure there's much more I can contribute to the quality of this change.
> Obviously, you need tests, but it's unclear to me whether you've designed
> your
> code with testing in mind given that the tests are coming 10 iterations
> into the
> patch.
>
> If you submitted this patch to WebKit, it would be unlikely to be accepted
> in
> this form.  However, I understand that Chromium is a different project from
> WebKit and there are different aesthetics.  Please don't let me foreign
> aesthetic get in the way of making progress here.
>
>
> http://codereview.chromium.org/6749021/
>

[ericu, 2011-04-08 02:12:14]

The filesystem stuff looks pretty good now; I look forward to seeing the tests.
I'll leave review of the extension-side stuff to others.

However, I sympathize with Adam's point of view that this CL is just too big,
and it's only going to grow as the tests go in.  You could cut a big chunk of
the the meat of it out if you put all the filesystem stuff in one CL, then added
the extension APIs that let you access it in a second.

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:282: // Make
sure this url really used by the right caller extension.
s/really/is really being/ ?

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:290:
FilePath(virtual_path),
FilePath constructor is redundant.

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:55: CachedResults
cached_resuts_;
typo: results

http://codereview.chromium.org/6749021/diff/49001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/6749021/diff/49001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:27: // Checks if access to
|virtual_path| is allowed from |origin_url|.
Copy/paste of comment.

http://codereview.chromium.org/6749021/diff/49001/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/6749021/diff/49001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:222: return 
std::vector<FilePath>();
Please add a TODO(ericu) to implement this and use it for access permissions as
you have.  We currently have another mechanism, but we should switch over.

[zel, 2011-04-08 04:16:36]

Eric, I have addressed your issues in following files. Please note that I have
also separated all File API related items to a new CL to ease the review process
of these changes:

http://codereview.chromium.org/6810037/

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.h:55: CachedResults
cached_resuts_;
On 2011/04/08 02:12:14, ericu wrote:
> typo: results

Done.

http://codereview.chromium.org/6749021/diff/49001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_mount_point_provider.h (right):

http://codereview.chromium.org/6749021/diff/49001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_mount_point_provider.h:27: // Checks if access to
|virtual_path| is allowed from |origin_url|.
On 2011/04/08 02:12:14, ericu wrote:
> Copy/paste of comment.

Done.

http://codereview.chromium.org/6749021/diff/49001/webkit/fileapi/sandbox_moun...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

http://codereview.chromium.org/6749021/diff/49001/webkit/fileapi/sandbox_moun...
webkit/fileapi/sandbox_mount_point_provider.cc:222: return 
std::vector<FilePath>();
On 2011/04/08 02:12:14, ericu wrote:
> Please add a TODO(ericu) to implement this and use it for access permissions
as
> you have.  We currently have another mechanism, but we should switch over.

Done.

[zel, 2011-04-08 04:56:48]

The highlights of the latest snapshot are:

* I have added extension tests that execute and verify the entire context menu
handler business and file permissions working end-2-end

* File API changes have moved to http://codereview.chromium.org/6810037/. I
can't see what else I can split apart for here and still make CL testable to a
certain degree.

* I've fixed the remaining comments from Eric (see below).

* Now using legit methods to create fileapi: urls from extension API
implementation code.

PTAL

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:282: // Make
sure this url really used by the right caller extension.
On 2011/04/08 02:12:14, ericu wrote:
> s/really/is really being/ ?

Done.

http://codereview.chromium.org/6749021/diff/49001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:290:
FilePath(virtual_path),
On 2011/04/08 02:12:14, ericu wrote:
> FilePath constructor is redundant.

Done.

[olivia69xx, 2011-04-08 18:17:57]

http://www.erosad204574.htm

[olivia69xx, 2011-04-08 18:34:23]

http://www.olivia-bostongfe.escort-site.com

[asargent_no_longer_on_chrome, 2011-04-08 18:52:32]

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_context_menu_api.cc (right):

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_context_menu_api.cc:69:
tmp_result.Add(ExtensionMenuItem::FILE);
As a heads up, you'll get a merge conflict with
http://codereview.chromium.org/6799020/ which Mihai recently checked in. Also,
you'll need to add "file" to the enum he changed us to using in
extension_api.json in http://codereview.chromium.org/6801032/.

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:70: // the
extension's name.
I seem to remember we do something just like this - sorting by extension name -
in render_view_context_menu (maybe moved to your context_menu_utils.{h,cc}. Can
we put this into a method and share it?

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:83: 
It looks like you don't ever actually do any sorting of sorted_ids.

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:117: // that are
sahred
typo "sahred"

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:183: *action_id
= task_id.substr(pos_slash + 1);
This might be made clearer with a little use of Tokenize (from
base/string_util.h). 

E.g. 

std::vector<std::string> result;
int count = base::Tokenize(task_id, ":/");
if (count != 3)
  return false;
*task_type = result[0];
*target_extension_id = result[1];
*action_id = result[2];

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:293: FilePath
root_path = path_manager->GetFileSystemRootPathOnFileThread(
Is this method (SetupFileAccessPermissions) running on the file thread? If so
can you add a DCHECK about that at the start of the function?

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/tab_contents...
File chrome/browser/tab_contents/context_menu_utils.cc (right):

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/tab_contents...
chrome/browser/tab_contents/context_menu_utils.cc:13: 
When you merge from trunk, make sure you pick up Mihai's changes from
http://codereview.chromium.org/6799020/diff/5001/chrome/browser/tab_contents/...
into this code.

[zel, 2011-04-08 19:35:01]

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_context_menu_api.cc (right):

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_context_menu_api.cc:69:
tmp_result.Add(ExtensionMenuItem::FILE);
On 2011/04/08 18:52:33, Antony Sargent wrote:
> As a heads up, you'll get a merge conflict with
> http://codereview.chromium.org/6799020/ which Mihai recently checked in. Also,
> you'll need to add "file" to the enum he changed us to using in
> extension_api.json in http://codereview.chromium.org/6801032/.

I know. He added "frame" - it's just below. I have already resolved these
conflicts. I have added this to extension_api.json as well.

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:70: // the
extension's name.
On 2011/04/08 18:52:33, Antony Sargent wrote:
> I seem to remember we do something just like this - sorting by extension name
-
> in render_view_context_menu (maybe moved to your context_menu_utils.{h,cc}.
Can
> we put this into a method and share it?

Done.

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:83: 
On 2011/04/08 18:52:33, Antony Sargent wrote:
> It looks like you don't ever actually do any sorting of sorted_ids.

Done.

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:117: // that are
sahred
On 2011/04/08 18:52:33, Antony Sargent wrote:
> typo "sahred"

Done.

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:183: *action_id
= task_id.substr(pos_slash + 1);
On 2011/04/08 18:52:33, Antony Sargent wrote:
> This might be made clearer with a little use of Tokenize (from
> base/string_util.h). 
> 
> E.g. 
> 
> std::vector<std::string> result;
> int count = base::Tokenize(task_id, ":/");
> if (count != 3)
>   return false;
> *task_type = result[0];
> *target_extension_id = result[1];
> *action_id = result[2];

Done.

http://codereview.chromium.org/6749021/diff/42043/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:293: FilePath
root_path = path_manager->GetFileSystemRootPathOnFileThread(
On 2011/04/08 18:52:33, Antony Sargent wrote:
> Is this method (SetupFileAccessPermissions) running on the file thread? If so
> can you add a DCHECK about that at the start of the function?

It's called from DidOpenFileSystem(), I have file thread check right at the
beginning of that function.

[Aaron Boodman, 2011-04-08 22:51:04]

(responding to adam's concerns...)

Maybe now would be a good time to request a design doc for this set of features.
I have also been feeling overwhelmed by the amount of code I'm being asked to
review, and the lack of overall context.

It doesn't have to be fancy. Another developer went through this exercise
recently for a feature, and it was incredibly useful. Here is his document for
reference:
https://docs.google.com/a/google.com/document/d/1UsHPgW6XJ8hfdHnR39SbpQ41OleT....

Doing this allows reviewers to focus on the high level picture before diving
into implementation detail.

It also allows us to easily pull in other experts from around Chrome if we find
that it is necessary.

[aa, 2011-04-08 23:11:35]

Hm, some people reported the URL was broken:

https://docs.google.com/a/google.com/document/d/1UsHPgW6XJ8hfdHnR39SbpQ41OleT...

- a

On Fri, Apr 8, 2011 at 3:51 PM,  <aa@chromium.org> wrote:
> (responding to adam's concerns...)
>
> Maybe now would be a good time to request a design doc for this set of
> features.
> I have also been feeling overwhelmed by the amount of code I'm being asked
> to
> review, and the lack of overall context.
>
> It doesn't have to be fancy. Another developer went through this exercise
> recently for a feature, and it was incredibly useful. Here is his document
> for
> reference:
>
https://docs.google.com/a/google.com/document/d/1UsHPgW6XJ8hfdHnR39SbpQ41OleT....
>
> Doing this allows reviewers to focus on the high level picture before diving
> into implementation detail.
>
> It also allows us to easily pull in other experts from around Chrome if we
> find
> that it is necessary.
>
> http://codereview.chromium.org/6749021/
>

[Aaron Boodman, 2011-04-09 02:47:03]

I'm really sorry. I forgot that you did in fact send out a design doc a week
ago.

Since my concerns are really design related, I'll take them back to that thread.

[asargent_no_longer_on_chrome, 2011-04-12 21:09:38]

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_menu_manager.cc (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_menu_manager.cc:409:
properties->SetString("mediaType", "file");
I think you can remove this change, since you're dispatching a different event
on a different namespace now, right?

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4518: "namespace":"fileSystem",
would it make sense to call this namespace "fileBrowserActions" instead of
"fileSystem"?

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

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:2001: // Initialize browser action
(optional).
nit: comment should be "Initialize file browser actions"

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:2013: return false;  // Failed to parse
browser action definition.
same naming nit on comment here

[Aaron Boodman, 2011-04-12 22:47:21]

And you're still going to make the change to remove
chrome.fileSystem.resolveURL, right? (using the Event.dispatch() hack we
discussed privately).

http://codereview.chromium.org/6749021/diff/57045/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/57045/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:316: #ifndef
NDEBUG
We have a DEBUG symbol too :)

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:60: 
Consider defining a struct. It is a bit more readable than std::pair.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:117: return
false;
indent--

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:133:
common_tasks->insert(common_tasks->begin(),
indent--

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:142:
ExtensionMenuItem::List::iterator intersection_end =
Move this line onto previous?

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:148:
common_tasks->clear();
Is the only reason for named_action_list so that you can sort? Because an
alternate would be to define a quick comparator that compares the titles of the
extension actions.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:243:
BrowserThread::PostTask(
You can also just pass Extension* into LocalFileSystemCallbackDispatcher().
ExtensionFunctions have a GetExtension() getter.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:255: if
(!service)
Unnecessary whitespace.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:309: const
Extension* extension = GetExtension();
Same question with this as in other places.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:405:
SendResponse(false);
You can avoid the cast with:

ListValue* result_list = new ListValue();
result_.reset(result_list);

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:417: if
(!FindCommonTasks(profile_, source_url_, files_list, &common_tasks))
This should probably be a CHECK. Is there any legit way that you can think of
that this extension wouldn't be there?

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:509:
path_manager->local_provider();
ExtensionFunction has a GetExtension() method.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:743:
dict->SetString("SELECT_OPEN_FILE_TITLE",
Unnecessary newline?

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.cc:205: // fileSystem
functions.
You should only register these functions on Chrome OS. (Ideally, you should only
build them on Chrome OS too).

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4518: "namespace":"fileSystem",
On 2011/04/12 21:09:38, Antony Sargent wrote:
> would it make sense to call this namespace "fileBrowserActions" instead of
> "fileSystem"? 

If you parse the name "fileBrowserActions" as "(file browser) actions", it
actually makes a lot of sense. But most people with familiarity with the
extension system will parse it as "file (browser actions)" and be confused.

How about fileBrowserHandler to remove this ambiguity?

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4558: "name": "fileUrls",
How about "entryURLs", since they can be either files or directories.

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

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:905: const DictionaryValue*
file_browser_actions, std::string* error) {
s/file_browser_actions/file_browser_action/

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:912: if
(!file_browser_actions->HasKey(keys::kPageActionId) ||
Can you name this property "name" instead? (We don't use the old support for
page action IDs anymore, so there's no precedent, and I think name is better).

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:914: *error =
errors::kInvalidPageActionId;
Incorrect error message.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:921: if
(!file_browser_actions->HasKey(keys::kPageActionDefaultTitle) ||
It's a bit weird to reuse these constants (pageActionDefaultTitle), but OK.

I do like the idea of using "default_title" and "default_icon", even if we don't
have any way to change them currently, because if we do add that, it will make
sense.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:929: ListValue* list_value;
= NULL

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:947: URLPattern::PARSE_LENIENT)) {
Use PARSE_STRICT and remove the TODO(skerner). The other one was only lenient
for legacy reasons.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:956: // Read the page action
|default_icon| (optional).
paste-o ('page action')

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:959: &default_icon) ||
intent+=4

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:2084: &&
!CommandLine::ForCurrentProcess()->HasSwitch(
Is this testing code? Can we remove it?

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:2686: ||
CommandLine::ForCurrentProcess()->HasSwitch(
Is this testing code? Can we remove it?

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.h:644: const ListValue* extension_actions,
std::string* error);
Similarly to my comment in extension_api.json, the term 'extension action' has
an existing meaning in this codebase, so I'd prefer 'handler' here.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
File chrome/common/extensions/file_browser_action.h (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_action.h:18: // ExtensionAction
encapsulates the state of a browser or page action.
Comment needs updating.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_action.h:21: class FileBrowserAction {
Did you look at just using ExtensionAction directly? It has more features than
you need, but you don't need to use those.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_action.h:39: const std::string&
default_title() const { return title_; }
s/default_// ?

http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (left):

http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:100: 
Unnecessary change.

http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:662:
apiFunctions["fileSystem.resolveLocalFileSystemURL"].customCallback =
I think you are planning to remove this soon?

http://codereview.chromium.org/6749021/diff/61014/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/6749021/diff/61014/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
Unnecessary change.

[Aaron Boodman, 2011-04-12 22:48:20]

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

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:912: if
(!file_browser_actions->HasKey(keys::kPageActionId) ||
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Can you name this property "name" instead? (We don't use the old support for
> page action IDs anymore, so there's no precedent, and I think name is better).

Whoops, I didn't mean to include this comment. I thought about it some more and
worried "name" might get confused with "title", so id is fine.

[zel, 2011-04-12 23:09:10]

Yes, I am working on removal of chrome.fileSystem.resolveURL right now. Will
be ready in 1-2 hours.


On Tue, Apr 12, 2011 at 3:47 PM, <aa@chromium.org> wrote:

> And you're still going to make the change to remove
> chrome.fileSystem.resolveURL, right? (using the Event.dispatch() hack we
> discussed privately).
>
>
>
>
http://codereview.chromium.org/6749021/diff/57045/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_file_browser_private_api.cc
> (right):
>
>
>
http://codereview.chromium.org/6749021/diff/57045/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:316:
> #ifndef NDEBUG
> We have a DEBUG symbol too :)
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:60:
> Consider defining a struct. It is a bit more readable than std::pair.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
>
> chrome/browser/extensions/extension_file_browser_private_api.cc:117:
> return false;
> indent--
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:133:
> common_tasks->insert(common_tasks->begin(),
> indent--
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:142:
> ExtensionMenuItem::List::iterator intersection_end =
> Move this line onto previous?
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:148:
> common_tasks->clear();
> Is the only reason for named_action_list so that you can sort? Because
> an alternate would be to define a quick comparator that compares the
> titles of the extension actions.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:243:
> BrowserThread::PostTask(
> You can also just pass Extension* into
> LocalFileSystemCallbackDispatcher(). ExtensionFunctions have a
> GetExtension() getter.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:255: if
> (!service)
> Unnecessary whitespace.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:309:
> const Extension* extension = GetExtension();
> Same question with this as in other places.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:405:
> SendResponse(false);
> You can avoid the cast with:
>
> ListValue* result_list = new ListValue();
> result_.reset(result_list);
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:417: if
> (!FindCommonTasks(profile_, source_url_, files_list, &common_tasks))
> This should probably be a CHECK. Is there any legit way that you can
> think of that this extension wouldn't be there?
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:509:
> path_manager->local_provider();
> ExtensionFunction has a GetExtension() method.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_file_browser_private_api.cc:743:
> dict->SetString("SELECT_OPEN_FILE_TITLE",
> Unnecessary newline?
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_function_dispatcher.cc (right):
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_function_dispatcher.cc:205: //
> fileSystem functions.
> You should only register these functions on Chrome OS. (Ideally, you
> should only build them on Chrome OS too).
>
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
> File chrome/common/extensions/api/extension_api.json (right):
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
> chrome/common/extensions/api/extension_api.json:4518:
> "namespace":"fileSystem",
> On 2011/04/12 21:09:38, Antony Sargent wrote:
>
>> would it make sense to call this namespace "fileBrowserActions"
>>
> instead of
>
>> "fileSystem"?
>>
>
> If you parse the name "fileBrowserActions" as "(file browser) actions",
> it actually makes a lot of sense. But most people with familiarity with
> the extension system will parse it as "file (browser actions)" and be
> confused.
>
> How about fileBrowserHandler to remove this ambiguity?
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
> chrome/common/extensions/api/extension_api.json:4558: "name":
> "fileUrls",
> How about "entryURLs", since they can be either files or directories.
>
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> File chrome/common/extensions/extension.cc (right):
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:905: const DictionaryValue*
> file_browser_actions, std::string* error) {
> s/file_browser_actions/file_browser_action/
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:912: if
> (!file_browser_actions->HasKey(keys::kPageActionId) ||
> Can you name this property "name" instead? (We don't use the old support
> for page action IDs anymore, so there's no precedent, and I think name
> is better).
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:914: *error =
> errors::kInvalidPageActionId;
> Incorrect error message.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:921: if
> (!file_browser_actions->HasKey(keys::kPageActionDefaultTitle) ||
> It's a bit weird to reuse these constants (pageActionDefaultTitle), but
> OK.
>
> I do like the idea of using "default_title" and "default_icon", even if
> we don't have any way to change them currently, because if we do add
> that, it will make sense.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:929: ListValue* list_value;
> = NULL
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:947: URLPattern::PARSE_LENIENT)) {
> Use PARSE_STRICT and remove the TODO(skerner). The other one was only
> lenient for legacy reasons.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:956: // Read the page action
> |default_icon| (optional).
> paste-o ('page action')
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:959: &default_icon) ||
> intent+=4
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:2084: &&
> !CommandLine::ForCurrentProcess()->HasSwitch(
> Is this testing code? Can we remove it?
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.cc:2686: ||
> CommandLine::ForCurrentProcess()->HasSwitch(
> Is this testing code? Can we remove it?
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> File chrome/common/extensions/extension.h (right):
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
> chrome/common/extensions/extension.h:644: const ListValue*
> extension_actions, std::string* error);
> Similarly to my comment in extension_api.json, the term 'extension
> action' has an existing meaning in this codebase, so I'd prefer
> 'handler' here.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
> File chrome/common/extensions/file_browser_action.h (right):
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
> chrome/common/extensions/file_browser_action.h:18: // ExtensionAction
> encapsulates the state of a browser or page action.
> Comment needs updating.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
> chrome/common/extensions/file_browser_action.h:21: class
> FileBrowserAction {
> Did you look at just using ExtensionAction directly? It has more
> features than you need, but you don't need to use those.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
> chrome/common/extensions/file_browser_action.h:39: const std::string&
> default_title() const { return title_; }
> s/default_// ?
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
> File chrome/renderer/resources/extension_process_bindings.js (left):
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
> chrome/renderer/resources/extension_process_bindings.js:100:
> Unnecessary change.
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
> File chrome/renderer/resources/extension_process_bindings.js (right):
>
>
>
http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
> chrome/renderer/resources/extension_process_bindings.js:662:
> apiFunctions["fileSystem.resolveLocalFileSystemURL"].customCallback =
> I think you are planning to remove this soon?
>
>
>
http://codereview.chromium.org/6749021/diff/61014/webkit/fileapi/file_system_...
> File webkit/fileapi/file_system_operation.cc (right):
>
>
>
http://codereview.chromium.org/6749021/diff/61014/webkit/fileapi/file_system_...
> webkit/fileapi/file_system_operation.cc:1: // Copyright (c) 2011 The
> Chromium Authors. All rights reserved.
> Unnecessary change.
>
>
> http://codereview.chromium.org/6749021/
>

[zel, 2011-04-13 17:49:55]

Few highlights here:

1. I have got rid of chrome.fileSystem.resolveLocalFileSystemURL() since we pass
the real Entry objects now.

2. With Aaron's guidance, I have "enhanced" chromeHidden.Event.dispatchJSON()
method to accommodate item #1 here. In the longer term, we should create another
IPC message and a special instance of RendererExtensionBindings::Invoke() that
will unpack JSON payload for extension events before it gets sends it to JS
layer. This is where my tweak for Entry would be migrated into.

3. A small webkit change is now needed too since I need to be able to create an
instance of FileEntry|DirectoryEntry from Chromium side. I have created an issue
for that at - https://bugs.webkit.org/show_bug.cgi?id=58456

http://codereview.chromium.org/6749021/diff/57045/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/6749021/diff/57045/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:316: #ifndef
NDEBUG
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> We have a DEBUG symbol too :)

I know, but for some strange reasons, I couldn't see them kick in ChromeOS
builds.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:117: return
false;
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> indent--

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:133:
common_tasks->insert(common_tasks->begin(),
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> indent--

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:142:
ExtensionMenuItem::List::iterator intersection_end =
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Move this line onto previous?

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:148:
common_tasks->clear();
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Is the only reason for named_action_list so that you can sort? Because an
> alternate would be to define a quick comparator that compares the titles of
the
> extension actions.

I know, that is what I was planning to do with my TODO comment that I've put
there.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:243:
BrowserThread::PostTask(
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> You can also just pass Extension* into LocalFileSystemCallbackDispatcher().
> ExtensionFunctions have a GetExtension() getter.

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:255: if
(!service)
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Unnecessary whitespace.

this method is no more

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:309: const
Extension* extension = GetExtension();
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Same question with this as in other places.

same issue with compilation, will look at it later and most likely remove this
code

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:405:
SendResponse(false);
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> You can avoid the cast with:
> 
> ListValue* result_list = new ListValue();
> result_.reset(result_list);

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:417: if
(!FindCommonTasks(profile_, source_url_, files_list, &common_tasks))
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> This should probably be a CHECK. Is there any legit way that you can think of
> that this extension wouldn't be there?

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:509:
path_manager->local_provider();
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> ExtensionFunction has a GetExtension() method.

yes, but that one gives me extension that is executing this call not that one
where I want to send files to

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:743:
dict->SetString("SELECT_OPEN_FILE_TITLE",
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Unnecessary newline?

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_menu_manager.cc (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_menu_manager.cc:409:
properties->SetString("mediaType", "file");
On 2011/04/12 21:09:38, Antony Sargent wrote:
> I think you can remove this change, since you're dispatching a different event
> on a different namespace now, right? 

Yes, this file should have been reverted. Done now.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4518: "namespace":"fileSystem",
On 2011/04/12 21:09:38, Antony Sargent wrote:
> would it make sense to call this namespace "fileBrowserActions" instead of
> "fileSystem"? 

I wanted to keep it generic enough in case we add more file browser related
stuff here in the future... we most certainly will. "fileBrowser" would do it
too - does that sound more acceptable to you?

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4518: "namespace":"fileSystem",
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> On 2011/04/12 21:09:38, Antony Sargent wrote:
> > would it make sense to call this namespace "fileBrowserActions" instead of
> > "fileSystem"? 
> 
> If you parse the name "fileBrowserActions" as "(file browser) actions", it
> actually makes a lot of sense. But most people with familiarity with the
> extension system will parse it as "file (browser actions)" and be confused.
> 
> How about fileBrowserHandler to remove this ambiguity?


"fileBrowserHandler" it is.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4558: "name": "fileUrls",
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> How about "entryURLs", since they can be either files or directories.

changed to 'entries' since this is what it is now and not url anymore

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

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:905: const DictionaryValue*
file_browser_actions, std::string* error) {
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> s/file_browser_actions/file_browser_action/

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:914: *error =
errors::kInvalidPageActionId;
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Incorrect error message.

This message is used for generic "id is missing" errors all across the code. We
should probably rename the variable?

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:921: if
(!file_browser_actions->HasKey(keys::kPageActionDefaultTitle) ||
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> It's a bit weird to reuse these constants (pageActionDefaultTitle), but OK.
> 
> I do like the idea of using "default_title" and "default_icon", even if we
don't
> have any way to change them currently, because if we do add that, it will make
> sense.

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:929: ListValue* list_value;
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> = NULL

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:947: URLPattern::PARSE_LENIENT)) {
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Use PARSE_STRICT and remove the TODO(skerner). The other one was only lenient
> for legacy reasons.

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:956: // Read the page action
|default_icon| (optional).
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> paste-o ('page action')

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:959: &default_icon) ||
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> intent+=4

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:2001: // Initialize browser action
(optional).
On 2011/04/12 21:09:38, Antony Sargent wrote:
> nit: comment should be "Initialize file browser actions"

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:2013: return false;  // Failed to parse
browser action definition.
On 2011/04/12 21:09:38, Antony Sargent wrote:
> same naming nit on comment here

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:2084: &&
!CommandLine::ForCurrentProcess()->HasSwitch(
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Is this testing code? Can we remove it?

This is super useful for us while debugging component extensions. It let us
rerun them as regular extensions without rebuilding chrome binary.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:2686: ||
CommandLine::ForCurrentProcess()->HasSwitch(
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Is this testing code? Can we remove it?

see comments above

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
File chrome/common/extensions/file_browser_action.h (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_action.h:18: // ExtensionAction
encapsulates the state of a browser or page action.
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Comment needs updating.

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_action.h:21: class FileBrowserAction {
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Did you look at just using ExtensionAction directly? It has more features than
> you need, but you don't need to use those.

I did. It felt way too complex for this purpose and I have no URL pattern
matching stuff there.

http://codereview.chromium.org/6749021/diff/61014/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_action.h:39: const std::string&
default_title() const { return title_; }
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> s/default_// ?

Done.

http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:662:
apiFunctions["fileSystem.resolveLocalFileSystemURL"].customCallback =
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> I think you are planning to remove this soon?

It's gone now.

http://codereview.chromium.org/6749021/diff/61014/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation.cc (right):

http://codereview.chromium.org/6749021/diff/61014/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> Unnecessary change.

remains of merge+revert+merge+revert. reverted now

[Aaron Boodman, 2011-04-13 18:08:03]

Basically: s/filebrowseraction/filebrowserhandler/g and make sure this only is
available on Chrome OS, and I am happy.

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.cc:205: // fileSystem
functions.
On 2011/04/12 22:47:21, Aaron Boodman wrote:
> You should only register these functions on Chrome OS. (Ideally, you should
only
> build them on Chrome OS too).

You didn't address this comment yet.

http://codereview.chromium.org/6749021/diff/65005/chrome/common/extensions/fi...
File chrome/common/extensions/file_browser_action.h (right):

http://codereview.chromium.org/6749021/diff/65005/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_action.h:19: class FileBrowserAction {
FileBrowserHandler

http://codereview.chromium.org/6749021/diff/65005/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (left):

http://codereview.chromium.org/6749021/diff/65005/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:100: 
Unnecessary change :)

http://codereview.chromium.org/6749021/diff/65005/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/filesystem_handler/manifest.json
(right):

http://codereview.chromium.org/6749021/diff/65005/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/filesystem_handler/manifest.json:7:
"file_browser_actions": [
file_browser_handlers

http://codereview.chromium.org/6749021/diff/65005/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/filesystem_handler/manifest.json:12:
"file_filters" : [ "filesystem:*.txt" ]
I am surprised that this pattern parses in URLPattern. Is the filesystem scheme
"standard" (eg, is it expected to have a host component?). I only expect this
kind of pattern to work for non-standard schemes, like file: and javascript:.

If filesystem is non-standard, is it on purpose?

[zel, 2011-04-14 21:58:05]

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/6749021/diff/61014/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.cc:205: // fileSystem
functions.
On 2011/04/13 18:08:03, Aaron Boodman wrote:
> On 2011/04/12 22:47:21, Aaron Boodman wrote:
> > You should only register these functions on Chrome OS. (Ideally, you should
> only
> > build them on Chrome OS too).
> 
> You didn't address this comment yet.

Done.

http://codereview.chromium.org/6749021/diff/65005/chrome/common/extensions/fi...
File chrome/common/extensions/file_browser_action.h (right):

http://codereview.chromium.org/6749021/diff/65005/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_action.h:19: class FileBrowserAction {
On 2011/04/13 18:08:03, Aaron Boodman wrote:
> FileBrowserHandler

Done.

http://codereview.chromium.org/6749021/diff/65005/chrome/renderer/resources/e...
File chrome/renderer/resources/extension_process_bindings.js (left):

http://codereview.chromium.org/6749021/diff/65005/chrome/renderer/resources/e...
chrome/renderer/resources/extension_process_bindings.js:100: 
On 2011/04/13 18:08:03, Aaron Boodman wrote:
> Unnecessary change :)

Reverted.

http://codereview.chromium.org/6749021/diff/65005/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/filesystem_handler/manifest.json
(right):

http://codereview.chromium.org/6749021/diff/65005/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/filesystem_handler/manifest.json:7:
"file_browser_actions": [
On 2011/04/13 18:08:03, Aaron Boodman wrote:
> file_browser_handlers

Done.

http://codereview.chromium.org/6749021/diff/65005/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/filesystem_handler/manifest.json:12:
"file_filters" : [ "filesystem:*.txt" ]
On 2011/04/13 18:08:03, Aaron Boodman wrote:
> I am surprised that this pattern parses in URLPattern. Is the filesystem
scheme
> "standard" (eg, is it expected to have a host component?). I only expect this
> kind of pattern to work for non-standard schemes, like file: and javascript:.
> 
> If filesystem is non-standard, is it on purpose?

filesystem: is a wacky one, there is no 'host' but it contains origin which can
be any url. For extensions, it will look like:

filesystem:chrome-extension://<ext_id>/(permanent|temporary|local)/<virtual_path>

And yes, URLPattern does work with these filters here.

[Aaron Boodman, 2011-04-14 22:04:34]

One last naming thing.

LGTM, no need for another review pass.

http://codereview.chromium.org/6749021/diff/73003/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6749021/diff/73003/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4522: "name": "onExecuteAction",
How about 'onExecute' since there is no 'action' in this API anymore.

[Evan Martin, 2011-04-14 22:06:32]

Drive-by: Can you make the first line of the change summary be a summary of what
the change is?  It's not useful in the future to see "4th rewrite" in the commit
log.

[asargent_no_longer_on_chrome, 2011-04-14 22:11:46]

lgtm as well assuming you address the final nits brought up by others

[zel, 2011-04-14 22:12:59]

yes, sorry there were other flavors of this in different CLs so I used this
a version identifier on my side

On Thu, Apr 14, 2011 at 3:06 PM, <evan@chromium.org> wrote:

> Drive-by: Can you make the first line of the change summary be a summary of
> what
> the change is?  It's not useful in the future to see "4th rewrite" in the
> commit
> log.
>
>
> http://codereview.chromium.org/6749021/
>

[zel, 2011-04-14 23:51:11]

http://codereview.chromium.org/6749021/diff/73003/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/6749021/diff/73003/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:4522: "name": "onExecuteAction",
On 2011/04/14 22:04:34, Aaron Boodman wrote:
> How about 'onExecute' since there is no 'action' in this API anymore.

Yes, I've been thinking about the same rename now when 'handler' is part of the
event name. Done.