Picasa import: MediaFileSystemRegistry and misc. plumbing

Add Picasa import hooks into MediaFileSystemRegistry.

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

[tommycli, 2013-05-22 00:56:22]

This was all that was left to get Picasa loading as a gallery in a "proof of
concept" form on Windows.

Still need to do sandboxing of the database reading...

https://codereview.chromium.org/15511009/diff/1/content/browser/fileapi/filea...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15511009/diff/1/content/browser/fileapi/filea...
content/browser/fileapi/fileapi_message_filter.cc:755: //   - Picasa filesystems
The Picasa filesystem has both filesystem read-access and the database_path'
read-access granted correctly.

The code still gets here though, since the database_path is not a parent of the
image paths.

According to the comments, this happens whenever the partition path (path of the
image) differs from the nominal path (path of the database).

[vandebo (ex-Chrome), 2013-05-22 15:05:49]

https://codereview.chromium.org/15511009/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/15511009/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/media_file_system_registry.cc:338: switch (type)
{
I'd prefer to do this switch in
MediaFileSystemContext::RegisterFileSystemForMassStorage

https://codereview.chromium.org/15511009/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/media_file_system_registry.cc:642: 
nit: extra line.

https://codereview.chromium.org/15511009/diff/1/chrome/browser/storage_monito...
File chrome/browser/storage_monitor/storage_info.cc (left):

https://codereview.chromium.org/15511009/diff/1/chrome/browser/storage_monito...
chrome/browser/storage_monitor/storage_info.cc:129: type == ITUNES ||
In one sense, iTunes and Picasa aren't mass storage, since they are virtual file
systems, but in another sense they are mass storage because all the data can be
accessed through a regular file interface (unlike MTP and ICC).  Until there's
some place that we need to treat iTunes and Picasa differently from everything
else, but the same as each other, I think we should consider them mass storage.
Thoughts?

[tommycli, 2013-05-22 18:03:09]

Making it behave the same way as mass storage was much less code.

https://codereview.chromium.org/15511009/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/15511009/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/media_file_system_registry.cc:338: switch (type)
{
On 2013/05/22 15:05:49, vandebo wrote:
> I'd prefer to do this switch in
> MediaFileSystemContext::RegisterFileSystemForMassStorage

Done.

https://codereview.chromium.org/15511009/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/media_file_system_registry.cc:642: 
On 2013/05/22 15:05:49, vandebo wrote:
> nit: extra line.

Done.

https://codereview.chromium.org/15511009/diff/1/chrome/browser/storage_monito...
File chrome/browser/storage_monitor/storage_info.cc (left):

https://codereview.chromium.org/15511009/diff/1/chrome/browser/storage_monito...
chrome/browser/storage_monitor/storage_info.cc:129: type == ITUNES ||
On 2013/05/22 15:05:49, vandebo wrote:
> In one sense, iTunes and Picasa aren't mass storage, since they are virtual
file
> systems, but in another sense they are mass storage because all the data can
be
> accessed through a regular file interface (unlike MTP and ICC).  Until there's
> some place that we need to treat iTunes and Picasa differently from everything
> else, but the same as each other, I think we should consider them mass
storage.
> Thoughts?

Done.

https://codereview.chromium.org/15511009/diff/1/content/browser/fileapi/filea...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15511009/diff/1/content/browser/fileapi/filea...
content/browser/fileapi/fileapi_message_filter.cc:755: //   - Picasa filesystems
On 2013/05/22 00:56:22, tommycli wrote:
> The Picasa filesystem has both filesystem read-access and the database_path'
> read-access granted correctly.
> 
> The code still gets here though, since the database_path is not a parent of
the
> image paths.
> 
> According to the comments, this happens whenever the partition path (path of
the
> image) differs from the nominal path (path of the database).

Done.

[vandebo (ex-Chrome), 2013-05-22 18:21:48]

https://codereview.chromium.org/15511009/diff/5002/chrome/browser/media_galle...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/15511009/diff/5002/chrome/browser/media_galle...
chrome/browser/media_galleries/media_file_system_registry.cc:582:
CHECK(StorageInfo::CrackDeviceId(device_id, &type, NULL));
We should probably leave this abstraction intact.  Please add methods:
IsPicasaDevice(device_id) etc to StorageInfo.

[vandebo (ex-Chrome), 2013-05-22 19:48:47]

LGTM

[commit-bot: I haz the power, 2013-05-22 19:53:57]

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

[tommycli, 2013-05-22 21:21:43]

kinuko: Need OWNER review on content/browser/fileapi/fileapi_message_filter.cc.

[commit-bot: I haz the power, 2013-05-22 23:01:11]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[kinuko, 2013-05-23 03:51:57]

lgtm with one request

https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:759: url.type() ==
fileapi::kFileSystemTypePicasa);
I think you could also return non-null snapshot_file from
PicasaFileUtil::CreateSnapshotFile (with DONT_DELETE_ON_SCOPE_OUT) to let the
existing code grant the permission. If this approach works for you do you mind
making a follow-up change to remove this change?

[tommycli, 2013-05-23 17:27:15]

https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:759: url.type() ==
fileapi::kFileSystemTypePicasa);
On 2013/05/23 03:51:58, kinuko wrote:
> I think you could also return non-null snapshot_file from
> PicasaFileUtil::CreateSnapshotFile (with DONT_DELETE_ON_SCOPE_OUT) to let the
> existing code grant the permission. If this approach works for you do you mind
> making a follow-up change to remove this change?

Yes, I will do that.

Why is it that IsolatedFileUtil returns an empty ScopedFile and PicasaFileUtil
and ITunesFileUtil needs to return a non-null snapshot file? I'm trying to
understand concept of the snapshot file.

[commit-bot: I haz the power, 2013-05-23 17:28:06]

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

[commit-bot: I haz the power, 2013-05-23 17:33:25]

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

[commit-bot: I haz the power, 2013-05-23 17:43:55]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-23 18:04:33]

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

[commit-bot: I haz the power, 2013-05-23 22:54:17]

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

[commit-bot: I haz the power, 2013-05-23 22:59:11]

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

[tommycli, 2013-05-23 23:00:05]

https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
content/browser/fileapi/fileapi_message_filter.cc:759: url.type() ==
fileapi::kFileSystemTypePicasa);
On 2013/05/23 17:27:15, tommycli wrote:
> On 2013/05/23 03:51:58, kinuko wrote:
> > I think you could also return non-null snapshot_file from
> > PicasaFileUtil::CreateSnapshotFile (with DONT_DELETE_ON_SCOPE_OUT) to let
the
> > existing code grant the permission. If this approach works for you do you
mind
> > making a follow-up change to remove this change?
> 
> Yes, I will do that.
> 
> Why is it that IsolatedFileUtil returns an empty ScopedFile and PicasaFileUtil
> and ITunesFileUtil needs to return a non-null snapshot file? I'm trying to
> understand concept of the snapshot file.

Talked to Steve. Seems better to add an explicit exception here rather than
creating a fake FileSnapshot. We circumvent the security system either way, but
it's more explicit here. Thoughts?

[kinuko, 2013-05-24 02:03:38]

On 2013/05/23 17:27:15, tommycli wrote:
>
https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
> File content/browser/fileapi/fileapi_message_filter.cc (right):
> 
>
https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
> content/browser/fileapi/fileapi_message_filter.cc:759: url.type() ==
> fileapi::kFileSystemTypePicasa);
> On 2013/05/23 03:51:58, kinuko wrote:
> > I think you could also return non-null snapshot_file from
> > PicasaFileUtil::CreateSnapshotFile (with DONT_DELETE_ON_SCOPE_OUT) to let
the
> > existing code grant the permission. If this approach works for you do you
mind
> > making a follow-up change to remove this change?
> 
> Yes, I will do that.
> 
> Why is it that IsolatedFileUtil returns an empty ScopedFile and PicasaFileUtil
> and ITunesFileUtil needs to return a non-null snapshot file? I'm trying to
> understand concept of the snapshot file.

IsolatedFileUtil pre-grants permission to a selected folder. Hmm.. in your case
maybe you can do the same?  Where the actual picasa/itunes files exist?  It's
probably ok to just grant a blanket permission to the particular folder when
those FileSystems are returned to an app?

[vandebo (ex-Chrome), 2013-05-24 03:46:48]

On 2013/05/24 02:03:38, kinuko wrote:
> On 2013/05/23 17:27:15, tommycli wrote:
> >
>
https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
> > File content/browser/fileapi/fileapi_message_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/15511009/diff/8002/content/browser/fileapi/fi...
> > content/browser/fileapi/fileapi_message_filter.cc:759: url.type() ==
> > fileapi::kFileSystemTypePicasa);
> > On 2013/05/23 03:51:58, kinuko wrote:
> > > I think you could also return non-null snapshot_file from
> > > PicasaFileUtil::CreateSnapshotFile (with DONT_DELETE_ON_SCOPE_OUT) to let
> the
> > > existing code grant the permission. If this approach works for you do you
> mind
> > > making a follow-up change to remove this change?
> > 
> > Yes, I will do that.
> > 
> > Why is it that IsolatedFileUtil returns an empty ScopedFile and
PicasaFileUtil
> > and ITunesFileUtil needs to return a non-null snapshot file? I'm trying to
> > understand concept of the snapshot file.
> 
> IsolatedFileUtil pre-grants permission to a selected folder. Hmm.. in your
case
> maybe you can do the same?  Where the actual picasa/itunes files exist?  It's
> probably ok to just grant a blanket permission to the particular folder when
> those FileSystems are returned to an app?

Picasa and iTunes files can be scattered across the whole disk.  The other
optiosn are to iterate all the directories (where the whole directory is used)
and individual files on first access.  We could also override snapshot and grant
from there.

[commit-bot: I haz the power, 2013-05-24 06:56:16]

Change committed as 201997