Support choosing multiple files with fileSystem.chooseEntry.

Support choosing multiple files with fileSystem.chooseEntry.

This adds acceptMultiple to ChooseEntryOptions, which when set to true,
presents the user with a file chooser dialog that allows the selection
of multiple files.

BUG=159062
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215269

[Matt Giuca, 2013-07-19 02:28:11]

Some initial comments. I haven't done a full review yet.

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:65: const char
kWritableFileErrorPrefix[] = "Error opening ";
I think this should be a full format string like "Error opening %s" and use
StringPrintf later.

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:273: // Handles
checking whether a list of paths are all OK for writing and calls a
"Checks whether".

What does "OK" mean? (Not read-only in the host OS? Valid file entries? Opened
as writable, or merely openable as writable?)

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.h:67: // thread.
Describe |multiple|.

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
chrome/common/extensions/api/file_system.idl:68: [instanceOf=FileEntry] optional
object fileEntry,
What is the first fileEntry for? (Why not just take a list?) If there is a good
reason, please document.

https://codereview.chromium.org/18331017/diff/39001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/18331017/diff/39001/chrome/renderer/resources...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:29: if
(request.callback && response && response) {
response.entries?
(Otherwise, why response && response?)

https://codereview.chromium.org/18331017/diff/39001/chrome/renderer/resources...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:35: var
maybeDone = function() {
Comment here to explain the fan-out.

https://codereview.chromium.org/18331017/diff/39001/chrome/renderer/resources...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:58:
maybeDone();
Why is maybeDone a function? I expected it to be used as a closure later on, but
you just call it here. Why not just inline the code?

[Sam McNally, 2013-07-19 04:22:14]

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:65: const char
kWritableFileErrorPrefix[] = "Error opening ";
On 2013/07/19 02:28:11, Matt Giuca wrote:
> I think this should be a full format string like "Error opening %s" and use
> StringPrintf later.

Done.

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:273: // Handles
checking whether a list of paths are all OK for writing and calls a
On 2013/07/19 02:28:11, Matt Giuca wrote:
> "Checks whether".
> 
> What does "OK" mean? (Not read-only in the host OS? Valid file entries? Opened
> as writable, or merely openable as writable?)

Done.

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/18331017/diff/39001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.h:67: // thread.
On 2013/07/19 02:28:11, Matt Giuca wrote:
> Describe |multiple|.

Done.

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
chrome/common/extensions/api/file_system.idl:68: [instanceOf=FileEntry] optional
object fileEntry,
On 2013/07/19 02:28:11, Matt Giuca wrote:
> What is the first fileEntry for? (Why not just take a list?) If there is a
good
> reason, please document.

Backwards compatibility; chooseEntry with acceptsMultiple: false returns a
single FileEntry. The callback needs to accept one FileEntry or FileEntry[] and
this syntax doesn't make the parser angry. A comment here would end up in the
docs.

https://codereview.chromium.org/18331017/diff/39001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/18331017/diff/39001/chrome/renderer/resources...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:29: if
(request.callback && response && response) {
On 2013/07/19 02:28:11, Matt Giuca wrote:
> response.entries?
> (Otherwise, why response && response?)

Done.

https://codereview.chromium.org/18331017/diff/39001/chrome/renderer/resources...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:35: var
maybeDone = function() {
On 2013/07/19 02:28:11, Matt Giuca wrote:
> Comment here to explain the fan-out.

Done.

https://codereview.chromium.org/18331017/diff/39001/chrome/renderer/resources...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:58:
maybeDone();
On 2013/07/19 02:28:11, Matt Giuca wrote:
> Why is maybeDone a function? I expected it to be used as a closure later on,
but
> you just call it here. Why not just inline the code?

Done.

[Matt Giuca, 2013-07-25 08:31:09]

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
chrome/common/extensions/api/file_system.idl:68: [instanceOf=FileEntry] optional
object fileEntry,
Is it worth refactoring so that there are two separate interfaces for opening a
dialog (single and multiple); the single one returns FileEntryCallback and the
multiple one returns MultipleFileEntryCallback? It seems messy this way. Will
the user see the name "OneOrMultipleFileEntryCallback" (even in the
documentation) or is that hidden?

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:277: // on_success
or on_failure callback when done. A file is OK for writing if it
If it "is" not a symlink.

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:314: friend class
base::RefCountedThreadSafe<WritableFileChecker>;
Can't you just make ~WritableFileChecker protected and then the base class will
have access? Maybe protected does not work that way.

In which case,
// Needed for base class access to ~WritableFileChecker.

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:317: void
TaskDone() {
Comment.

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:319: if
(!--outstanding_tasks_) {
--outstanding_tasks_ == 0

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:327: void
Error(const std::string& message) {
Comment. Include the fact that it must only be called once.

Actually: Can you guarantee this will only be called once? Looks like
CheckLocalWritableFiles can call it as well as CheckRemoteWritableFile, so we
could potentially get two calls to Error, the second of which will fail.

(Though I haven't yet read the code that calls into this class, I shouldn't have
to.)

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:519: void
FileSystemEntryFunction::RegisterFileSystemsAndSendResponse(
This used to use RegisterFileSystemAndSendResponseWithIdOverride to avoid code
duplication. Now it doesn't. Any reason why? Can you rearrange it so they are
sharing code again?

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:112:
destination_names.begin();
Indentation?

https://codereview.chromium.org/18331017/diff/75001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_system/open_multiple_existing/test.js
(right):

https://codereview.chromium.org/18331017/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_multiple_existing/test.js:8:
{acceptsMultiple: true},
Can I see a test where suggestedName is used in conjunction with multiple?

(Not a big deal.)

[benwells, 2013-07-25 09:08:31]

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
chrome/common/extensions/api/file_system.idl:68: [instanceOf=FileEntry] optional
object fileEntry,
On 2013/07/25 08:31:09, Matt Giuca wrote:
> Is it worth refactoring so that there are two separate interfaces for opening
a
> dialog (single and multiple); the single one returns FileEntryCallback and the
> multiple one returns MultipleFileEntryCallback? It seems messy this way. Will
> the user see the name "OneOrMultipleFileEntryCallback" (even in the
> documentation) or is that hidden?

Do you mean there would be another function like chooseEntries (or
chooseMultipleEntries) which is identical except it implies multiple entries and
has an array in the callback?

That would work; I'm not sure if it is better. My vote would be to leave it as
is but rename the type to FileEntriesCallback (or something). We can explain in
doco how to use it. I'd weigh the vote of someone with more javascript
experience more than mine though; I was under the impression this kind of type
shenanigans are common in JS.

[Matt Giuca, 2013-07-25 23:37:02]

On 2013/07/25 09:08:31, benwells wrote:
>
https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
> File chrome/common/extensions/api/file_system.idl (right):
> 
>
https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
> chrome/common/extensions/api/file_system.idl:68: [instanceOf=FileEntry]
optional
> object fileEntry,
> On 2013/07/25 08:31:09, Matt Giuca wrote:
> > Is it worth refactoring so that there are two separate interfaces for
opening
> a
> > dialog (single and multiple); the single one returns FileEntryCallback and
the
> > multiple one returns MultipleFileEntryCallback? It seems messy this way.
Will
> > the user see the name "OneOrMultipleFileEntryCallback" (even in the
> > documentation) or is that hidden?
> 
> Do you mean there would be another function like chooseEntries (or
> chooseMultipleEntries) which is identical except it implies multiple entries
and
> has an array in the callback?
> 
> That would work; I'm not sure if it is better. My vote would be to leave it as
> is but rename the type to FileEntriesCallback (or something). We can explain
in
> doco how to use it. I'd weigh the vote of someone with more javascript
> experience more than mine though; I was under the impression this kind of type
> shenanigans are common in JS.

Not implying I have much JS experience. I'd be happy with FileEntriesCallback as
well. (I think mostly I just didn't like the name with "Or" in it.)

[Sam McNally, 2013-07-26 03:58:03]

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
File chrome/common/extensions/api/file_system.idl (right):

https://codereview.chromium.org/18331017/diff/39001/chrome/common/extensions/...
chrome/common/extensions/api/file_system.idl:68: [instanceOf=FileEntry] optional
object fileEntry,
On 2013/07/25 09:08:31, benwells wrote:
> On 2013/07/25 08:31:09, Matt Giuca wrote:
> > Is it worth refactoring so that there are two separate interfaces for
opening
> a
> > dialog (single and multiple); the single one returns FileEntryCallback and
the
> > multiple one returns MultipleFileEntryCallback? It seems messy this way.
Will
> > the user see the name "OneOrMultipleFileEntryCallback" (even in the
> > documentation) or is that hidden?
> 
> Do you mean there would be another function like chooseEntries (or
> chooseMultipleEntries) which is identical except it implies multiple entries
and
> has an array in the callback?
> 
> That would work; I'm not sure if it is better. My vote would be to leave it as
> is but rename the type to FileEntriesCallback (or something). We can explain
in
> doco how to use it. I'd weigh the vote of someone with more javascript
> experience more than mine though; I was under the impression this kind of type
> shenanigans are common in JS.

Done.

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:277: // on_success
or on_failure callback when done. A file is OK for writing if it
On 2013/07/25 08:31:09, Matt Giuca wrote:
> If it "is" not a symlink.

Done.

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:314: friend class
base::RefCountedThreadSafe<WritableFileChecker>;
On 2013/07/25 08:31:09, Matt Giuca wrote:
> Can't you just make ~WritableFileChecker protected and then the base class
will
> have access? Maybe protected does not work that way.
> 
> In which case,
> // Needed for base class access to ~WritableFileChecker.

This is what the instructions say to do:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co...

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:317: void
TaskDone() {
On 2013/07/25 08:31:09, Matt Giuca wrote:
> Comment.

Done.

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:319: if
(!--outstanding_tasks_) {
On 2013/07/25 08:31:09, Matt Giuca wrote:
> --outstanding_tasks_ == 0

Done.

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:327: void
Error(const std::string& message) {
On 2013/07/25 08:31:09, Matt Giuca wrote:
> Comment. Include the fact that it must only be called once.
> 
> Actually: Can you guarantee this will only be called once? Looks like
> CheckLocalWritableFiles can call it as well as CheckRemoteWritableFile, so we
> could potentially get two calls to Error, the second of which will fail.
> 
> (Though I haven't yet read the code that calls into this class, I shouldn't
have
> to.)

Done.

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_api.cc:519: void
FileSystemEntryFunction::RegisterFileSystemsAndSendResponse(
On 2013/07/25 08:31:09, Matt Giuca wrote:
> This used to use RegisterFileSystemAndSendResponseWithIdOverride to avoid code
> duplication. Now it doesn't. Any reason why? Can you rearrange it so they are
> sharing code again?

Done.

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/18331017/diff/75001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:112:
destination_names.begin();
On 2013/07/25 08:31:09, Matt Giuca wrote:
> Indentation?

Clang format likes it this way.

https://codereview.chromium.org/18331017/diff/75001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/file_system/open_multiple_existing/test.js
(right):

https://codereview.chromium.org/18331017/diff/75001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/file_system/open_multiple_existing/test.js:8:
{acceptsMultiple: true},
On 2013/07/25 08:31:09, Matt Giuca wrote:
> Can I see a test where suggestedName is used in conjunction with multiple?
> 
> (Not a big deal.)

Done.

[Matt Giuca, 2013-08-01 00:02:23]

LGTM with nits.

https://codereview.chromium.org/18331017/diff/104001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/104001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:332:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Nit: Remove this DCHECK. There is nothing in this function that requires the UI
thread, other than the call to TaskDone (which does its own DCHECK).

https://codereview.chromium.org/18331017/diff/104001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:537: void
FileSystemEntryFunction::RegisterFileSystemsAndSendResponseWithIdOverrides(
Nit: Move this function below RegisterFileSystemAndSendResponseWithIdOverride
(since it is a utility function for both the above and below function, it
doesn't belong in the middle between them).

[Matt Giuca, 2013-08-01 00:02:47]

Apologies for the delay in reviewing this.

[Sam McNally, 2013-08-01 00:08:38]

https://codereview.chromium.org/18331017/diff/104001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/104001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:332:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2013/08/01 00:02:24, Matt Giuca wrote:
> Nit: Remove this DCHECK. There is nothing in this function that requires the
UI
> thread, other than the call to TaskDone (which does its own DCHECK).

Done.

https://codereview.chromium.org/18331017/diff/104001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:537: void
FileSystemEntryFunction::RegisterFileSystemsAndSendResponseWithIdOverrides(
On 2013/08/01 00:02:24, Matt Giuca wrote:
> Nit: Move this function below RegisterFileSystemAndSendResponseWithIdOverride
> (since it is a utility function for both the above and below function, it
> doesn't belong in the middle between them).

Done.

[benwells, 2013-08-01 07:48:32]

Initial review, I've only looked at non-test code so far.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:320:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
Can you reduce the amount of task posting by executing TaskDone and Error on the
FILE thread, posting back to the UI thread once all tasks are done? I think that
would make the code clearer and reduce the amount of work overall.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:505: void
FileSystemEntryFunction::CheckWritableFile(const base::FilePath& path) {
Optional: I think you should remove this and put the implementation in the one
caller. It is only used in GetWritableFile which we're planning to remove.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:512: const
std::vector<base::FilePath>& paths, bool multiple) {
Instead of plumbing multiple through everywhere you should be able to just have
a field on FileSystemEntryFunction. There is one FileSystemEntryFunction created
for every invocation of the function.

Maybe we could also do that for entry type, but keep it separate if you want to
do that to keep this patch asap (as simple as possible).

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:536: void
FileSystemEntryFunction::RegisterFileSystemAndSendResponseWithIdOverride(
Same for this as CheckWritableFileEntry: it is only used once, do we need a
wrapper function?

Alternatively, since the only thing that uses an id override doesn't handle
multiple can we not worry with a multiple capable send response with id
override? This would remove a lot of code, and if you instead have functions
like CreateResponse and AddEntryToResponse you can reduce duplication.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:853:
file_system_api::SetLastChooseEntryDirectory(
Add DCHECK that there is at least one path.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.h:62: void
CheckWritableFiles(const std::vector<base::FilePath>& path,
Nit: document both methods, and |multiple|.

https://codereview.chromium.org/18331017/diff/123001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/18331017/diff/123001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:40: var fs =
GetIsolatedFileSystem(fileSystemId);
Should you check hasError here instead of in the error handler / exception
handler?

[Sam McNally, 2013-08-02 05:35:31]

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:320:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2013/08/01 07:48:33, benwells wrote:
> Can you reduce the amount of task posting by executing TaskDone and Error on
the
> FILE thread, posting back to the UI thread once all tasks are done? I think
that
> would make the code clearer and reduce the amount of work overall.

Done.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:505: void
FileSystemEntryFunction::CheckWritableFile(const base::FilePath& path) {
On 2013/08/01 07:48:33, benwells wrote:
> Optional: I think you should remove this and put the implementation in the one
> caller. It is only used in GetWritableFile which we're planning to remove.

Done.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:512: const
std::vector<base::FilePath>& paths, bool multiple) {
On 2013/08/01 07:48:33, benwells wrote:
> Instead of plumbing multiple through everywhere you should be able to just
have
> a field on FileSystemEntryFunction. There is one FileSystemEntryFunction
created
> for every invocation of the function.
> 
> Maybe we could also do that for entry type, but keep it separate if you want
to
> do that to keep this patch asap (as simple as possible).

Done.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:536: void
FileSystemEntryFunction::RegisterFileSystemAndSendResponseWithIdOverride(
On 2013/08/01 07:48:33, benwells wrote:
> Same for this as CheckWritableFileEntry: it is only used once, do we need a
> wrapper function?
> 
> Alternatively, since the only thing that uses an id override doesn't handle
> multiple can we not worry with a multiple capable send response with id
> override? This would remove a lot of code, and if you instead have functions
> like CreateResponse and AddEntryToResponse you can reduce duplication.

Done.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:853:
file_system_api::SetLastChooseEntryDirectory(
On 2013/08/01 07:48:33, benwells wrote:
> Add DCHECK that there is at least one path.

Done.

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/18331017/diff/123001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.h:62: void
CheckWritableFiles(const std::vector<base::FilePath>& path,
On 2013/08/01 07:48:33, benwells wrote:
> Nit: document both methods, and |multiple|.

Done.

https://codereview.chromium.org/18331017/diff/123001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/file_system_custom_bindings.js
(right):

https://codereview.chromium.org/18331017/diff/123001/chrome/renderer/resource...
chrome/renderer/resources/extensions/file_system_custom_bindings.js:40: var fs =
GetIsolatedFileSystem(fileSystemId);
On 2013/08/01 07:48:33, benwells wrote:
> Should you check hasError here instead of in the error handler / exception
> handler?

Added a comment describing what's going on here.

[benwells, 2013-08-02 07:41:31]

lg with nits so far, still looking at tests

https://codereview.chromium.org/18331017/diff/140001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/140001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:319: void
TaskDone() {
Nit: Add a DCHECK on file thread here.

Also consider adding to Error and CheckRemoteWritableFile, but maybe that's
getting too paranoid.

https://codereview.chromium.org/18331017/diff/140001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:536: const
std::string& id) {
nit: id -> id_override

[benwells, 2013-08-02 08:05:02]

lgtm. LGTM!

[Sam McNally, 2013-08-02 08:17:52]

https://codereview.chromium.org/18331017/diff/140001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/18331017/diff/140001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:319: void
TaskDone() {
On 2013/08/02 07:41:31, benwells wrote:
> Nit: Add a DCHECK on file thread here.
> 
> Also consider adding to Error and CheckRemoteWritableFile, but maybe that's
> getting too paranoid.

Done.

https://codereview.chromium.org/18331017/diff/140001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:536: const
std::string& id) {
On 2013/08/02 07:41:31, benwells wrote:
> nit: id -> id_override

Done.

[commit-bot: I haz the power, 2013-08-02 08:21:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/18331017/149001

[commit-bot: I haz the power, 2013-08-02 11:26:02]

Change committed as 215269