Media Gallery: Implement a basic version of GetMediaFileSystems().

Media Gallery: Implement a basic version of GetMediaFileSystems().

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139766

[Lei Zhang, 2012-05-22 22:58:19]

This is a basic implementation of GetMediaFileSystems() which just makes
"DIR_USER_PICTURES" available as an isolated file system to an extension.

This code does not handle attached media devices yet, because I have concerns
about overlapping mount points. I will add support for attached media devices in
a later CL. It also keeps this CL small and easier to review.

I haven't added any tests yet. I would like some feedback first before I spend
more time polishing this CL.

A (poorly made) demo extension that exercises this code is available in
/home/thestig/mediafs_extension.

[vandebo (ex-Chrome), 2012-05-23 00:50:46]

Approach looks good to me

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:34: for (size_t
i = 0; i < filesystems->size(); i++) {
nit: Can you add a TODO here about checking security / access lists.

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/medi...
File chrome/browser/media_gallery/media_device_manager.h (right):

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/medi...
chrome/browser/media_gallery/media_device_manager.h:29: class MediaDeviceManager
{
nit: MediaFileSystemsManager / MediaFileSystemRegistry

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/medi...
chrome/browser/media_gallery/media_device_manager.h:39:
std::vector<MediaFSIDAndPath>* GetMediaFileSystems();
I think (you may want to check with others), that because of Return Value
Optimization, you don't need to return a pointer to the vector, just return the
vector.

[Lei Zhang, 2012-05-23 03:12:49]

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/exte...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/exte...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:34: for (size_t
i = 0; i < filesystems->size(); i++) {
On 2012/05/23 00:50:46, vandebo wrote:
> nit: Can you add a TODO here about checking security / access lists.

Done.

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/medi...
File chrome/browser/media_gallery/media_device_manager.h (right):

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/medi...
chrome/browser/media_gallery/media_device_manager.h:29: class MediaDeviceManager
{
On 2012/05/23 00:50:46, vandebo wrote:
> nit: MediaFileSystemsManager / MediaFileSystemRegistry

Sure, MediaFileSystemRegistry it is.

https://chromiumcodereview.appspot.com/10409084/diff/2002/chrome/browser/medi...
chrome/browser/media_gallery/media_device_manager.h:39:
std::vector<MediaFSIDAndPath>* GetMediaFileSystems();
On 2012/05/23 00:50:46, vandebo wrote:
> I think (you may want to check with others), that because of Return Value
> Optimization, you don't need to return a pointer to the vector, just return
the
> vector.

Done. I see a few other places that do this and cited RVO as the reason.

[kinuko, 2012-05-24 05:17:05]

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/browser/medi...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/browser/medi...
chrome/browser/media_gallery/media_file_system_registry.cc:59: // TODO(thestig)
Check how BaseName() works for say, 'C:\' on Windows.
Ah this is a good point... we may have a problem if the given path is the root
of a drive.

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/browser/medi...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/browser/medi...
chrome/browser/media_gallery/media_file_system_registry.h:54: MediaPathToFSIDMap
media_fs_map_;
Do we want to have a lock to guard it or may we want to make the Registry class
NonThreadSafe?

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/renderer/ext...
File chrome/renderer/extensions/media_gallery_custom_bindings.cc (right):

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/renderer/ext...
chrome/renderer/extensions/media_gallery_custom_bindings.cc:72:
dirname.c_str());
Please use fileapi::GetFileSystemRootURI and GetFileSystemName for the common
part, and append fsid and dirname after that.

[Lei Zhang, 2012-05-24 22:55:41]

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/browser/medi...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/browser/medi...
chrome/browser/media_gallery/media_file_system_registry.h:54: MediaPathToFSIDMap
media_fs_map_;
On 2012/05/24 05:17:05, kinuko wrote:
> Do we want to have a lock to guard it or may we want to make the Registry
class
> NonThreadSafe?

For now, I think we only need to access |media_fs_map_| on the UI thread. I've
added additional comments and DCHECKs.

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/renderer/ext...
File chrome/renderer/extensions/media_gallery_custom_bindings.cc (right):

https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/renderer/ext...
chrome/renderer/extensions/media_gallery_custom_bindings.cc:72:
dirname.c_str());
On 2012/05/24 05:17:05, kinuko wrote:
> Please use fileapi::GetFileSystemRootURI and GetFileSystemName for the common
> part, and append fsid and dirname after that.

I switched over to using GetFileSystemRootURI().

For GetFileSystemName(), it calls GetFileSystemTypeString(), which does not
handle isolated file systems. Should I change GetFileSystemTypeString() to
handle isolated file systems, or was it deliberately left out?

[kinuko, 2012-05-25 02:33:27]

On Fri, May 25, 2012 at 7:55 AM, <thestig@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10409084/diff/**
>
4002/chrome/browser/media_**gallery/media_file_system_**registry.h<https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/browser/media_gallery/media_file_system_registry.h>
> File chrome/browser/media_gallery/**media_file_system_registry.h (right):
>
> https://chromiumcodereview.**appspot.com/10409084/diff/**
> 4002/chrome/browser/media_**gallery/media_file_system_**
>
registry.h#newcode54<https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/browser/media_gallery/media_file_system_registry.h#newcode54>
> chrome/browser/media_gallery/**media_file_system_registry.h:**54:
> MediaPathToFSIDMap media_fs_map_;
> On 2012/05/24 05:17:05, kinuko wrote:
>
>> Do we want to have a lock to guard it or may we want to make the
>>
> Registry class
>
>> NonThreadSafe?
>>
>
> For now, I think we only need to access |media_fs_map_| on the UI
> thread. I've added additional comments and DCHECKs.
>
> https://chromiumcodereview.**appspot.com/10409084/diff/**
>
4002/chrome/renderer/**extensions/media_gallery_**custom_bindings.cc<https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/renderer/extensions/media_gallery_custom_bindings.cc>
> File chrome/renderer/extensions/**media_gallery_custom_bindings.**cc
> (right):
>
> https://chromiumcodereview.**appspot.com/10409084/diff/**
> 4002/chrome/renderer/**extensions/media_gallery_**
>
custom_bindings.cc#newcode72<https://chromiumcodereview.appspot.com/10409084/diff/4002/chrome/renderer/extensions/media_gallery_custom_bindings.cc#newcode72>
> chrome/renderer/extensions/**media_gallery_custom_bindings.**cc:72:
> dirname.c_str());
> On 2012/05/24 05:17:05, kinuko wrote:
>
>> Please use fileapi::GetFileSystemRootURI and GetFileSystemName for the
>>
> common
>
>> part, and append fsid and dirname after that.
>>
>
> I switched over to using GetFileSystemRootURI().
>
> For GetFileSystemName(), it calls GetFileSystemTypeString(), which does
> not handle isolated file systems. Should I change
> GetFileSystemTypeString() to handle isolated file systems, or was it
> deliberately left out?
>

A change which includes the fix to add the string is in the commit queue:
https://chromiumcodereview.appspot.com/10332071/

I had expected this one would get in earlier but it didn't, sorry about
that.

https://chromiumcodereview.**appspot.com/10409084/<https://chromiumcodereview...
>

[Lei Zhang, 2012-05-25 02:43:58]

On 2012/05/25 02:33:27, kinuko wrote:
> A change which includes the fix to add the string is in the commit queue:
> https://chromiumcodereview.appspot.com/10332071/
> 
> I had expected this one would get in earlier but it didn't, sorry about
> that.

It looks like that should land soon. So I'll just adjust my CL based on the
assumption that the webkit/ portion of that CL has landed.

[Lei Zhang, 2012-05-25 03:18:26]

kinuko: Please see patch set 4.

[kinuko, 2012-05-25 09:30:38]

fileapi related part lgtm

http://codereview.chromium.org/10409084/diff/21001/chrome/browser/extensions/...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

http://codereview.chromium.org/10409084/diff/21001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:55:
ChildProcessSecurityPolicy::GetInstance()->GrantPermissionsForFile(
Instead of specifying detailed permissions you can simply call GrantReadFile(id,
path).  Also I recommend you check if the file is already readable before
calling GrantReadFile. In your case it won't be a problem but granting
permissions to a file would overwrite any existing permissions given to the file
or to its parent.

if (!policy->CanReadFile(id, path))
  policy->CanReadFile(id, path);

[vandebo (ex-Chrome), 2012-05-29 16:46:07]

http://codereview.chromium.org/10409084/diff/21001/chrome/browser/extensions/...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

http://codereview.chromium.org/10409084/diff/21001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:55:
ChildProcessSecurityPolicy::GetInstance()->GrantPermissionsForFile(
On 2012/05/25 09:30:38, kinuko wrote:
> Instead of specifying detailed permissions you can simply call
GrantReadFile(id,
> path).  Also I recommend you check if the file is already readable before
> calling GrantReadFile. In your case it won't be a problem but granting
> permissions to a file would overwrite any existing permissions given to the
file
> or to its parent.
> 
> if (!policy->CanReadFile(id, path))
>   policy->CanReadFile(id, path);

Should this be GrantReadDirectory or even GrantAccessFileSystem ?

[Lei Zhang, 2012-05-29 23:26:40]

https://chromiumcodereview.appspot.com/10409084/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10409084/diff/21001/chrome/browser/ext...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:55:
ChildProcessSecurityPolicy::GetInstance()->GrantPermissionsForFile(
On 2012/05/25 09:30:38, kinuko wrote:
> Instead of specifying detailed permissions you can simply call
GrantReadFile(id,
> path).  Also I recommend you check if the file is already readable before
> calling GrantReadFile. In your case it won't be a problem but granting
> permissions to a file would overwrite any existing permissions given to the
file
> or to its parent.
> 
> if (!policy->CanReadFile(id, path))
>   policy->CanReadFile(id, path);

Done.

https://chromiumcodereview.appspot.com/10409084/diff/21001/chrome/browser/ext...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:55:
ChildProcessSecurityPolicy::GetInstance()->GrantPermissionsForFile(
On 2012/05/29 16:46:07, vandebo wrote:
> On 2012/05/25 09:30:38, kinuko wrote:
> > Instead of specifying detailed permissions you can simply call
> GrantReadFile(id,
> > path).  Also I recommend you check if the file is already readable before
> > calling GrantReadFile. In your case it won't be a problem but granting
> > permissions to a file would overwrite any existing permissions given to the
> file
> > or to its parent.
> > 
> > if (!policy->CanReadFile(id, path))
> >   policy->CanReadFile(id, path);
> 
> Should this be GrantReadDirectory or even GrantAccessFileSystem ?

In the current ChildProcessSecurityPolicy implementation, GrantReadFile() is
recursive - there's no GrantReadDirectory().

GrantAccessFileSystem() doesn't seem to do anything, though kinuko is working on
that.

[kinuko, 2012-05-30 09:45:12]

https://chromiumcodereview.appspot.com/10409084/diff/21001/chrome/browser/ext...
File chrome/browser/extensions/api/media_gallery/media_gallery_api.cc (right):

https://chromiumcodereview.appspot.com/10409084/diff/21001/chrome/browser/ext...
chrome/browser/extensions/api/media_gallery/media_gallery_api.cc:55:
ChildProcessSecurityPolicy::GetInstance()->GrantPermissionsForFile(
On 2012/05/29 23:26:41, Lei Zhang wrote:
> On 2012/05/29 16:46:07, vandebo wrote:
> > On 2012/05/25 09:30:38, kinuko wrote:
> > > Instead of specifying detailed permissions you can simply call
> > GrantReadFile(id,
> > > path).  Also I recommend you check if the file is already readable before
> > > calling GrantReadFile. In your case it won't be a problem but granting
> > > permissions to a file would overwrite any existing permissions given to
the
> > file
> > > or to its parent.
> > > 
> > > if (!policy->CanReadFile(id, path))
> > >   policy->CanReadFile(id, path);
> > 
> > Should this be GrantReadDirectory or even GrantAccessFileSystem ?
> 
> In the current ChildProcessSecurityPolicy implementation, GrantReadFile() is
> recursive - there's no GrantReadDirectory().
> 
> GrantAccessFileSystem() doesn't seem to do anything, though kinuko is working
on
> that.

Yup... if my one lands earlier please change these lines to:
GrantReadFileSystem().

[vandebo (ex-Chrome), 2012-05-30 19:00:57]

You'll need to update chrome/test/data/extensions/api_test/media_gallery/test.js
to expect one media fs to come back, but otherwise LGTM

[Lei Zhang, 2012-05-31 03:49:39]

On 2012/05/30 19:00:57, vandebo wrote:
> You'll need to update
chrome/test/data/extensions/api_test/media_gallery/test.js
> to expect one media fs to come back, but otherwise LGTM

Done in patch set 6.

[Lei Zhang, 2012-05-31 03:50:48]

On 2012/05/30 09:45:12, kinuko wrote:
> Yup... if my one lands earlier please change these lines to:
> GrantReadFileSystem().

Kinuko: I was waiting for your CL to land first, but it looks like your CL has
issues with BlobStrorage. So I'm going to try to land mine first.

[kinuko, 2012-05-31 03:59:07]

Yes I noticed that.
Sounds good, please go ahead!
 On May 31, 2012 12:49 PM, <thestig@chromium.org> wrote:

> On 2012/05/30 19:00:57, vandebo wrote:
>
>> You'll need to update
>>
> chrome/test/data/extensions/**api_test/media_gallery/test.js
>
>> to expect one media fs to come back, but otherwise LGTM
>>
>
> Done in patch set 6.
>
>
https://chromiumcodereview.**appspot.com/10409084/<https://chromiumcodereview...
>

[vandebo (ex-Chrome), 2012-05-31 04:10:38]

http://codereview.chromium.org/10409084/diff/26004/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/media_gallery/test.js (right):

http://codereview.chromium.org/10409084/diff/26004/chrome/test/data/extension...
chrome/test/data/extensions/api_test/media_gallery/test.js:6: var
emptyListCallback = function(results) {
nit: change the name of the function.

[Lei Zhang, 2012-05-31 08:32:38]

https://chromiumcodereview.appspot.com/10409084/diff/26004/chrome/test/data/e...
File chrome/test/data/extensions/api_test/media_gallery/test.js (right):

https://chromiumcodereview.appspot.com/10409084/diff/26004/chrome/test/data/e...
chrome/test/data/extensions/api_test/media_gallery/test.js:6: var
emptyListCallback = function(results) {
On 2012/05/31 04:10:39, vandebo wrote:
> nit: change the name of the function.

Done.

[commit-bot: I haz the power, 2012-05-31 08:32:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10409084/24004

[commit-bot: I haz the power, 2012-05-31 09:41:45]

Change committed as 139766