Rename the drive: scheme with the externalfile: scheme.

Rename the drive: scheme with the externalfile: scheme.

The CL rename following symbols.

* kDriveScheme -> kExternalFileScheme
* drive::DriveProtocolHandler -> chromeos::ExternalFileProtocolHandler
* drive::DriveURLRequestJob -> chromeos::ExternalFileURLRequestJob
* Utility functions in drive/file_system_util.{h,cc} -> fileapi/external_file_url_util.{h,cc}
* Related tests

BUG=367027
TEST=Open HTML, MHTML, PDF, GDOC files on the drive volume.

Committed: https://crrev.com/90949afe82b9f509e09dcde95700a3839a04f7db
Cr-Commit-Position: refs/heads/master@{#296642}

[hirono, 2014-09-18 11:31:29]

Patchset #1 (id:1) has been deleted

[hirono, 2014-09-18 11:45:03]

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

[hirono, 2014-09-18 11:45:04]

PTAL the CL? The CL depend on the previous CL (crrev.com/560313002).

@kinaba - chrome/browser/chromeos/drive/*
@mtomasz - chrome/browser/chromeos/fileapi/file_system_backend.cc

Thank you!

[mtomasz, 2014-09-19 00:22:06]

On 2014/09/18 11:45:04, hirono wrote:
> PTAL the CL? The CL depend on the previous CL (crrev.com/560313002).
> 
> @kinaba - chrome/browser/chromeos/drive/*
> @mtomasz - chrome/browser/chromeos/fileapi/file_system_backend.cc
> 
> Thank you!

file_system_backend.cc lgtm

[kinaba, 2014-09-19 01:56:27]

https://codereview.chromium.org/580023003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/file_system_util_unittest.cc (right):

https://codereview.chromium.org/580023003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/file_system_util_unittest.cc:1: // Copyright 2014
The Chromium Authors. All rights reserved.
I think you can keep the year as is

[kinaba, 2014-09-19 01:56:35]

lgtm

[hirono, 2014-09-19 07:36:28]

On 2014/09/19 01:56:35, kinaba wrote:
> lgtm

@mtomasz - I added the test to parse filesystem:externalfile:/. Also could you
take a look chrome/browser/chromeos/fileapi/* other than file_system_backend.cc
(sorry for misdescription in the previous request) and
chrome/browser/chromeos/file_manager/open_with_browser.cc? Thank you!

[mtomasz, 2014-09-22 01:58:48]

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

https://codereview.chromium.org/580023003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:38: if
(!drive::util::IsUnderDriveMountPoint(path))
I'm wondering if we should have a dependency to drive here. This file
responsibility is to create an external file url, right? Maybe we should have
this check just when we call the function? WDYT?

https://codereview.chromium.org/580023003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/fileapi/external_file_url_util.h (right):

https://codereview.chromium.org/580023003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/fileapi/external_file_url_util.h:23: // Overwrites |url|
with an external file URL when appropriate.
When is it appropriate? Could you please describe when the url will be replaced?
Also, how about simplifying the interface?

GURL CreateExternalFileURL(Profile* profile, const base::FilePath);

[hirono, 2014-09-22 03:32:20]

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

https://codereview.chromium.org/580023003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:38: if
(!drive::util::IsUnderDriveMountPoint(path))
On 2014/09/22 01:58:48, mtomasz wrote:
> I'm wondering if we should have a dependency to drive here. This file
> responsibility is to create an external file url, right? Maybe we should have
> this check just when we call the function? WDYT?

I'll update the function at the next patch, but I'll still use the drive related
code there.
https://codereview.chromium.org/589473002/diff/20001/chrome/browser/chromeos/...

Currently the function masks drive hash with 'drive' prefix (e.g.
'filesystem:origin/drive-xxxx/root/test.txt' ->
'externalfile:drive/root/test.txt') as well as the existing drive: scheme. If I
stop this, I can remove the drive related code. Masking drive hash is @kinaba's
suggestion, so let me discuss with him.

https://codereview.chromium.org/580023003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/fileapi/external_file_url_util.h (right):

https://codereview.chromium.org/580023003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/fileapi/external_file_url_util.h:23: // Overwrites |url|
with an external file URL when appropriate.
On 2014/09/22 01:58:48, mtomasz wrote:
> When is it appropriate? Could you please describe when the url will be
replaced?
> Also, how about simplifying the interface?
> 
> GURL CreateExternalFileURL(Profile* profile, const base::FilePath);

I updated the function signature and comments. Thank you!

[mtomasz, 2014-09-22 05:03:32]

lgtm

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

https://codereview.chromium.org/580023003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/fileapi/external_file_url_util.cc:38: if
(!drive::util::IsUnderDriveMountPoint(path))
On 2014/09/22 03:32:19, hirono wrote:
> On 2014/09/22 01:58:48, mtomasz wrote:
> > I'm wondering if we should have a dependency to drive here. This file
> > responsibility is to create an external file url, right? Maybe we should
have
> > this check just when we call the function? WDYT?
> 
> I'll update the function at the next patch, but I'll still use the drive
related
> code there.
>
https://codereview.chromium.org/589473002/diff/20001/chrome/browser/chromeos/...
> 
> Currently the function masks drive hash with 'drive' prefix (e.g.
> 'filesystem:origin/drive-xxxx/root/test.txt' ->
> 'externalfile:drive/root/test.txt') as well as the existing drive: scheme. If
I
> stop this, I can remove the drive related code. Masking drive hash is
@kinaba's
> suggestion, so let me discuss with him.
> 

Got it.

[hirono, 2014-09-22 06:21:33]

PTAL the CL?

@davemoore

 * chrome/browser/profiles/profile_io_data.cc?

@sky

 * chrome/browser/ui/browser.cc
 * chrome/browser/ui/location_bar/origin_chip_info.cc
 * chrome/renderer/chrome_content_renderer_client.cc

Thank you!

[hirono, 2014-09-22 06:22:04]

hirono@chromium.org changed reviewers:
+ davemoore@chromium.org, sky@chromium.org

[hirono, 2014-09-22 06:22:05]

PTAL the CL?

@davemoore

 * chrome/browser/profiles/profile_io_data.cc?

@sky

 * chrome/browser/ui/browser.cc
 * chrome/browser/ui/location_bar/origin_chip_info.cc
 * chrome/renderer/chrome_content_renderer_client.cc

Thank you!

[sky, 2014-09-22 16:44:19]

https://codereview.chromium.org/580023003/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/580023003/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1927: const GURL external_url =
Is there a reason this is conditional? And do you really want 1924 if chromeos?

[hirono, 2014-09-23 09:25:17]

https://codereview.chromium.org/580023003/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/580023003/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1927: const GURL external_url =
On 2014/09/22 16:44:18, sky wrote:
> Is there a reason this is conditional? And do you really want 1924 if
chromeos?

Yes, In Chrome OS, some volumes are mapped to native directories (e.g.
Downloads, USB storages), and in that case we don't need to use externalfile://
scheme URL. For non-native volumes (e.g. Google Drive, MTP, FSP) we access the
files by using externalfile:// URL.

The original function MaybeSetDriveURL replaces the URL for drive volumes, and
keeps the file:// URL for other types of volumes.

[sky, 2014-09-23 13:20:05]

Ok, LGTM

[hirono, 2014-09-24 04:50:29]

@DaveMoore - friendly ping, PTAL chrome/browser/profiles/profile_io_data.cc?

[hirono, 2014-09-25 03:20:54]

hirono@chromium.org changed reviewers:
+ mmenke@chromium.org
- davemoore@chromium.org

[hirono, 2014-09-25 03:20:55]

It looks there are primary reviewers for ProfileIOData.
@mmenke - PTAL profile_io_data.cc? Thank you.

[mmenke, 2014-09-25 03:28:37]

On 2014/09/25 03:20:55, hirono wrote:
> It looks there are primary reviewers for ProfileIOData.
> @mmenke - PTAL profile_io_data.cc? Thank you.

profiles/ LGTM

[hirono, 2014-09-25 03:30:29]

The CQ bit was checked by hirono@chromium.org

[hirono, 2014-09-25 03:30:35]

On 2014/09/25 03:28:37, mmenke wrote:
> On 2014/09/25 03:20:55, hirono wrote:
> > It looks there are primary reviewers for ProfileIOData.
> > @mmenke - PTAL profile_io_data.cc? Thank you.
> 
> profiles/ LGTM

Thank you!

[commit-bot: I haz the power, 2014-09-25 03:30:50]

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

[commit-bot: I haz the power, 2014-09-25 04:23:45]

Committed patchset #5 (id:100001) as 4487b88fa1da07022be61d9095a6b15a356948f3

[commit-bot: I haz the power, 2014-09-25 04:24:47]

Patchset 5 (id:??) landed as
https://crrev.com/90949afe82b9f509e09dcde95700a3839a04f7db
Cr-Commit-Position: refs/heads/master@{#296642}