Move Media Galleries FileAPI code out of webkit.

Move Media Galleries FileAPI code out of webkit.

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

[Avi (use Gerrit), 2013-04-22 20:41:28]

lgtm

stamp

[vandebo (ex-Chrome), 2013-04-22 22:32:28]

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:2116: additional_providers) {
nit: fits on previous line.

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/media_galle...
File
chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.h
(right):

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.h:49:
GetCopyOrMoveFileValidatorFactory(
nit: I think this should line up with virtual

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc (right):

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc:56:
filesystem_id, &device_path)) {
nit: four more spaces.

https://codereview.chromium.org/14247034/diff/4001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_context.h (right):

https://codereview.chromium.org/14247034/diff/4001/webkit/fileapi/file_system...
webkit/fileapi/file_system_context.h:245: friend class
chrome::NativeMediaFileUtilTest;
Is this still needed?  Not sure this is Kosher.

[tommycli, 2013-04-22 23:45:31]

kinuko: Need OWNER's review on fileapi stuff.
thestig: Need OWNER's review on chrome/chrome_browser.gypi

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:2116: additional_providers) {
On 2013/04/22 22:32:28, vandebo wrote:
> nit: fits on previous line.

Done.

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/media_galle...
File
chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.h
(right):

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/media_file_system_mount_point_provider.h:49:
GetCopyOrMoveFileValidatorFactory(
On 2013/04/22 22:32:28, vandebo wrote:
> nit: I think this should line up with virtual

Done.

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc (right):

https://codereview.chromium.org/14247034/diff/4001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc:56:
filesystem_id, &device_path)) {
On 2013/04/22 22:32:28, vandebo wrote:
> nit: four more spaces.

Done.

https://codereview.chromium.org/14247034/diff/4001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_context.h (right):

https://codereview.chromium.org/14247034/diff/4001/webkit/fileapi/file_system...
webkit/fileapi/file_system_context.h:245: friend class
chrome::NativeMediaFileUtilTest;
On 2013/04/22 22:32:28, vandebo wrote:
> Is this still needed?  Not sure this is Kosher.

It's gross, but still needed. We could eliminate this if we made GetFileUtil
public. I can do that in a separate CL if desired.

[Lei Zhang, 2013-04-23 00:57:34]

lgtm

[kinuko, 2013-04-23 03:53:22]

(+jam)

lgtm, please wait for jam@'s review for the change in
content/public/browser/content_browser_client.h.

Thx for making this huge cleanup happen!

[kinuko, 2013-04-23 03:53:39]

(leftover comments)

https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
content/public/browser/content_browser_client.h:505: virtual void
GetAdditionalFileSystemMountPointProviders(
I forgot to include it in my original CL but we may also want to pass
'is_incognito' flag to this method?

[tommycli, 2013-04-23 17:00:22]

On 2013/04/23 03:53:39, kinuko wrote:
> (leftover comments)
> 
>
https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
> File content/public/browser/content_browser_client.h (right):
> 
>
https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
> content/public/browser/content_browser_client.h:505: virtual void
> GetAdditionalFileSystemMountPointProviders(
> I forgot to include it in my original CL but we may also want to pass
> 'is_incognito' flag to this method?

Thanks kinuko. I'll add is_incognito on a separate CL.

[tommycli, 2013-04-23 17:01:31]

https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
content/public/browser/content_browser_client.h:505: virtual void
GetAdditionalFileSystemMountPointProviders(
On 2013/04/23 03:53:39, kinuko wrote:
> I forgot to include it in my original CL but we may also want to pass
> 'is_incognito' flag to this method?

I'll add in separate CL.

[jam, 2013-04-23 17:21:39]

lgtm

https://codereview.chromium.org/14247034/diff/10002/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/14247034/diff/10002/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:2116:
DCHECK(additional_providers);
nit: skip this. if it's null, the crash on the next line is just as useful of a
signal

https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
content/public/browser/content_browser_client.h:506: const base::FilePath&
partition_path,
nit: to be clearer, I would name this storage_partition_path

[commit-bot: I haz the power, 2013-04-23 17:27:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/14247034/25001

[tommycli, 2013-04-23 17:37:01]

https://codereview.chromium.org/14247034/diff/10002/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/14247034/diff/10002/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:2116:
DCHECK(additional_providers);
On 2013/04/23 17:21:39, jam wrote:
> nit: skip this. if it's null, the crash on the next line is just as useful of
a
> signal

Done.

https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/14247034/diff/10002/content/public/browser/co...
content/public/browser/content_browser_client.h:506: const base::FilePath&
partition_path,
On 2013/04/23 17:21:39, jam wrote:
> nit: to be clearer, I would name this storage_partition_path

Done.

[commit-bot: I haz the power, 2013-04-24 02:05:45]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-04-24 04:57:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/14247034/25001

[commit-bot: I haz the power, 2013-04-24 19:29:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/14247034/57001

[commit-bot: I haz the power, 2013-04-24 23:52:55]

Change committed as 196265