Files.app: Enable externalfile: protocol for MTP volumes.

Files.app: Enable externalfile: protocol for MTP volumes.

Previously drive paths are used to obtain 'externalfile:' URL and the URL is
converted back to the drive path.

The CL uses FileSystemURL instead of drive paths so that the app can generate
external file URLs for other than drive files, and enables external file URLs
for MTP and FSP volumes.

BUG=367027
TEST=manually

Committed: https://crrev.com/83a448b281c6b3e3344731fae2725c32237f47ec
Cr-Commit-Position: refs/heads/master@{#296901}

[hirono, 2014-09-24 05:53:50]

hirono@chromium.org changed reviewers:
+ mtomasz@chromium.org

[hirono, 2014-09-24 05:53:51]

PTAL the CL? This depends on crrev.com/580023003, and the last patch to series
of externalfle: protocol handler.

[hirono, 2014-09-25 06:28:02]

@mtomasz - All dependency has been submitted. Also I removed drive dependency
from external_file_util as a result of discussing with @kinaba.
PTAL the CL? Thank you!

[mtomasz, 2014-09-25 06:48:09]

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_browser_handlers.cc (right):

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_browser_handlers.cc:454: num_opened +=
util::OpenFileWithBrowser(profile, file_url);
We're adding here a bool to an int. It works, but the code is not clean. Can we
just add ? 1 : 0 or at list a static_cast?

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right):

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/filesystem_api_util.cc:39:
entry->file_specific_info().content_mime_type().empty()) {
Could you please extract it into a separate CL? This is a serious bug.

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/filesystem_api_util.cc:181: // As a
fallback just return failure with an empty mime type value.
nit: The comment doesn't make much sense anymore. Fallback is usually some
alternative solution, but we just clearly return an error, so maybe the word
"fallback" is confusing here?

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:152: if
(drive::util::HasHostedDocumentExtension(file_path)) {
Do we need this "HasHostedDocumentExtension()" check? It works only for Drive,
not other file systems.

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:153: if
(drive::util::IsUnderDriveMountPoint(file_path)) {
Similarly, do we need this IsUnderDriveMountPoint check? Shouldn't we just check
if |url| returned in #155 is empty?

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:90: if
(FileSystemURLToExternalFileURL(file_system_url_).is_empty()) {
It this check needed?

[hirono, 2014-09-25 07:38:08]

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/file_browser_handlers.cc (right):

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/file_browser_handlers.cc:454: num_opened +=
util::OpenFileWithBrowser(profile, file_url);
On 2014/09/25 06:48:09, mtomasz wrote:
> We're adding here a bool to an int. It works, but the code is not clean. Can
we
> just add ? 1 : 0 or at list a static_cast?

Done.

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/filesystem_api_util.cc (right):

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/filesystem_api_util.cc:39:
entry->file_specific_info().content_mime_type().empty()) {
On 2014/09/25 06:48:09, mtomasz wrote:
> Could you please extract it into a separate CL? This is a serious bug.

I got it. Let me separate.

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/filesystem_api_util.cc:181: // As a
fallback just return failure with an empty mime type value.
On 2014/09/25 06:48:09, mtomasz wrote:
> nit: The comment doesn't make much sense anymore. Fallback is usually some
> alternative solution, but we just clearly return an error, so maybe the word
> "fallback" is confusing here?

Let me handle this in the extracted patch.

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:152: if
(drive::util::HasHostedDocumentExtension(file_path)) {
On 2014/09/25 06:48:09, mtomasz wrote:
> Do we need this "HasHostedDocumentExtension()" check? It works only for Drive,
> not other file systems.

Actually, if drive hosted files (e.g. gdoc) are located in Downloads directory,
they can be opened thanks to this check.
Maybe we can remove this after solving crbug.com/414555.

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:153: if
(drive::util::IsUnderDriveMountPoint(file_path)) {
On 2014/09/25 06:48:09, mtomasz wrote:
> Similarly, do we need this IsUnderDriveMountPoint check? Shouldn't we just
check
> if |url| returned in #155 is empty?

If a file is under the drive, we have redirection in DriveBackendDelegate for
the hosted file, and ExternalFileProtocolHandler use it.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

Otherwise, we obtain the hosted URL by parsing the contents of files in
ReadUrlFromGDocOnBlockingPool.

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/fileapi/external_file_url_request_job.cc (right):

https://codereview.chromium.org/589473002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/fileapi/external_file_url_request_job.cc:90: if
(FileSystemURLToExternalFileURL(file_system_url_).is_empty()) {
On 2014/09/25 06:48:09, mtomasz wrote:
> It this check needed?

FileSystemURLToExternalFileURL checks the file system type and returns empty URL
for invalid volume types. (e.g. Downloads, MediaGallery ...)
It prevents us from accessing any volumes of external file system by using
externalfile: scheme.

[kinaba, 2014-09-25 08:15:20]

kinaba@chromium.org changed reviewers:
+ kinaba@chromium.org

[kinaba, 2014-09-25 08:15:21]

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:146: page_url =
net::FilePathToFileURL(file_system_url.path());
nit: file_system_url.path() => file_path

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:160: // Parse the file
to extract the Docs url and open this url.
This path is meaningful only for really local files. That is, for files on FSP
or MTP it won't work. The branch should reflect that, rather than splitting by
Drive and non-Drive.

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/fileapi/external_file_url_util.cc (right):

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:28: const
storage::FileSystemURL& file_system_url) {
Either DCHECK or runtime check for
  file_system_url.mount_type() == kFileSystemTypeExternal
may be necessary. URLs mounted in isolated context have different virtual_path
hence don't work.

[hirono, 2014-09-25 09:00:04]

Thanks!

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:146: page_url =
net::FilePathToFileURL(file_system_url.path());
On 2014/09/25 08:15:21, kinaba wrote:
> nit: file_system_url.path() => file_path

Done.

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:160: // Parse the file
to extract the Docs url and open this url.
On 2014/09/25 08:15:21, kinaba wrote:
> This path is meaningful only for really local files. That is, for files on FSP
> or MTP it won't work. The branch should reflect that, rather than splitting by
> Drive and non-Drive.

Done.

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/fileapi/external_file_url_util.cc (right):

https://codereview.chromium.org/589473002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:28: const
storage::FileSystemURL& file_system_url) {
On 2014/09/25 08:15:21, kinaba wrote:
> Either DCHECK or runtime check for
>   file_system_url.mount_type() == kFileSystemTypeExternal
> may be necessary. URLs mounted in isolated context have different virtual_path
> hence don't work.

Done.

[kinaba, 2014-09-25 09:10:28]

lgtm

[mtomasz, 2014-09-25 09:53:11]

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:157: // drive's web
interface. Otherwise (e.g. MTP, FSP), the file is mrely
typo: mrely -> ?

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/fileapi/external_file_url_util.cc (right):

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:58: if
(!file_manager::util::ConvertAbsoluteFilePathToFileSystemUrl(
Why do we return the file system url as GURL instead of FileSystemURL?

[hirono, 2014-09-25 11:01:53]

Thank you!

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/open_with_browser.cc (right):

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/open_with_browser.cc:157: // drive's web
interface. Otherwise (e.g. MTP, FSP), the file is mrely
On 2014/09/25 09:53:11, mtomasz wrote:
> typo: mrely -> ?

Done.

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/fileapi/external_file_url_util.cc (right):

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:58: if
(!file_manager::util::ConvertAbsoluteFilePathToFileSystemUrl(
On 2014/09/25 09:53:11, mtomasz wrote:
> Why do we return the file system url as GURL instead of FileSystemURL?

Not sure, but in other usages, FileSystemURL is not always needed. And other
utility function in file_manager::util also does not use FileSystemURL.

So I'm guessing this is because:

1. historical reason
2. for consistency

I found another usage to convert GURL to FileSystemURL after calling
ConvertAbsoluteFilePathToFileSystemUrl. So cracking the URL here is not a
problem.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

If you thin we should replace the GURL with FileSystemURL, Can I handle it in
separated patch? The function is also used in other places.

[mtomasz, 2014-09-26 00:27:13]

lgtm

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/fileapi/external_file_url_util.cc (right):

https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:58: if
(!file_manager::util::ConvertAbsoluteFilePathToFileSystemUrl(
On 2014/09/25 11:01:53, hirono wrote:
> On 2014/09/25 09:53:11, mtomasz wrote:
> > Why do we return the file system url as GURL instead of FileSystemURL?
> 
> Not sure, but in other usages, FileSystemURL is not always needed. And other
> utility function in file_manager::util also does not use FileSystemURL.
> 
> So I'm guessing this is because:
> 
> 1. historical reason
> 2. for consistency
> 
> I found another usage to convert GURL to FileSystemURL after calling
> ConvertAbsoluteFilePathToFileSystemUrl. So cracking the URL here is not a
> problem.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> 
> If you thin we should replace the GURL with FileSystemURL, Can I handle it in
> separated patch? The function is also used in other places.

Ah got it. In general I think we should avoid creating FileSystemURL manually by
string concatenation, since we have better way to do that, not relying on the
format. Since all of the utility functions are like that I think we don't need
to change it anytime soon.

[mtomasz, 2014-09-26 00:28:32]

On 2014/09/26 00:27:13, mtomasz wrote:
> lgtm
> 
>
https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
> File chrome/browser/chromeos/fileapi/external_file_url_util.cc (right):
> 
>
https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
> chrome/browser/chromeos/fileapi/external_file_url_util.cc:58: if
> (!file_manager::util::ConvertAbsoluteFilePathToFileSystemUrl(
> On 2014/09/25 11:01:53, hirono wrote:
> > On 2014/09/25 09:53:11, mtomasz wrote:
> > > Why do we return the file system url as GURL instead of FileSystemURL?
> > 
> > Not sure, but in other usages, FileSystemURL is not always needed. And other
> > utility function in file_manager::util also does not use FileSystemURL.
> > 
> > So I'm guessing this is because:
> > 
> > 1. historical reason
> > 2. for consistency
> > 
> > I found another usage to convert GURL to FileSystemURL after calling
> > ConvertAbsoluteFilePathToFileSystemUrl. So cracking the URL here is not a
> > problem.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > 
> > If you thin we should replace the GURL with FileSystemURL, Can I handle it
in
> > separated patch? The function is also used in other places.
> 
> Ah got it. In general I think we should avoid creating FileSystemURL manually
by
> string concatenation, since we have better way to do that, not relying on the
> format. Since all of the utility functions are like that I think we don't need
> to change it anytime soon.

This CL seems to break tests. Please check.

[mtomasz, 2014-09-26 00:28:32]

On 2014/09/26 00:27:13, mtomasz wrote:
> lgtm
> 
>
https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
> File chrome/browser/chromeos/fileapi/external_file_url_util.cc (right):
> 
>
https://codereview.chromium.org/589473002/diff/220001/chrome/browser/chromeos...
> chrome/browser/chromeos/fileapi/external_file_url_util.cc:58: if
> (!file_manager::util::ConvertAbsoluteFilePathToFileSystemUrl(
> On 2014/09/25 11:01:53, hirono wrote:
> > On 2014/09/25 09:53:11, mtomasz wrote:
> > > Why do we return the file system url as GURL instead of FileSystemURL?
> > 
> > Not sure, but in other usages, FileSystemURL is not always needed. And other
> > utility function in file_manager::util also does not use FileSystemURL.
> > 
> > So I'm guessing this is because:
> > 
> > 1. historical reason
> > 2. for consistency
> > 
> > I found another usage to convert GURL to FileSystemURL after calling
> > ConvertAbsoluteFilePathToFileSystemUrl. So cracking the URL here is not a
> > problem.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > 
> > If you thin we should replace the GURL with FileSystemURL, Can I handle it
in
> > separated patch? The function is also used in other places.
> 
> Ah got it. In general I think we should avoid creating FileSystemURL manually
by
> string concatenation, since we have better way to do that, not relying on the
> format. Since all of the utility functions are like that I think we don't need
> to change it anytime soon.

This CL seems to break tests. Please check.

[hirono, 2014-09-26 03:50:08]

hirono@chromium.org changed reviewers:
+ tzik@chromium.org

[hirono, 2014-09-26 03:50:09]

@tzik - PTAL the CL?
I need a new factory function for FileSystemURL to generated fake cracked
FileSystemURL for testing.

[tzik, 2014-09-26 06:47:33]

lgtm

[hirono, 2014-09-26 06:48:22]

The CQ bit was checked by hirono@chromium.org

[hirono, 2014-09-26 06:48:40]

Thank you!

[commit-bot: I haz the power, 2014-09-26 06:49:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/589473002/260001

[commit-bot: I haz the power, 2014-09-26 06:53:45]

Committed patchset #14 (id:260001) as ef6a9df4ca2d32b1375f2c49f1b8ef17c1cfbd6d

[commit-bot: I haz the power, 2014-09-26 06:54:21]

Patchset 14 (id:??) landed as
https://crrev.com/83a448b281c6b3e3344731fae2725c32237f47ec
Cr-Commit-Position: refs/heads/master@{#296901}