Move chrome.fileSystem implementation to //extensions

Move chrome.fileSystem implementation to //extensions

This ancient API has several Chrome dependencies, which have been moved
into a FileSystemDelegate class that performs the Chrome-specific work.

I also made a delegate for apps::SavedFilesService which is only
implemented in //apps, but allows //extensions to invoke this
apps-specific code without adding a hard dependency.

Since so much of the code was shuffled around anyway, I also ran
`git cl format` on everything.

Future work includes creating an AppShellFileSystemDelegate to make
certain functionality available in app_shell.

BUG=729713
R=benwells@chromium.org,rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2934143002
Cr-Commit-Position: refs/heads/master@{#486609}
Committed: https://chromium.googlesource.com/chromium/src/+/56c27b3401b61598c079687a032fc9a55258dbdb

[michaelpg, 2017-06-13 20:30:05]

Description was changed from

==========
Move chrome.fileSystem implementation to //extensions

This ancient API has several Chrome dependencies, which have been moved
into a FileSystemDelegate class that performs the Chrome-specific work.

I also made a delegate for apps::SavedFilesService which is only
implemented in //apps, but allows //extensions to invoke this
apps-specific code without adding a hard dependency.

Since so much of the code was shuffled around anyway, I also ran
`git cl format` on everything.

Future work includes creating an AppShellFileSystemDelegatea to make
certain functionality available in app_shell.

BUG=729713
R=benwells@chromium.org,rdevlin.cronin@chromium.org
==========

to

==========
Move chrome.fileSystem implementation to //extensions

This ancient API has several Chrome dependencies, which have been moved
into a FileSystemDelegate class that performs the Chrome-specific work.

I also made a delegate for apps::SavedFilesService which is only
implemented in //apps, but allows //extensions to invoke this
apps-specific code without adding a hard dependency.

Since so much of the code was shuffled around anyway, I also ran
`git cl format` on everything.

Future work includes creating an AppShellFileSystemDelegate to make
certain functionality available in app_shell.

BUG=729713
R=benwells@chromium.org,rdevlin.cronin@chromium.org
==========

[michaelpg, 2017-06-13 20:36:49]

benwells: PTAL. Sorry it's large -- I extracted what I could
beforehand. This extension API will need to be added to
app_shell ASAP to unblock our fishfood.

Devlin: FYI. Maybe check that my callbacks looks safe? (Hopefully this is even a
safety improvement over the existing code.)

[benwells, 2017-06-14 03:46:37]

I'm unfamiliar with the new extension function stuff so I'll leave that to
Devlin. Overall looks great.

I'm also unfamiliar now with the file system stuff ... but as you're mainly
moving stuff around that should be OK.

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:400: // TODO(michaelpg):
Use the FileSystemdelegate to override functionality for
Nit: Delegate

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:883: auto
saved_files_service_delegate =
Did you consider:
* changing the existing SavedFileService to SavedFileServiceImpl
* having what you call SavedFileServiceDelegate be a SavedFileService interface
* have a GetSavedFileService function on FileSystemDelegate?

I think there would be one less delegate implementation and lifetime to worry
about then.

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:939:
Respond(Error(kUnknownErrorDoNotUse));
I have a feeling you're not meant to use this. Want to just add a proper error
while you're here?

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_delegate.cc (right):

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_delegate.cc:26: NOTIMPLEMENTED();
Can you just leave this as a pure interface?

[michaelpg, 2017-06-14 04:51:41]

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:400: // TODO(michaelpg):
Use the FileSystemdelegate to override functionality for
On 2017/06/14 03:46:37, benwells wrote:
> Nit: Delegate

Done.

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:883: auto
saved_files_service_delegate =
On 2017/06/14 03:46:37, benwells wrote:
> Did you consider:
> * changing the existing SavedFileService to SavedFileServiceImpl
> * having what you call SavedFileServiceDelegate be a SavedFileService
interface
> * have a GetSavedFileService function on FileSystemDelegate?
> 
> I think there would be one less delegate implementation and lifetime to worry
> about then.

I'm not sure I want to expose entire SavedFilesService interface from
//extensions instead of //apps. The "delegate" shim is annoying, but it's an
app-specific service, not extensions-wide.

Ideally this API would *itself* live in //apps, and then we wouldn't have to use
indirection at all. I think that's relatively doable but would require some
rejiggering of the generated bindings, which is an investigation I don't have
the cycles for right now. Devlin, do you think that's something we can pursue in
the short-to-medium term?

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:939:
Respond(Error(kUnknownErrorDoNotUse));
On 2017/06/14 03:46:37, benwells wrote:
> I have a feeling you're not meant to use this. Want to just add a proper error
> while you're here?

Hehe. Well, the comment for this error says "DON'T USE THIS. It's only here
during conversion to flag cases where errors aren't already set." Sooo I *am*
using it during the conversion (from AsyncExtension to
UIThreadExtensionFunction) to flag this case of the error not being set.

But really I just don't feel like going through all of the callbacks right now
to determine all the different possible errors and handling them. If I did, it
would be as a follow-up.

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_delegate.cc (right):

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_delegate.cc:26: NOTIMPLEMENTED();
On 2017/06/14 03:46:37, benwells wrote:
> Can you just leave this as a pure interface?

Done.

[benwells, 2017-06-14 06:33:04]

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:883: auto
saved_files_service_delegate =
On 2017/06/14 04:51:41, michaelpg wrote:
> On 2017/06/14 03:46:37, benwells wrote:
> > Did you consider:
> > * changing the existing SavedFileService to SavedFileServiceImpl
> > * having what you call SavedFileServiceDelegate be a SavedFileService
> interface
> > * have a GetSavedFileService function on FileSystemDelegate?
> > 
> > I think there would be one less delegate implementation and lifetime to
worry
> > about then.
> 
> I'm not sure I want to expose entire SavedFilesService interface from
> //extensions instead of //apps. The "delegate" shim is annoying, but it's an
> app-specific service, not extensions-wide.
> 
> Ideally this API would *itself* live in //apps, and then we wouldn't have to
use
> indirection at all. I think that's relatively doable but would require some
> rejiggering of the generated bindings, which is an investigation I don't have
> the cycles for right now. Devlin, do you think that's something we can pursue
in
> the short-to-medium term?

I didn't mean exposing the whole SavedFilesService. You'd still have the small
interface, but you wouldn't have your little wrapper class. I'll explain
differently:
* Change your SavedFileServiceDelegate interface to SavedFileServiceInterface
* Implement it on the existing SavedFileService, instead of introducing
SavedFileServiceDelegateImpl
* Have a GetSavedFileService function on FileSystemDelegate:

SavedFileServiceInterface GetSavedFileService(BrowserContext)

which just returns the SavedFileService.

Does that make sense?

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:939:
Respond(Error(kUnknownErrorDoNotUse));
On 2017/06/14 04:51:41, michaelpg wrote:
> On 2017/06/14 03:46:37, benwells wrote:
> > I have a feeling you're not meant to use this. Want to just add a proper
error
> > while you're here?
> 
> Hehe. Well, the comment for this error says "DON'T USE THIS. It's only here
> during conversion to flag cases where errors aren't already set." Sooo I *am*
> using it during the conversion (from AsyncExtension to
> UIThreadExtensionFunction) to flag this case of the error not being set.
> 
> But really I just don't feel like going through all of the callbacks right now
> to determine all the different possible errors and handling them. If I did, it
> would be as a follow-up.

This was the only one I saw, but I can see there is one other use about 40 lines
up.

I was just thinking you'd add kCouldntGetFileInfo error for this one. The same
error would probably work for the other use as well.

But I'm not too fussed. If you want to make patches smaller and easier to review
in future though, it would have been nice not to have the extension function
conversion mixed in with moving the API, if that was possible.

[Devlin, 2017-06-14 14:53:22]

Oof, this one's rough (though not your fault at all - it's a tricky space).

Also +1 to Ben's suggestion re the delegation pattern.

https://codereview.chromium.org/2934143002/diff/80001/apps/saved_files_servic...
File apps/saved_files_service_delegate_impl.cc (right):

https://codereview.chromium.org/2934143002/diff/80001/apps/saved_files_servic...
apps/saved_files_service_delegate_impl.cc:14: saved_files_service_ =
SavedFilesService::Get(browser_context);
why not initialize this in the initializer list?

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc
(right):

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc:258:
&ChromeFileSystemDelegate::OnConsentReceived, base::Unretained(this),
document why this unretained is safe

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc:290:
error_callback.Run("");
I know this was just copied, but prefer std::string() over "".

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/chrome_file_system_delegate.h
(right):

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.h:38: 
// FileSystemDelegate:

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.h:72: //
system. May prompt the user for consent.
Shouldn't need comments for overridden methods.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:135:
delegate->GetDescriptionIdForAcceptType(accept_type, &description_id);
Shouldn't we check the return value?

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:695:
base::Bind(&FileSystemChooseEntryFunction::OnDirectoryAccessConfirmed,
Could these be BindOnce's?

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:883: auto
saved_files_service_delegate =
nit: don't use auto here - it's not clear what the return result will be (i.e.,
std::unique_ptr<>, stack allocated, etc.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:902: return
RespondNow(Error(kUnknownErrorDoNotUse));
Can we think of an error here?

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:939:
Respond(Error(kUnknownErrorDoNotUse));
ditto re error

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:947: auto
saved_files_service_delegate =
ditto re auto

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:964: auto
saved_files_service_delegate =
ditto re auto (apply to the rest of the file :))

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.h:169: content::WebContents*
web_contents);
\n

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.h:173: content::WebContents*
web_contents);
\n

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_delegate.h (right):

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_delegate.h:55: virtual
std::unique_ptr<ui::SelectFilePolicy> CreateSelectFilePolicy(
add function comments

[michaelpg, 2017-06-21 00:40:08]

Some minor changes. I've uploaded benwells' suggestion for
SavedFilesService as a separate standalone CL. So after
sending this patch I'll rebase this CL on that one, hopefully
making this one smaller... see https://codereview.chromium.org/2951003002/

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:883: auto
saved_files_service_delegate =
On 2017/06/14 06:33:04, benwells wrote:
> On 2017/06/14 04:51:41, michaelpg wrote:
> > On 2017/06/14 03:46:37, benwells wrote:
> > > Did you consider:
> > > * changing the existing SavedFileService to SavedFileServiceImpl
> > > * having what you call SavedFileServiceDelegate be a SavedFileService
> > interface
> > > * have a GetSavedFileService function on FileSystemDelegate?
> > > 
> > > I think there would be one less delegate implementation and lifetime to
> worry
> > > about then.
> > 
> > I'm not sure I want to expose entire SavedFilesService interface from
> > //extensions instead of //apps. The "delegate" shim is annoying, but it's an
> > app-specific service, not extensions-wide.
> > 
> > Ideally this API would *itself* live in //apps, and then we wouldn't have to
> use
> > indirection at all. I think that's relatively doable but would require some
> > rejiggering of the generated bindings, which is an investigation I don't
have
> > the cycles for right now. Devlin, do you think that's something we can
pursue
> in
> > the short-to-medium term?
> 
> I didn't mean exposing the whole SavedFilesService. You'd still have the small
> interface, but you wouldn't have your little wrapper class. I'll explain
> differently:
> * Change your SavedFileServiceDelegate interface to SavedFileServiceInterface
> * Implement it on the existing SavedFileService, instead of introducing
> SavedFileServiceDelegateImpl
> * Have a GetSavedFileService function on FileSystemDelegate:
> 
> SavedFileServiceInterface GetSavedFileService(BrowserContext)
> 
> which just returns the SavedFileService.
> 
> Does that make sense?

Yes, I think so. Check out https://codereview.chromium.org/2951003002/
which I'll rebase this off of.

https://codereview.chromium.org/2934143002/diff/60001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:939:
Respond(Error(kUnknownErrorDoNotUse));
On 2017/06/14 06:33:04, benwells wrote:
> On 2017/06/14 04:51:41, michaelpg wrote:
> > On 2017/06/14 03:46:37, benwells wrote:
> > > I have a feeling you're not meant to use this. Want to just add a proper
> error
> > > while you're here?
> > 
> > Hehe. Well, the comment for this error says "DON'T USE THIS. It's only here
> > during conversion to flag cases where errors aren't already set." Sooo I
*am*
> > using it during the conversion (from AsyncExtension to
> > UIThreadExtensionFunction) to flag this case of the error not being set.
> > 
> > But really I just don't feel like going through all of the callbacks right
now
> > to determine all the different possible errors and handling them. If I did,
it
> > would be as a follow-up.
> 
> This was the only one I saw, but I can see there is one other use about 40
lines
> up.
> 
> I was just thinking you'd add kCouldntGetFileInfo error for this one. The same
> error would probably work for the other use as well.

Done.

> 
> But I'm not too fussed. If you want to make patches smaller and easier to
review
> in future though, it would have been nice not to have the extension function
> conversion mixed in with moving the API, if that was possible.

Acknowledged -- that could've been done beforehand.

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc
(right):

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc:258:
&ChromeFileSystemDelegate::OnConsentReceived, base::Unretained(this),
On 2017/06/14 14:53:22, Devlin wrote:
> document why this unretained is safe

Maybe it's not? Calling RequestConsent below may trigger a dialog which might
call this callback *eventually*. Is it safe to assume the FileSystemDelegate
outlives the web contents' dialog?

In the old code, |this| was "safe" because the ExtensionFunction is ref
counted... hrm. We could go back to the idea of using a static callback which
tries to get this delegate and does nothing if it finds a nullptr.

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc:290:
error_callback.Run("");
On 2017/06/14 14:53:22, Devlin wrote:
> I know this was just copied, but prefer std::string() over "".

Done.

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/chrome_file_system_delegate.h
(right):

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.h:38: 
On 2017/06/14 14:53:22, Devlin wrote:
> // FileSystemDelegate:

Done.

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.h:72: //
system. May prompt the user for consent.
On 2017/06/14 14:53:22, Devlin wrote:
> Shouldn't need comments for overridden methods.

Done.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:135:
delegate->GetDescriptionIdForAcceptType(accept_type, &description_id);
On 2017/06/14 14:53:22, Devlin wrote:
> Shouldn't we check the return value?

No reason to. We're still following the same logic as before; if description_id
is unset it just stays 0.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:695:
base::Bind(&FileSystemChooseEntryFunction::OnDirectoryAccessConfirmed,
On 2017/06/14 14:53:22, Devlin wrote:
> Could these be BindOnce's?

I had tried, but this triggers through a looong chain of RepeatingCallbacks. (A
OnceCallback can't be cast to a RepeatingCallback so it's like "const" -- they'd
all have to be updated.) I'll let that sort itself out when they do the mass
conversion.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:883: auto
saved_files_service_delegate =
On 2017/06/14 14:53:22, Devlin wrote:
> nit: don't use auto here - it's not clear what the return result will be
(i.e.,
> std::unique_ptr<>, stack allocated, etc.

Done.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:902: return
RespondNow(Error(kUnknownErrorDoNotUse));
On 2017/06/14 14:53:22, Devlin wrote:
> Can we think of an error here?

Done.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:939:
Respond(Error(kUnknownErrorDoNotUse));
On 2017/06/14 14:53:22, Devlin wrote:
> ditto re error

Done.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:947: auto
saved_files_service_delegate =
On 2017/06/14 14:53:22, Devlin wrote:
> ditto re auto

Done.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:964: auto
saved_files_service_delegate =
On 2017/06/14 14:53:22, Devlin wrote:
> ditto re auto (apply to the rest of the file :))

Done.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.h (right):

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.h:169: content::WebContents*
web_contents);
On 2017/06/14 14:53:22, Devlin wrote:
> \n

Done.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.h:173: content::WebContents*
web_contents);
On 2017/06/14 14:53:22, Devlin wrote:
> \n

Done.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_delegate.h (right):

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_delegate.h:55: virtual
std::unique_ptr<ui::SelectFilePolicy> CreateSelectFilePolicy(
On 2017/06/14 14:53:22, Devlin wrote:
> add function comments

Done.

[Devlin, 2017-06-21 14:53:30]

nice!  A few last comments.

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc
(right):

https://codereview.chromium.org/2934143002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc:258:
&ChromeFileSystemDelegate::OnConsentReceived, base::Unretained(this),
On 2017/06/21 00:40:07, michaelpg wrote:
> On 2017/06/14 14:53:22, Devlin wrote:
> > document why this unretained is safe
> 
> Maybe it's not? Calling RequestConsent below may trigger a dialog which might
> call this callback *eventually*. Is it safe to assume the FileSystemDelegate
> outlives the web contents' dialog?
> 
> In the old code, |this| was "safe" because the ExtensionFunction is ref
> counted... hrm. We could go back to the idea of using a static callback which
> tries to get this delegate and does nothing if it finds a nullptr.

I don't know the lifetime of the dialog, but I wouldn't necessarily count on it
being safe if you can't prove it.  Two options:
- Make OnConsentReceived static, like you suggest.  But I don't know that you'd
necessarily even have to get the delegate - I don't see anything in
OnConsentReceived that requires an instance of the delegate to run.  Am I
missing anything?
- Give the delegate a WeakPtrFactory and use that.  If the delegate is destroyed
before that point, then it's almost certainly during shutdown and we don't
really care about responding.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:135:
delegate->GetDescriptionIdForAcceptType(accept_type, &description_id);
On 2017/06/21 00:40:07, michaelpg wrote:
> On 2017/06/14 14:53:22, Devlin wrote:
> > Shouldn't we check the return value?
> 
> No reason to. We're still following the same logic as before; if
description_id
> is unset it just stays 0.

In that case, should we have GetDescriptionIdForAcceptType be void, or return
the id directly?  It's misleading to have a bool method indicating success and
not check the result.

https://codereview.chromium.org/2934143002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc
(right):

https://codereview.chromium.org/2934143002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc:199:
return consent_provider.IsGrantable(extension);
optional nit: inlineable:
return ConsentProvider(&consent_provider_delegate).IsGrantable(extension);

https://codereview.chromium.org/2934143002/diff/100001/extensions/browser/api...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/100001/extensions/browser/api...
extensions/browser/api/file_system/file_system_api.cc:71: // using
apps::SavedFilesService;
remove?

[michaelpg, 2017-06-23 21:30:53]

PTAL. Surprisingly the diffs to previous patchsets are quite tame, even after
rebasing and splitting out the SavedFilesService stuff.

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/80001/extensions/browser/api/...
extensions/browser/api/file_system/file_system_api.cc:135:
delegate->GetDescriptionIdForAcceptType(accept_type, &description_id);
On 2017/06/21 14:53:30, Devlin wrote:
> On 2017/06/21 00:40:07, michaelpg wrote:
> > On 2017/06/14 14:53:22, Devlin wrote:
> > > Shouldn't we check the return value?
> > 
> > No reason to. We're still following the same logic as before; if
> description_id
> > is unset it just stays 0.
> 
> In that case, should we have GetDescriptionIdForAcceptType be void, or return
> the id directly?  It's misleading to have a bool method indicating success and
> not check the result.

Done. (I just get uneasy using sentinels.)

https://codereview.chromium.org/2934143002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc
(right):

https://codereview.chromium.org/2934143002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/chrome_file_system_delegate.cc:199:
return consent_provider.IsGrantable(extension);
On 2017/06/21 14:53:30, Devlin wrote:
> optional nit: inlineable:
> return ConsentProvider(&consent_provider_delegate).IsGrantable(extension);

Done.

https://codereview.chromium.org/2934143002/diff/100001/extensions/browser/api...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/100001/extensions/browser/api...
extensions/browser/api/file_system/file_system_api.cc:71: // using
apps::SavedFilesService;
On 2017/06/21 14:53:30, Devlin wrote:
> remove?

Done.

https://codereview.chromium.org/2934143002/diff/180001/extensions/browser/api...
File extensions/browser/api/file_system/file_system_delegate.h (right):

https://codereview.chromium.org/2934143002/diff/180001/extensions/browser/api...
extensions/browser/api/file_system/file_system_delegate.h:109:
CreateSavedFilesServiceInterface(
benwells: Awkward choice between calling this "CreateFooInterface" or calling it
"CreateFoo" but returning an interface. WDYT?

[michaelpg, 2017-06-23 21:34:45]

michaelpg@chromium.org changed reviewers:
+ sky@chromium.org, tommycli@chromium.org

[michaelpg, 2017-06-23 21:34:46]

tommycli: media_galleries files
sky: added "+ui/shell_dialogs" to
  //extensions/browser/api/file_system/DEPS (it's hard to
  avoid using concepts like SelectFilePolicy and
  SelectFileDialog::Listener)

[benwells, 2017-06-26 02:28:15]

lgtm

https://codereview.chromium.org/2934143002/diff/180001/extensions/browser/api...
File extensions/browser/api/file_system/file_system_delegate.h (right):

https://codereview.chromium.org/2934143002/diff/180001/extensions/browser/api...
extensions/browser/api/file_system/file_system_delegate.h:109:
CreateSavedFilesServiceInterface(
On 2017/06/23 21:30:53, michaelpg wrote:
> benwells: Awkward choice between calling this "CreateFooInterface" or calling
it
> "CreateFoo" but returning an interface. WDYT?

I'd go with CreateFoo. The underlying implementation will be a Foo, so I'm OK
with that.

[Devlin, 2017-06-26 15:05:21]

lgtm

[michaelpg, 2017-06-26 15:36:33]

Thanks guys! tommycli, sky, PTAL.

tommycli: media_galleries files
sky: added "+ui/shell_dialogs" to
  //extensions/browser/api/file_system/DEPS (it's hard to
  avoid using concepts like SelectFilePolicy and
  SelectFileDialog::Listener)

https://codereview.chromium.org/2934143002/diff/180001/extensions/browser/api...
File extensions/browser/api/file_system/file_system_delegate.h (right):

https://codereview.chromium.org/2934143002/diff/180001/extensions/browser/api...
extensions/browser/api/file_system/file_system_delegate.h:109:
CreateSavedFilesServiceInterface(
On 2017/06/26 02:28:15, benwells wrote:
> On 2017/06/23 21:30:53, michaelpg wrote:
> > benwells: Awkward choice between calling this "CreateFooInterface" or
calling
> it
> > "CreateFoo" but returning an interface. WDYT?
> 
> I'd go with CreateFoo. The underlying implementation will be a Foo, so I'm OK
> with that.

Yeah, that's better. Done.

[tommycli, 2017-06-26 15:59:03]

media_galleries LGTM.

Feel free to TBR me on trivial changes to media_galleries. I won't get mad. :)

[sky, 2017-06-26 17:16:12]

new DEPS LGTM

[michaelpg, 2017-06-26 18:03:49]

On 2017/06/26 15:59:03, tommycli wrote:
> media_galleries LGTM.
> 
> Feel free to TBR me on trivial changes to media_galleries. I won't get mad. :)

ugh, sometimes I forget TBR exists!

[michaelpg, 2017-06-26 18:04:22]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-06-26 18:04:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2934143002/#ps200001
(title: "benwells/rebase")

[commit-bot: I haz the power, 2017-06-26 18:04:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaelpg, 2017-06-26 18:16:49]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2017-06-26 18:51:31]

The CQ bit was checked by michaelpg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 18:51:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaelpg, 2017-06-26 19:44:15]

The CQ bit was unchecked by michaelpg@chromium.org

[michaelpg, 2017-06-26 20:30:46]

Devlin: I'm having trouble understanding why GetAssociatedWebContents() is
null. The web contents is either gone (but the call happens synchronously!),
being fetched incorrectly, or non-existent (if it's called from a
background page somehow).
I may need to pick your brain tomorrow if I can't get to the bottom of it.

[michaelpg, 2017-06-26 21:49:08]

On 2017/06/26 20:30:46, michaelpg wrote:
> Devlin: I'm having trouble understanding why GetAssociatedWebContents() is
> null. The web contents is either gone (but the call happens synchronously!),
> being fetched incorrectly, or non-existent (if it's called from a
> background page somehow).

So the ChromeExtensionFunctionDetails::GetAssociatedWebContents() has magic
that UIThreadExtensionFunction::GetAssociatedWebContents() doesn't have.

If the dispatcher doesn't have a WebContents, it gets the active
WebContents from the Browser's TabStripModel:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_extensi...

So... my questions for you:

1. Why are Chrome extension functions that can't find an associated WebContents
using the WebContents of the active tab? That might make sense sometimes, but
surely there are extensions running on other tabs or whose background pages
try to do something. IOW, I think a test that calls a function from a background
page, when that function shouldn't be called from a background page, should
fail, but passes in Chrome because there's a tab open.

2. Why do the File Manager app's "integration_tests" run as an extension??
That stops working when we stop using ChromeUIThreadExtensionFunction because
UIThreadExtensionFunction doesn't "cheat" by finding a Browser.

> I may need to pick your brain tomorrow if I can't get to the bottom of it.

[Devlin, 2017-06-26 21:57:52]

On 2017/06/26 21:49:08, michaelpg wrote:
> On 2017/06/26 20:30:46, michaelpg wrote:
> > Devlin: I'm having trouble understanding why GetAssociatedWebContents() is
> > null. The web contents is either gone (but the call happens synchronously!),
> > being fetched incorrectly, or non-existent (if it's called from a
> > background page somehow).
> 
> So the ChromeExtensionFunctionDetails::GetAssociatedWebContents() has magic
> that UIThreadExtensionFunction::GetAssociatedWebContents() doesn't have.
> 
> If the dispatcher doesn't have a WebContents, it gets the active
> WebContents from the Browser's TabStripModel:
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_extensi...
> 
> So... my questions for you:
> 
> 1. Why are Chrome extension functions that can't find an associated
WebContents
> using the WebContents of the active tab? That might make sense sometimes, but
> surely there are extensions running on other tabs or whose background pages
> try to do something. IOW, I think a test that calls a function from a
background
> page, when that function shouldn't be called from a background page, should
> fail, but passes in Chrome because there's a tab open.

Ideally, they shouldn't.  I added GetSenderWebContents() for this exact reason -
GetAssociatedWebContents() returns so many different things at different times
that it's basically random.  I added a note here [1], but it's not as
strongly-worded as I remember.  I think it should actually say "Don't use this,
use GetSenderWebContents() or find your own."  But there's a *ton* of legacy
code that uses these methods.

[1]
https://cs.chromium.org/chromium/src/extensions/browser/extension_function.h?...

> 2. Why do the File Manager app's "integration_tests" run as an extension??
> That stops working when we stop using ChromeUIThreadExtensionFunction because
> UIThreadExtensionFunction doesn't "cheat" by finding a Browser.

Another good question.  I'm not familiar enough with file manager stuff to know
for sure.  Maybe one of the file manager OWNERS would know?

> > I may need to pick your brain tomorrow if I can't get to the bottom of it.

Sure thing - I'll help if I can. :)

[michaelpg, 2017-06-26 23:52:39]

The CQ bit was checked by michaelpg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-26 23:52:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 01:19:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 01:19:30]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2017-06-27 15:34:49]

Haven't solved the test issue yet.

https://codereview.chromium.org/2934143002/diff/240001/extensions/browser/api...
File extensions/browser/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/2934143002/diff/240001/extensions/browser/api...
extensions/browser/api/file_system/file_system_api.cc:508: if
(extension_->is_platform_app() && !web_contents) {
I was attempting to addresses crbug.com/736930 by allow calling this method from
an extension's background page instead of failing... but this causes a CHECK
failure in SelectFileDialogImpl if no WebContents are found.

[michaelpg, 2017-07-05 20:17:45]

On 2017/06/27 15:34:49, michaelpg wrote:
> Haven't solved the test issue yet.
> 
>
https://codereview.chromium.org/2934143002/diff/240001/extensions/browser/api...
> File extensions/browser/api/file_system/file_system_api.cc (right):
> 
>
https://codereview.chromium.org/2934143002/diff/240001/extensions/browser/api...
> extensions/browser/api/file_system/file_system_api.cc:508: if
> (extension_->is_platform_app() && !web_contents) {
> I was attempting to addresses crbug.com/736930 by allow calling this method
from
> an extension's background page instead of failing... but this causes a CHECK
> failure in SelectFileDialogImpl if no WebContents are found.

Made tests pass by restoring the original behavior. The new
FileSystemDelegate::ShowSelectFileDialog method allows Chrome to use its funky
version of GetAssociatedWebContents().

[michaelpg, 2017-07-13 20:54:32]

The CQ bit was checked by michaelpg@chromium.org

[michaelpg, 2017-07-13 20:54:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, rdevlin.cronin@chromium.org, benwells@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2934143002/#ps400001
(title: "rebase")

[commit-bot: I haz the power, 2017-07-13 20:54:51]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-13 22:38:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-13 22:38:32]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[michaelpg, 2017-07-13 22:50:20]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2017-07-13 22:50:47]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-14 01:35:32]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1499986220809200,
"parent_rev": "5573361f7b2cf91b5b4f38d3c277e8687b7a60de", "commit_rev":
"56c27b3401b61598c079687a032fc9a55258dbdb"}

[commit-bot: I haz the power, 2017-07-14 01:35:45]

Description was changed from

==========
Move chrome.fileSystem implementation to //extensions

This ancient API has several Chrome dependencies, which have been moved
into a FileSystemDelegate class that performs the Chrome-specific work.

I also made a delegate for apps::SavedFilesService which is only
implemented in //apps, but allows //extensions to invoke this
apps-specific code without adding a hard dependency.

Since so much of the code was shuffled around anyway, I also ran
`git cl format` on everything.

Future work includes creating an AppShellFileSystemDelegate to make
certain functionality available in app_shell.

BUG=729713
R=benwells@chromium.org,rdevlin.cronin@chromium.org
==========

to

==========
Move chrome.fileSystem implementation to //extensions

This ancient API has several Chrome dependencies, which have been moved
into a FileSystemDelegate class that performs the Chrome-specific work.

I also made a delegate for apps::SavedFilesService which is only
implemented in //apps, but allows //extensions to invoke this
apps-specific code without adding a hard dependency.

Since so much of the code was shuffled around anyway, I also ran
`git cl format` on everything.

Future work includes creating an AppShellFileSystemDelegate to make
certain functionality available in app_shell.

BUG=729713
R=benwells@chromium.org,rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2934143002
Cr-Commit-Position: refs/heads/master@{#486609}
Committed:
https://chromium.googlesource.com/chromium/src/+/56c27b3401b61598c079687a032f...
==========

[commit-bot: I haz the power, 2017-07-14 01:35:48]

Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/56c27b3401b61598c079687a032f...