Isolated FS for media devices.

Isolated FS for Media devices.

Implemented a skeleton code to handle media file systems.

BUG=140332
TEST=none


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

[kmadhusu, 2012-07-16 20:17:03]

Cc'ed vandebo@

[kinuko, 2012-07-18 10:33:24]

http://codereview.chromium.org/10781014/diff/1/chrome/browser/media_gallery/m...
File chrome/browser/media_gallery/media_device_notifications_window_win.cc
(right):

http://codereview.chromium.org/10781014/diff/1/chrome/browser/media_gallery/m...
chrome/browser/media_gallery/media_device_notifications_window_win.cc:202:
hDeviceNotify = RegisterDeviceNotification(hwnd, &db,
DEVICE_NOTIFY_WINDOW_HANDLE);
nit: over 80 cols (here and elsewhere; maybe you can run cpplint)

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/isolated_mount_...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/isolated_mount_...
webkit/fileapi/isolated_mount_point_provider.cc:114: FilePath temp =
GetPathFromURL(url);
Please check if the path is not empty.  GetPathFromURL() may return an empty
path if the given URL is not valid.
(Or probably we should change GetPathFromURL() to return boolean value and let
it return the path by pointer)

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/isolated_mount_...
webkit/fileapi/isolated_mount_point_provider.cc:115: printf("temp.path:%s\n",
temp.value().c_str());
Please remove this when you create a final patch

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/isolated_mount_...
webkit/fileapi/isolated_mount_point_provider.cc:116: FilePath::StringType
Filename(temp.value());
nit: Filename -> filename

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/isolated_mount_...
webkit/fileapi/isolated_mount_point_provider.cc:117: const wchar_t file_prefix[]
= L"\\?\\";
wchar_t -> FilePath::CharType

Since this works only for windows I don't have this code directly in this
provider.  Instead can we define a method like IsMediaFileSystem() which is
implemented in each MediaFileSystemProxy{Win,...} or somewhere you can put
platform-specific code?

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/isolated_mount_...
webkit/fileapi/isolated_mount_point_provider.cc:120: return new
fileapi::MediaFileSystemOperation(context, new
fileapi::MediaFileSystemProxyWin(Filename));
Ditto. We should have a platform-common factory method.

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
File webkit/fileapi/media_file_system_operation.h (right):

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
webkit/fileapi/media_file_system_operation.h:117:
scoped_refptr<fileapi::MediaFileSystemProxyInterface> media_filesystem_proxy_;
nit: filesystem -> file_system (though we often use both interchangeably...)

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
File webkit/fileapi/media_file_system_proxy_win.cc (right):

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
webkit/fileapi/media_file_system_proxy_win.cc:92: hr =
pDevice->Open(const_cast<wchar_t*>(device_id.c_str()), client_info);
nit: over 80 cols (here and more lines after 129-)

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
webkit/fileapi/media_file_system_proxy_win.cc:203: FileSystemOperationContext*
context,
You don't probably need to pass this here, but instead can pass the task runner
when you create MediaFileSystemProxyWin.

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
webkit/fileapi/media_file_system_proxy_win.cc:204: const FileSystemURL& url,
How do you use the URL (from which you can access a virtual path by calling
url.path()) in each operation?

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
webkit/fileapi/media_file_system_proxy_win.cc:208:
context->file_task_runner()->PostTaskAndReply(
As we talked offline I think we'd better separate 'proxy' part (which just posts
a task to a runner to do the actual job) and the 'implementation' part (could be
platform-specific, like the ones you wrote in ReadDirectoryHelper).

Maybe we can have two or three classes like:

* MediaFileSystemOperation (the entry point)
* MediaFileSystemProxy (proxying the task to the actual implementation class on
a blocking task runner)
* MediaFileSystemImpl{Win,Linux,Mac}

Since there wouldn't be much code in MediaFileSystemOperation we could also
probably put the proxying code in MediaFSO instead of having a separate class
just for proxying.

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/webkit_fileapi....
File webkit/fileapi/webkit_fileapi.gypi (right):

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/webkit_fileapi....
webkit/fileapi/webkit_fileapi.gypi:81: 'media_file_util.h',
nit: please avoid using tabs

[kinuko, 2012-07-18 14:51:44]

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
File webkit/fileapi/media_file_system_proxy_win.cc (right):

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
webkit/fileapi/media_file_system_proxy_win.cc:208:
context->file_task_runner()->PostTaskAndReply(
On 2012/07/18 10:33:24, kinuko wrote:
> As we talked offline I think we'd better separate 'proxy' part (which just
posts
> a task to a runner to do the actual job) and the 'implementation' part (could
be
> platform-specific, like the ones you wrote in ReadDirectoryHelper).
> 
> Maybe we can have two or three classes like:
> 
> * MediaFileSystemOperation (the entry point)
> * MediaFileSystemProxy (proxying the task to the actual implementation class
on
> a blocking task runner)
> * MediaFileSystemImpl{Win,Linux,Mac}
> 
> Since there wouldn't be much code in MediaFileSystemOperation we could also
> probably put the proxying code in MediaFSO instead of having a separate class
> just for proxying.

I was thinking about the architecture a bit further.  I think we/you can
probably implement FileSystemFileUtil for MTP as it seems to have more
advantages.

The most visible benefit is we can start smaller changes:
1) We can reuse FileSystemFileUtilProxy for all those proxying, therefore can
focus on writing MTP code.
2) We can still create our own MediaFileSystemOperation if necessary or if it
simplifies the architecture.  The new operation class then just call into the
FileUtil implementation via FileSystemFileUtilProxy.
3) If we find FileSystemFileUtil doesn't work very well for us, it wouldn't be
too late to switch to start writing MTP-specific proxying code.

Possible concerns:
* As for long-running operation issue we can use a separate task runner if
necessary.  For the time being we can start testing with the current FILE
thread, as your current patch is doing, so that it won't block implementation.
* The interface may not work best for MTP file system since FileUtil interface
assumes local file operations.  I need your judge here.

To help your judge let me explain a bit more about FileSystemFileUtil
interface... the interface has following methods (there're some other methods
but they can be ignored):

  CreateOrOpen()
  Close()
  EnsureFileExists()
  GetFileInfo()
  CreateFileEnumerator()
  Truncate()
  PathExists()
  DirectoryExists()
  IsDirectoryEmpty()
  CopyOrMoveFile()
  DeleteFile()
  DeleteSingleDirectory()

All methods are synchronous.  There's no ReadDirectory() but instead FSFU has
CreateFileEnumerator() method which returns a file enumerator instance.  All
recursive operations are implemented on top of the enumerator.

If this interface works for you I can write a skeleton code which glues media
FileUtils and existing code.  Let me hear your thoughts.

Thanks!

[kmadhusu, 2012-07-19 00:57:50]

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
File webkit/fileapi/media_file_system_proxy_win.cc (right):

http://codereview.chromium.org/10781014/diff/1/webkit/fileapi/media_file_syst...
webkit/fileapi/media_file_system_proxy_win.cc:208:
context->file_task_runner()->PostTaskAndReply(
On 2012/07/18 14:51:44, kinuko wrote:
> On 2012/07/18 10:33:24, kinuko wrote:
> > As we talked offline I think we'd better separate 'proxy' part (which just
> posts
> > a task to a runner to do the actual job) and the 'implementation' part
(could
> be
> > platform-specific, like the ones you wrote in ReadDirectoryHelper).
> > 
> > Maybe we can have two or three classes like:
> > 
> > * MediaFileSystemOperation (the entry point)
> > * MediaFileSystemProxy (proxying the task to the actual implementation class
> on
> > a blocking task runner)
> > * MediaFileSystemImpl{Win,Linux,Mac}
> > 
> > Since there wouldn't be much code in MediaFileSystemOperation we could also
> > probably put the proxying code in MediaFSO instead of having a separate
class
> > just for proxying.
> 
> I was thinking about the architecture a bit further.  I think we/you can
> probably implement FileSystemFileUtil for MTP as it seems to have more
> advantages.
> 
> The most visible benefit is we can start smaller changes:
> 1) We can reuse FileSystemFileUtilProxy for all those proxying, therefore can
> focus on writing MTP code.
> 2) We can still create our own MediaFileSystemOperation if necessary or if it
> simplifies the architecture.  The new operation class then just call into the
> FileUtil implementation via FileSystemFileUtilProxy.
> 3) If we find FileSystemFileUtil doesn't work very well for us, it wouldn't be
> too late to switch to start writing MTP-specific proxying code.
> 
> Possible concerns:
> * As for long-running operation issue we can use a separate task runner if
> necessary.  For the time being we can start testing with the current FILE
> thread, as your current patch is doing, so that it won't block implementation.
> * The interface may not work best for MTP file system since FileUtil interface
> assumes local file operations.  I need your judge here.
> 
> To help your judge let me explain a bit more about FileSystemFileUtil
> interface... the interface has following methods (there're some other methods
> but they can be ignored):
> 
>   CreateOrOpen()
>   Close()
>   EnsureFileExists()
>   GetFileInfo()
>   CreateFileEnumerator()
>   Truncate()
>   PathExists()
>   DirectoryExists()
>   IsDirectoryEmpty()
>   CopyOrMoveFile()
>   DeleteFile()
>   DeleteSingleDirectory()
> 
> All methods are synchronous.  There's no ReadDirectory() but instead FSFU has
> CreateFileEnumerator() method which returns a file enumerator instance.  All
> recursive operations are implemented on top of the enumerator.

Thanks for your comments. I discussed about this architecture with vandebo@ and
we would like to give it a try to accomplish the required task. 

> 
> If this interface works for you I can write a skeleton code which glues media
> FileUtils and existing code.  Let me hear your thoughts.
> 
> Thanks!

Currently, I don't have any other high priority tasks. So, I would like to write
the skeleton code. Apart from that, it will help me to understand the workflow.
I will keep you updated with our status.

[Lei Zhang, 2012-07-19 07:10:12]

On 2012/07/18 14:51:44, kinuko wrote:
> * The interface may not work best for MTP file system since FileUtil interface
> assumes local file operations.  I need your judge here.
> 
> To help your judge let me explain a bit more about FileSystemFileUtil
> interface... the interface has following methods (there're some other methods
> but they can be ignored):
> 
>   CreateOrOpen()
>   Close()
>   EnsureFileExists()
>   GetFileInfo()
>   CreateFileEnumerator()
>   Truncate()
>   PathExists()
>   DirectoryExists()
>   IsDirectoryEmpty()
>   CopyOrMoveFile()
>   DeleteFile()
>   DeleteSingleDirectory()

So I have been playing with this code on Linux to hook it up to libmtp as an
experiment. I have a sample extension that uses the HTML5 file api to enumerate
the pictures on a camera and turn them into images in a <DIV>.

For reading the files after enumerating the directories, I don't think I ever
saw FileSystemOperation::OpenFile() getting called. Thus CreateOrOpen() probably
never gets called. Instead FileSystemOperation::GetMetadata() calls
GetFileInfo() to get a FilePath, and later the code in content satisfy the file
read request with a file blob.

I'm not sure if this workflow will work well for MTP, where there is no real
FilePath that corresponds to the files on cameras. Instead, I think what might
work better is if we can read a file off the camera and create a data blob from
it.

[kinuko, 2012-07-19 13:12:33]

On 2012/07/19 07:10:12, Lei Zhang wrote:
> On 2012/07/18 14:51:44, kinuko wrote:
> > * The interface may not work best for MTP file system since FileUtil
interface
> > assumes local file operations.  I need your judge here.
> > 
> > To help your judge let me explain a bit more about FileSystemFileUtil
> > interface... the interface has following methods (there're some other
methods
> > but they can be ignored):
> > 
> >   CreateOrOpen()
> >   Close()
> >   EnsureFileExists()
> >   GetFileInfo()
> >   CreateFileEnumerator()
> >   Truncate()
> >   PathExists()
> >   DirectoryExists()
> >   IsDirectoryEmpty()
> >   CopyOrMoveFile()
> >   DeleteFile()
> >   DeleteSingleDirectory()
> 
> So I have been playing with this code on Linux to hook it up to libmtp as an
> experiment. I have a sample extension that uses the HTML5 file api to
enumerate
> the pictures on a camera and turn them into images in a <DIV>.
>
> For reading the files after enumerating the directories, I don't think I ever
> saw FileSystemOperation::OpenFile() getting called.

OpenFile and Close() are only called via pepper, so if NaCL/pepper is not in the
immediate target you can simply ignore them too.

> Thus CreateOrOpen() probably
> never gets called. Instead FileSystemOperation::GetMetadata() calls
> GetFileInfo() to get a FilePath, and later the code in content satisfy the
file
> read request with a file blob.
> 
> I'm not sure if this workflow will work well for MTP, where there is no real
> FilePath that corresponds to the files on cameras. Instead, I think what might
> work better is if we can read a file off the camera and create a data blob
from
> it.

Currently we have the limitation that reading/writing a file requires a local
file instance, regardless of whether we fork at FileSystemOperation or FileUtil
layer.  It's unfortunate and we're trying to fix it as our mid-term goal but
it's not happening in M22.

For short-term one option is to implement CreateSnapshotFile() (of either
FileUtil or FileSystemOperation), which is supposed to prepare a local snapshot
file and returns the snapshot file's metadata.  For regular filesystem this
could simply do the same as GetFileInfo() does, but for non-regular fs's (like
MTP or GData) you'll need to do:
1. create a local file (if it's not there yet),
2. transfer the data into it and
3. return the metadata of the file.

The file could be a local temporary file, in that case the Blob backend will
take care of the file's lifetime (i.e. it will delete the file when the last
reference of the file is dropped).

[Lei Zhang, 2012-07-19 18:59:21]

On 2012/07/19 13:12:33, kinuko wrote:
> On 2012/07/19 07:10:12, Lei Zhang wrote:
> > I'm not sure if this workflow will work well for MTP, where there is no real
> > FilePath that corresponds to the files on cameras. Instead, I think what
might
> > work better is if we can read a file off the camera and create a data blob
> from
> > it.
> 
> Currently we have the limitation that reading/writing a file requires a local
> file instance, regardless of whether we fork at FileSystemOperation or
FileUtil
> layer.  It's unfortunate and we're trying to fix it as our mid-term goal but
> it's not happening in M22.
> 
> For short-term one option is to implement CreateSnapshotFile() (of either
> FileUtil or FileSystemOperation), which is supposed to prepare a local
snapshot
> file and returns the snapshot file's metadata.  For regular filesystem this
> could simply do the same as GetFileInfo() does, but for non-regular fs's (like
> MTP or GData) you'll need to do:
> 1. create a local file (if it's not there yet),
> 2. transfer the data into it and
> 3. return the metadata of the file.
> 
> The file could be a local temporary file, in that case the Blob backend will
> take care of the file's lifetime (i.e. it will delete the file when the last
> reference of the file is dropped).

Sure. I will try this today and see if it works.

[Lei Zhang, 2012-07-20 04:20:12]

On 2012/07/19 18:59:21, Lei Zhang wrote:
> On 2012/07/19 13:12:33, kinuko wrote:
> > On 2012/07/19 07:10:12, Lei Zhang wrote:
> > > I'm not sure if this workflow will work well for MTP, where there is no
real
> > > FilePath that corresponds to the files on cameras. Instead, I think what
> might
> > > work better is if we can read a file off the camera and create a data blob
> > from
> > > it.
> > 
> > Currently we have the limitation that reading/writing a file requires a
local
> > file instance, regardless of whether we fork at FileSystemOperation or
> FileUtil
> > layer.  It's unfortunate and we're trying to fix it as our mid-term goal but
> > it's not happening in M22.
> > 
> > For short-term one option is to implement CreateSnapshotFile() (of either
> > FileUtil or FileSystemOperation), which is supposed to prepare a local
> snapshot
> > file and returns the snapshot file's metadata.  For regular filesystem this
> > could simply do the same as GetFileInfo() does, but for non-regular fs's
(like
> > MTP or GData) you'll need to do:
> > 1. create a local file (if it's not there yet),
> > 2. transfer the data into it and
> > 3. return the metadata of the file.
> > 
> > The file could be a local temporary file, in that case the Blob backend will
> > take care of the file's lifetime (i.e. it will delete the file when the last
> > reference of the file is dropped).
> 
> Sure. I will try this today and see if it works.

Transferring the file to disk works, but there's a few caveats:
- We need to set the modification time after writing the data so it matches the
timestamp on the camera, which is what we report in the metadata. Fortunately
file_util::SetLastModifiedTime() works just fine on Linux.
- Right now, I just hard coded the files to /tmp/mtpXXXXXX.extension. This fails
the security check in FileAPIMessageFilter::OnAppendBlobDataItem(). I probably
just need to tell ChildProcessSecurityPolicy to grant read permission somewhere,
right?
-- I'm creating the temp file with the same extension as what it has on camera,
so FileAPIMessageFilter::DidCreateSnapshot() can create the blob with the right
mime type. Not sure how important this is though. However, because I need to
create a temp file with a given extension, I have to use mkstemps() directly.
There's no function in base/file_util.h to do this.
- In my hacked version of CL, I've included chrome/ code in webkit/, which is a
layering violation. We can't get away with that in production. Just something to
keep in mind.

Performance-wise, I have an ancient PTP digital camera connected with via USB
1.1. I can fetch 20 pictures (14.3 MB) in ~18 seconds, with ~17 seconds of that
being the time it takes to transfer the pictures from the camera. (Even at
maximum bus speed, it would take 9.5s.) I'll try a Galaxy Nexus tomorrow and see
how well that works.

[kinuko, 2012-07-20 08:14:58]

On Fri, Jul 20, 2012 at 1:20 PM, <thestig@chromium.org> wrote:

> On 2012/07/19 18:59:21, Lei Zhang wrote:
>
>> On 2012/07/19 13:12:33, kinuko wrote:
>> > On 2012/07/19 07:10:12, Lei Zhang wrote:
>> > > I'm not sure if this workflow will work well for MTP, where there is
>> no
>>
> real
>
>> > > FilePath that corresponds to the files on cameras. Instead, I think
>> what
>> might
>> > > work better is if we can read a file off the camera and create a data
>> blob
>> > from
>> > > it.
>> >
>> > Currently we have the limitation that reading/writing a file requires a
>>
> local
>
>> > file instance, regardless of whether we fork at FileSystemOperation or
>> FileUtil
>> > layer.  It's unfortunate and we're trying to fix it as our mid-term
>> goal but
>> > it's not happening in M22.
>> >
>> > For short-term one option is to implement CreateSnapshotFile() (of
>> either
>> > FileUtil or FileSystemOperation), which is supposed to prepare a local
>> snapshot
>> > file and returns the snapshot file's metadata.  For regular filesystem
>> this
>> > could simply do the same as GetFileInfo() does, but for non-regular fs's
>>
> (like
>
>> > MTP or GData) you'll need to do:
>> > 1. create a local file (if it's not there yet),
>> > 2. transfer the data into it and
>> > 3. return the metadata of the file.
>> >
>> > The file could be a local temporary file, in that case the Blob backend
>> will
>> > take care of the file's lifetime (i.e. it will delete the file when the
>> last
>> > reference of the file is dropped).
>>
>
>  Sure. I will try this today and see if it works.
>>
>
> Transferring the file to disk works, but there's a few caveats:
> - We need to set the modification time after writing the data so it
> matches the
> timestamp on the camera, which is what we report in the metadata.
> Fortunately
> file_util::**SetLastModifiedTime() works just fine on Linux.
>

I think if we pass back the file metadata which has the desired last
modified date File object will just use that. (If I could take a look at
the relevant code when you're ready I think I could have a better answer)


> - Right now, I just hard coded the files to /tmp/mtpXXXXXX.extension. This
> fails
> the security check in FileAPIMessageFilter::**OnAppendBlobDataItem(). I
> probably
> just need to tell ChildProcessSecurityPolicy to grant read permission
> somewhere,
> right?
>

Yes, hmm but I thought it'd just work.


> -- I'm creating the temp file with the same extension as what it has on
> camera,
> so FileAPIMessageFilter::**DidCreateSnapshot() can create the blob with
> the right
> mime type. Not sure how important this is though. However, because I need
> to
> create a temp file with a given extension, I have to use mkstemps()
> directly.
> There's no function in base/file_util.h to do this.
>

As far as the virtual FileSystem path (e.g.
filesystem://bula/xxxxxx.exteionsion) has the right extension
the snapshot's real file path shouldn't matter.  For example GData file
system creates snapshot cached files with cryptic filenames.

- In my hacked version of CL, I've included chrome/ code in webkit/, which
> is a
> layering violation. We can't get away with that in production. Just
> something to
> keep in mind.
>

Yup, I anticipated you'd want to have your code in chrome/. If necessary we
can move most of the MTP related code under chrome/ with layering fixups,
but it can be done later..

Performance-wise, I have an ancient PTP digital camera connected with via
> USB
> 1.1. I can fetch 20 pictures (14.3 MB) in ~18 seconds, with ~17 seconds of
> that
> being the time it takes to transfer the pictures from the camera. (Even at
> maximum bus speed, it would take 9.5s.) I'll try a Galaxy Nexus tomorrow
> and see
> how well that works.
>

Nice to see the earlier performance stats!  Let's try addressing
performance bugs once we make sure the basic requirements work.


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

[kmadhusu, 2012-07-23 17:58:46]

Kinuko@: As per our earlier discussion, I implemented a new skeleton code with
the following classes:

(1) MediaFileUtil: FileSystemFileUtil class for media devices.
(2) MtpDeviceInterface{Win, Linux}: This class contains the platform specific
code to communicate with the MTP device. This class is instantiated per mtp
device. Mtp device is opened for communication during initialization and closed
during destruction. 
(3) MediaDeviceMapService: Class that maintains a map of attached MTP devices
with the corresponding MtpDeviceInterface objects.

TODO:
1. Implement MediaFileUtil unit tests.
2. When an attached media device is detached from the system, remove MediaDevice
entry from MediaDeviceMap. 
3. Need a better way of identifying whether the isolated file system url belongs
to mtp device or not.
4. Rename MediaDeviceMapService to MtpDeviceInterfaceStore?
5. Rename MtpDeviceInterface{Win, Linux} => MtpDeviceInterfaceImpl{Win, Linux}?
4. Complete mtp device implementation on windows.

Please take a look at the skeleton code and let me know your comments. 

Thanks.

http://codereview.chromium.org/10781014/diff/14001/webkit/fileapi/media_devic...
File webkit/fileapi/media_device_map_service.h (right):

http://codereview.chromium.org/10781014/diff/14001/webkit/fileapi/media_devic...
webkit/fileapi/media_device_map_service.h:26: class MediaDeviceMapService {
This class needs a better name. How about MediaDeviceInterfaceStore?

[kmadhusu, 2012-07-23 18:02:29]

CC'ed thestig@

[kinuko, 2012-07-24 19:57:19]

I want to take another look but the overall structure looks good to me.  We're
planning to make a related refactoring around IsolatedFS and some part will
affect your change (hopefully it'd simplify your change).  Let's talk about that
f2f!

http://codereview.chromium.org/10781014/diff/6003/webkit/fileapi/isolated_mou...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

http://codereview.chromium.org/10781014/diff/6003/webkit/fileapi/isolated_mou...
webkit/fileapi/isolated_mount_point_provider.cc:56: #endif
We are thinking about a refactoring like following:
- Extend the IsolatedContext::Register API to take a filesystem type (therefore
if MTP device is attached we can register the path as MTPFileSystem)
- Make the GetFileUtil take FileSystemType parameter

Then in the fileapi code all we need to do is dispatch appropriate FileUtils
based on the types.  WDYT?  Will it work for you?
(Unsubmitted CL: http://codereview.chromium.org/10810053/)

[kmadhusu, 2012-07-24 20:56:34]

http://codereview.chromium.org/10781014/diff/6003/webkit/fileapi/isolated_mou...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

http://codereview.chromium.org/10781014/diff/6003/webkit/fileapi/isolated_mou...
webkit/fileapi/isolated_mount_point_provider.cc:56: #endif
On 2012/07/24 19:57:19, kinuko wrote:
> We are thinking about a refactoring like following:
> - Extend the IsolatedContext::Register API to take a filesystem type
(therefore
> if MTP device is attached we can register the path as MTPFileSystem)
> - Make the GetFileUtil take FileSystemType parameter
> 
> Then in the fileapi code all we need to do is dispatch appropriate FileUtils
> based on the types.  WDYT?  Will it work for you?
> (Unsubmitted CL: http://codereview.chromium.org/10810053/)

sgtm. Your changes will simplify this CL. I will start working on my TODO list.

[vandebo (ex-Chrome), 2012-07-24 21:36:55]

http://codereview.chromium.org/10781014/diff/6003/webkit/fileapi/media_file_u...
File webkit/fileapi/media_file_util.cc (right):

http://codereview.chromium.org/10781014/diff/6003/webkit/fileapi/media_file_u...
webkit/fileapi/media_file_util.cc:62: if (!GetPlatformPath(url, platform_path))
nit: looks like you could put all of this into the helper... crack the FS url
and lookup then return the MediaDevice object?

[Lei Zhang, 2012-07-25 04:48:29]

https://chromiumcodereview.appspot.com/10781014/diff/5016/webkit/fileapi/isol...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/5016/webkit/fileapi/isol...
webkit/fileapi/isolated_mount_point_provider.cc:121: FilePath::StringType
device_name(actual_path.value());
Let's say we have a device called "usb_foo". The first time this gets called,
|device_name| is usb_foo. Then you call DirectoryExists() for usb_foo and it
returns "DCIM". Next time around, |device_name| will be usb_foo/DCIM here. Is
that intended, or should |device_name| be consistent between
CreateFileSystemOperation() calls?

[kinuko, 2012-07-25 23:22:28]

Some high-level comments. (I'll take yet another look!)

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_devic...
File webkit/fileapi/media_device_map_service.h (right):

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_devic...
webkit/fileapi/media_device_map_service.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
A suggestion-- I think it'd be better to create a new directory, e.g.
webkit/fileapi/media and put all these media FS specific details into the dir. 
Then eventually you'll want to have its own OWNERS file for the directory so
that Media Gallery team can touch the files under the directory more freely.
WDYT?

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
File webkit/fileapi/media_file_util.cc (right):

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:17: #include
"webkit/fileapi/mtp_device_interface_linux.h"
I think more common pattern is to have a common interface in a common header
(e.g. webkit/fileapi/mt_device_interface.h) and have platform-specific
implementation inside it as pimpl  (Like how net/base/file_stream.h does).  Then
the consumer of the device interface should only know about the common
interface.

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:35: url.path(), &filesystem_id, &root,
platform_path))
As we chatted offline can we call
IsolatedContext::GetInstance()->GetRegisteredPath(url.filesystem_id(),
&root_path) to get the root path?

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:85: if (!GetPlatformPathAndDeviceForUrl(url,
platform_path, &device))
after r148440 url.path() will be the platform path

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:154: return false;
Maybe this should rather return true?

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
File webkit/fileapi/media_file_util.h (right):

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.h:22: class FILEAPI_EXPORT_PRIVATE MediaFileUtil
: public FileSystemFileUtil {
Might be better to name this MTPFileUtil (and use kFileSystemTypeMTP)?

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.h:98: DISALLOW_COPY_AND_ASSIGN(MediaFileUtil);
This needs to be in private:

[kinuko, 2012-07-26 00:38:06]

Some more comments!  I'm a bit worry about how all these device related tasks
work on UI/IO/File thread.

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/media_file_...
File webkit/fileapi/media_file_util.cc (right):

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:27: bool GetPlatformPathAndDeviceForUrl(
Let's call GetMediaDevice in IsolatedMPP::CreateFileSystemOperation(), add a new
field (e.g. media_device) in FileSystemOperationContext and set the field to the
obtained device ptr there.  Then we don't need to call GetDevice in each method
in FileUtil, but instead can just use the given operation context's member field
(e.g. context->media_device()).

We're going to submit this change: http://codereview.chromium.org/10834021/
so that you can set type-specific per-operation data in the
FileSystemOperationContext in CreateFileSystemOperation().

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/mtp_device_...
File webkit/fileapi/mtp_device_interface_win.cc (right):

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/mtp_device_...
webkit/fileapi/mtp_device_interface_win.cc:26: CoUninitialize();
Is it really ok to call these on another thread from the one we called
CoInitialize and Init()?

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/mtp_device_...
File webkit/fileapi/mtp_device_interface_win.h (right):

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/mtp_device_...
webkit/fileapi/mtp_device_interface_win.h:33: public
base::RefCountedThreadSafe<MtpDeviceInterfaceWin> {
Keeping this device interface as RefCountedThreadSafe feels dangerous to me. Is
it really safe to call destructor of this class on any threads on any platform? 
Also since RemoveMediaDevice could be used on UI thread if one implements some
blocking operation in the dtor it could stall the UI.

[kmadhusu, 2012-07-27 02:13:40]

Addressed review comments. PTAL. Thanks.

http://codereview.chromium.org/10781014/diff/6003/webkit/fileapi/media_file_u...
File webkit/fileapi/media_file_util.cc (right):

http://codereview.chromium.org/10781014/diff/6003/webkit/fileapi/media_file_u...
webkit/fileapi/media_file_util.cc:62: if (!GetPlatformPath(url, platform_path))
On 2012/07/24 21:36:55, vandebo wrote:
> nit: looks like you could put all of this into the helper... crack the FS url
> and lookup then return the MediaDevice object?

Done

http://codereview.chromium.org/10781014/diff/5016/webkit/fileapi/isolated_mou...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

http://codereview.chromium.org/10781014/diff/5016/webkit/fileapi/isolated_mou...
webkit/fileapi/isolated_mount_point_provider.cc:121: FilePath::StringType
device_name(actual_path.value());
On 2012/07/25 04:48:29, Lei Zhang wrote:
> Let's say we have a device called "usb_foo". The first time this gets called,
> |device_name| is usb_foo. Then you call DirectoryExists() for usb_foo and it
> returns "DCIM". Next time around, |device_name| will be usb_foo/DCIM here. Is
> that intended, or should |device_name| be consistent between
> CreateFileSystemOperation() calls?

It is not intentional. I fixed the code to get the root path of the file system.
Now, |device_name| is consistent between file operations. Thanks for catching
this.

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_devic...
File webkit/fileapi/media_device_map_service.h (right):

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_devic...
webkit/fileapi/media_device_map_service.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/07/25 23:22:28, kinuko wrote:
> A suggestion-- I think it'd be better to create a new directory, e.g.
> webkit/fileapi/media and put all these media FS specific details into the dir.

> Then eventually you'll want to have its own OWNERS file for the directory so
> that Media Gallery team can touch the files under the directory more freely.
> WDYT?

Done.

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
File webkit/fileapi/media_file_util.cc (right):

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:17: #include
"webkit/fileapi/mtp_device_interface_linux.h"
On 2012/07/25 23:22:28, kinuko wrote:
> I think more common pattern is to have a common interface in a common header
> (e.g. webkit/fileapi/mt_device_interface.h) and have platform-specific
> implementation inside it as pimpl  (Like how net/base/file_stream.h does). 
Then
> the consumer of the device interface should only know about the common
> interface. 

Done. Created media_device_interface.h, media_device_interface_impl.h,
media_device_interface_impl_linux.h && .cc

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:35: url.path(), &filesystem_id, &root,
platform_path))
On 2012/07/25 23:22:28, kinuko wrote:
> As we chatted offline can we call
> IsolatedContext::GetInstance()->GetRegisteredPath(url.filesystem_id(),
> &root_path) to get the root path?

Done.

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:85: if (!GetPlatformPathAndDeviceForUrl(url,
platform_path, &device))
On 2012/07/25 23:22:28, kinuko wrote:
> after r148440 url.path() will be the platform path

Done.

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:154: return false;
On 2012/07/25 23:22:28, kinuko wrote:
> Maybe this should rather return true?

Done.

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
File webkit/fileapi/media_file_util.h (right):

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.h:22: class FILEAPI_EXPORT_PRIVATE MediaFileUtil
: public FileSystemFileUtil {
On 2012/07/25 23:22:28, kinuko wrote:
> Might be better to name this MTPFileUtil (and use kFileSystemTypeMTP)?

As we discussed, I am going with DeviceMediaFileUtil. I am planning to use this
util for both mtp and ptp devices. So, I am not sure if we need
'kFileSystemTypeMTP' type. I would like to call it as
'kFileSystemTypeDeviceMedia' for device media and for local media we can call it
as 'kFileSystemTypeLocalMedia'

http://codereview.chromium.org/10781014/diff/23001/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.h:98: DISALLOW_COPY_AND_ASSIGN(MediaFileUtil);
On 2012/07/25 23:22:28, kinuko wrote:
> This needs to be in private:

Done.

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/media_file_...
File webkit/fileapi/media_file_util.cc (right):

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/media_file_...
webkit/fileapi/media_file_util.cc:27: bool GetPlatformPathAndDeviceForUrl(
On 2012/07/26 00:38:06, kinuko wrote:
> Let's call GetMediaDevice in IsolatedMPP::CreateFileSystemOperation(), add a
new
> field (e.g. media_device) in FileSystemOperationContext and set the field to
the
> obtained device ptr there.  Then we don't need to call GetDevice in each
method
> in FileUtil, but instead can just use the given operation context's member
field
> (e.g. context->media_device()).
> 
> We're going to submit this change: http://codereview.chromium.org/10834021/
> so that you can set type-specific per-operation data in the
> FileSystemOperationContext in CreateFileSystemOperation().

Done.

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/mtp_device_...
File webkit/fileapi/mtp_device_interface_win.cc (right):

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/mtp_device_...
webkit/fileapi/mtp_device_interface_win.cc:26: CoUninitialize();
On 2012/07/26 00:38:06, kinuko wrote:
> Is it really ok to call these on another thread from the one we called
> CoInitialize and Init()?

That should be okay. I removed the win specific code from this CL. Lets focus on
the linux stuff first.

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/mtp_device_...
File webkit/fileapi/mtp_device_interface_win.h (right):

http://codereview.chromium.org/10781014/diff/25002/webkit/fileapi/mtp_device_...
webkit/fileapi/mtp_device_interface_win.h:33: public
base::RefCountedThreadSafe<MtpDeviceInterfaceWin> {
On 2012/07/26 00:38:06, kinuko wrote:
> Keeping this device interface as RefCountedThreadSafe feels dangerous to me.
Is
> it really safe to call destructor of this class on any threads on any
platform? 
> Also since RemoveMediaDevice could be used on UI thread if one implements some
> blocking operation in the dtor it could stall the UI. 

As we discussed, we are going to make the constructor and destructor very light
and to do lazy initialization during file operation.

[Lei Zhang, 2012-07-27 06:03:04]

I tried patch set 16 and hit a few surprises:
- The "linux" implementation code suddenly became CrOS only.
- Register the path as type fileapi::kFileSystemTypeDeviceMedia in
MediaFileSystemRegistry::RegisterPathAsFileSystem() when appropriate.
-  Handle kFileSystemTypeDeviceMedia in
FileSystemContext::GetMountPointProvider().
- DeviceMediaFileUtil::CreateSnapshotFile() needs to be hooked up, or the
extension will crash with WK assert failures.

[Lei Zhang, 2012-07-27 06:09:15]

https://chromiumcodereview.appspot.com/10781014/diff/30025/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/30025/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:60: *platform_path = url.path();
This won't work. With the current scheme, the returned path is bogus or it's
relative to a device, and not a path on the real file system. Instead, you
probably need to let MediaDeviceInterfaceImpl::GetFileInfo() write the file data
to a temporary file, and set |platform_path| to that temp file. (I haven't tried
to see if that actually works though)

[Lei Zhang, 2012-07-27 06:18:47]

https://chromiumcodereview.appspot.com/10781014/diff/30025/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/30025/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:60: *platform_path = url.path();
On 2012/07/27 06:09:15, Lei Zhang wrote:
> This won't work. With the current scheme, the returned path is bogus or it's
> relative to a device, and not a path on the real file system. Instead, you
> probably need to let MediaDeviceInterfaceImpl::GetFileInfo() write the file
data
> to a temporary file, and set |platform_path| to that temp file. (I haven't
tried
> to see if that actually works though)

And now I have confirmed that works. My test extension is serving pictures off
the camera again.

[kinuko, 2012-07-27 18:20:35]

On Thu, Jul 26, 2012 at 11:18 PM, <thestig@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10781014/diff/**
>
30025/webkit/fileapi/media/**device_media_file_util.cc<https://chromiumcodereview.appspot.com/10781014/diff/30025/webkit/fileapi/media/device_media_file_util.cc>
> File webkit/fileapi/media/device_**media_file_util.cc (right):
>
> https://chromiumcodereview.**appspot.com/10781014/diff/**
>
30025/webkit/fileapi/media/**device_media_file_util.cc#**newcode60<https://chromiumcodereview.appspot.com/10781014/diff/30025/webkit/fileapi/media/device_media_file_util.cc#newcode60>
> webkit/fileapi/media/device_**media_file_util.cc:60: *platform_path =
> url.path();
> On 2012/07/27 06:09:15, Lei Zhang wrote:
>
>> This won't work. With the current scheme, the returned path is bogus
>>
> or it's
>
>> relative to a device, and not a path on the real file system. Instead,
>>
> you
>
>> probably need to let MediaDeviceInterfaceImpl::**GetFileInfo() write the
>>
> file data
>
>> to a temporary file, and set |platform_path| to that temp file. (I
>>
> haven't tried
>
>> to see if that actually works though)
>>
>
> And now I have confirmed that works. My test extension is serving
> pictures off the camera again.
>

Lei, can you tell what API calls does your extension make for serving
pictures?
It looks like our bug, as it should call CreateSnapshotFile internally
instead of GetFileInfo whenever it needs actual platform_path for a local
file.

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

[Lei Zhang, 2012-07-27 18:28:36]

On 2012/07/27 18:20:35, kinuko wrote:
> Lei, can you tell what API calls does your extension make for serving
> pictures?
> It looks like our bug, as it should call CreateSnapshotFile internally
> instead of GetFileInfo whenever it needs actual platform_path for a local
> file.

My test extension calls:

entries[i].file(function(f) {
  var reader = new FileReader();
  reader.onloadend = function(e) {
    processData(this.result);
  };
  reader.readAsDataURL(f);
}, errorHandler);

As I mentioned, in patch set 16, CreateSnapshotFile() isn't hooked up to
anything, so I made it call GetFileInfo().

[kinuko, 2012-07-27 18:30:24]

On 2012/07/27 18:28:36, Lei Zhang wrote:
> On 2012/07/27 18:20:35, kinuko wrote:
> > Lei, can you tell what API calls does your extension make for serving
> > pictures?
> > It looks like our bug, as it should call CreateSnapshotFile internally
> > instead of GetFileInfo whenever it needs actual platform_path for a local
> > file.
> 
> My test extension calls:
> 
> entries[i].file(function(f) {
>   var reader = new FileReader();
>   reader.onloadend = function(e) {
>     processData(this.result);
>   };
>   reader.readAsDataURL(f);
> }, errorHandler);
> 
> As I mentioned, in patch set 16, CreateSnapshotFile() isn't hooked up to
> anything, so I made it call GetFileInfo().

Oh ok, makes sense.  So once we hook up CreateSnapshotFile() I expect the
reading would work without returning platform path in GetFileInfo.

[kinuko, 2012-07-28 02:29:59]

I think it's getting closer now!

https://chromiumcodereview.appspot.com/10781014/diff/26023/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/26023/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:23: #include
"webkit/fileapi/media/media_device_map_service.h"
nit: maybe you could add the ifdef inside the header file and remove ifdefs from
the include-sites?

https://chromiumcodereview.appspot.com/10781014/diff/26023/webkit/fileapi/fil...
File webkit/fileapi/file_system_operation_context.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/26023/webkit/fileapi/fil...
webkit/fileapi/file_system_operation_context.h:46:
scoped_refptr<MediaDeviceInterfaceImpl> media_device() const {
nit: this could probably just return a pointer?

https://chromiumcodereview.appspot.com/10781014/diff/26023/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/26023/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.cc:24:
DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_IO);
This DCHECK (and the one at line 36) may not be really necessary?

https://chromiumcodereview.appspot.com/10781014/diff/26023/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/26023/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:21: // Do the clean up
in DeleteOnCurrentThread() function.
nit: Current -> Correct

[kmadhusu, 2012-07-28 23:15:29]

kinuko@: Addressed review comments.

thestig@: 
>> - The "linux" implementation code suddenly became CrOS only.
Since we are currently focusing on CrOS, I wasn't sure if you wanted to build
mtp_device_interface_impl_linux code on both linux and CrOS. Therefore, I wrote
CrOS specific configuration. Assuming, you wanted to build on both CrOs and
linux, I have made config changes accordingly. 

>> - Register the path as type fileapi::kFileSystemTypeDeviceMedia in
MediaFileSystemRegistry::RegisterPathAsFileSystem() when appropriate.
Done.

>> -  Handle kFileSystemTypeDeviceMedia in
FileSystemContext::GetMountPointProvider().
Done.

>> - DeviceMediaFileUtil::CreateSnapshotFile() needs to be hooked up, or the
extension will crash with WK assert failures.
Done. Please test with the latest patch and let me your comments. If possible,
upload the mtp implementation code, I need it for testing.

PTAL. Thanks.

http://codereview.chromium.org/10781014/diff/26023/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

http://codereview.chromium.org/10781014/diff/26023/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.cc:23: #include
"webkit/fileapi/media/media_device_map_service.h"
On 2012/07/28 02:29:59, kinuko wrote:
> nit: maybe you could add the ifdef inside the header file and remove ifdefs
from
> the include-sites?

As we discussed, this is a temporary code. Therefore, I am leaving it as it is.

http://codereview.chromium.org/10781014/diff/26023/webkit/fileapi/file_system...
File webkit/fileapi/file_system_operation_context.h (right):

http://codereview.chromium.org/10781014/diff/26023/webkit/fileapi/file_system...
webkit/fileapi/file_system_operation_context.h:46:
scoped_refptr<MediaDeviceInterfaceImpl> media_device() const {
On 2012/07/28 02:29:59, kinuko wrote:
> nit: this could probably just return a pointer?

Done.

http://codereview.chromium.org/10781014/diff/26023/webkit/fileapi/media/media...
File webkit/fileapi/media/media_device_map_service.cc (right):

http://codereview.chromium.org/10781014/diff/26023/webkit/fileapi/media/media...
webkit/fileapi/media/media_device_map_service.cc:24:
DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_IO);
On 2012/07/28 02:29:59, kinuko wrote:
> This DCHECK (and the one at line 36) may not be really necessary?

Changed DCHECK()'s to comments.

http://codereview.chromium.org/10781014/diff/26023/webkit/fileapi/media/mtp_d...
File webkit/fileapi/media/mtp_device_interface_impl_linux.cc (right):

http://codereview.chromium.org/10781014/diff/26023/webkit/fileapi/media/mtp_d...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:21: // Do the clean up
in DeleteOnCurrentThread() function.
On 2012/07/28 02:29:59, kinuko wrote:
> nit: Current -> Correct

Done.

[Lei Zhang, 2012-07-30 23:03:25]

lgtm

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:188: bool file_created =
file_util::CreateTemporaryFileInDir(
kinuko: What do you think of this potential issue?

When you do this, the temporary file will no longer have its original extension.
As a result, the net::GetWellKnownMimeTypeFromExtension() call in
FileAPIMessageFilter::DidCreateSnapshot() will not work correctly. Though this
did not affect my test extension.

[kinuko, 2012-07-30 23:19:03]

Looking good, mostly nits only.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/fil...
File webkit/fileapi/file_system_operation_context.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/fil...
webkit/fileapi/file_system_operation_context.h:60: MediaDeviceInterfaceImpl*
media_device_;
Shouldn't this be scoped_refptr?

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:43:
scoped_refptr<MediaDeviceInterfaceImpl>* device) {
This might be a minor coding-taste issue, but maybe we can simply return
MediaDeviceInterfaceImpl pointer (or NULL on failure) instead of bool and get
rid of this *device param?  Returning a pointer to scoped_refptr via an out
pointer makes me feel a bit uneasy, but it might be just me.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:26:
FILE_PATH_LITERAL("IsolatedMediaFileSystem");
DeviceMediaFileSystem might reflect where the path is used better?

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:71: *platform_path = url.path();
Returning this url.path() won't be usable outside this FileUtil, maybe we could
just leave it empty.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:72: return
device->GetFileInfo(*platform_path, file_info);
nit: *platform_path -> url.path()  (though they have the same value)

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:170: FilePath* local_path) {
Can we DCHECK out ptrs before we dereference them?

DCHECK(result);
DCHECK(file_info);
DCHEC(local_path);

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:175: if (!device.get())
Maybe we should set a better error code to *result in this case.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:184: if
(!file_util::CreateDirectory(isolated_media_file_system_dir_path))
ditto.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:188: bool file_created =
file_util::CreateTemporaryFileInDir(
On 2012/07/30 23:03:25, Lei Zhang wrote:
> kinuko: What do you think of this potential issue?
> 
> When you do this, the temporary file will no longer have its original
extension.
> As a result, the net::GetWellKnownMimeTypeFromExtension() call in
> FileAPIMessageFilter::DidCreateSnapshot() will not work correctly. Though this
> did not affect my test extension.

I see what you mean.  I think we should rather fix
FileAPIMessageFilter::DidCreateSnapshot() to use the extension of the virtual
path... will fix now.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:190: if (!file_created)
ditto.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:196: if (*result !=
base::PLATFORM_FILE_ERROR_INVALID_URL) {
if (*result == base::PLATFORM_FILE_OK)

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
File webkit/fileapi/media/media_device_interface.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/media_device_interface.h:38: virtual bool
CreateSnapshotFile(const FilePath& device_file_path,
Since this could fail for various reasons maybe we should return
PlatformFileError rather than bool for this one?

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:22: // This object is
destructed by media_task_runner.
nit: is destructed by -> must be destructed on ?

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:26: if
(!media_task_runner_->RunsTasksOnCurrentThread())
I think this must be DCHECK.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:35:
media_task_runner_->DeleteSoon(FROM_HERE, this);
nit: If we're already on media_task_runner we don't need to call DeleteSoon but
can simply do 'delete this'.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:37: 
nit: please remove extra empty line

[kmadhusu, 2012-07-31 01:17:48]

kinuko@: Addressed review comments. PTAL. Thanks.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/fil...
File webkit/fileapi/file_system_operation_context.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/fil...
webkit/fileapi/file_system_operation_context.h:60: MediaDeviceInterfaceImpl*
media_device_;
On 2012/07/30 23:19:03, kinuko wrote:
> Shouldn't this be scoped_refptr?

I misunderstood your previous comment and changed scoped_refptr to raw ptr.
Fixed.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:43:
scoped_refptr<MediaDeviceInterfaceImpl>* device) {
On 2012/07/30 23:19:03, kinuko wrote:
> This might be a minor coding-taste issue, but maybe we can simply return
> MediaDeviceInterfaceImpl pointer (or NULL on failure) instead of bool and get
> rid of this *device param?  Returning a pointer to scoped_refptr via an out
> pointer makes me feel a bit uneasy, but it might be just me.

As we discussed, I changed this function signature.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:26:
FILE_PATH_LITERAL("IsolatedMediaFileSystem");
On 2012/07/30 23:19:03, kinuko wrote:
> DeviceMediaFileSystem might reflect where the path is used better?

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:71: *platform_path = url.path();
On 2012/07/30 23:19:03, kinuko wrote:
> Returning this url.path() won't be usable outside this FileUtil, maybe we
could
> just leave it empty.

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:72: return
device->GetFileInfo(*platform_path, file_info);
On 2012/07/30 23:19:03, kinuko wrote:
> nit: *platform_path -> url.path()  (though they have the same value)

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:170: FilePath* local_path) {
On 2012/07/30 23:19:03, kinuko wrote:
> Can we DCHECK out ptrs before we dereference them?
> 
> DCHECK(result);
> DCHECK(file_info);
> DCHEC(local_path);

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:175: if (!device.get())
On 2012/07/30 23:19:03, kinuko wrote:
> Maybe we should set a better error code to *result in this case.

This is very unlikely to happen. As we discussed, changing this to DCHECK.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:184: if
(!file_util::CreateDirectory(isolated_media_file_system_dir_path))
On 2012/07/30 23:19:03, kinuko wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:190: if (!file_created)
On 2012/07/30 23:19:03, kinuko wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:196: if (*result !=
base::PLATFORM_FILE_ERROR_INVALID_URL) {
On 2012/07/30 23:19:03, kinuko wrote:
> if (*result == base::PLATFORM_FILE_OK)

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
File webkit/fileapi/media/media_device_interface.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/media_device_interface.h:38: virtual bool
CreateSnapshotFile(const FilePath& device_file_path,
On 2012/07/30 23:19:03, kinuko wrote:
> Since this could fail for various reasons maybe we should return
> PlatformFileError rather than bool for this one?

Done

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:22: // This object is
destructed by media_task_runner.
On 2012/07/30 23:19:03, kinuko wrote:
> nit: is destructed by -> must be destructed on ?

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:26: if
(!media_task_runner_->RunsTasksOnCurrentThread())
On 2012/07/30 23:19:03, kinuko wrote:
> I think this must be DCHECK.

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:35:
media_task_runner_->DeleteSoon(FROM_HERE, this);
On 2012/07/30 23:19:03, kinuko wrote:
> nit: If we're already on media_task_runner we don't need to call DeleteSoon
but
> can simply do 'delete this'.

Done.

https://chromiumcodereview.appspot.com/10781014/diff/22026/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:37: 
On 2012/07/30 23:19:03, kinuko wrote:
> nit: please remove extra empty line

Done.

[kinuko, 2012-07-31 05:35:44]

lgtm

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:54: device);
Maybe GetMediaDevice() can simply return the device ptr so that we could do:

return MediaDeviceMapService::GetInstance()->GetMediaDevice(root_path);

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:25: const FilePath::CharType
kIsolatedMediaFileSystemDir[] =
nit: Similarly kIsolatedMediaFileSystemDir should be probably
kDeviceMediaFileSystemDir?  Or since it's file-local simply 'kFileUtilTempDir'
or something might be ok.

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:68:
scoped_refptr<MediaDeviceInterfaceImpl> device(context->media_device());
nit: to make sure: since now we keep a ref in OperationContext we can safely use
the rawptr as is, i.e. code like following would be also safe (and simpler):

DCHECK(context->media_device());
return context->media_device()->GetFileInfo(url.path(), file_info);

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:169: *result =
base::PLATFORM_FILE_ERROR_INVALID_URL;
nit: maybe we can simply initialize this to PLATFORM_FILE_ERROR_FAILED and
remove line 181 and 189?

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:193: if
(device->CreateSnapshotFile(url.path(), *local_path, file_info))
I think you've changed this method to return PlatformFileError, therefore this
code should be:

*result = device->CreateSnapshotFile(...)

?

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.cc:49: return true;
I think this could also just return MediaDeviceInterfaceImpl instead of boolean
since this basically looks to be a simple getter.  Then the callsite doesn't
need to check both the return value and the out device ptr. (And the latest
patch doesn't seem to check the return value either)

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.h:28: void AddMediaDevice(const
FilePath::StringType& device_id,
nit: AddMediaDevice -> MayAddMediaDevice if this may not add a new one if it
already exists?

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.h:34: // This function is called
on FILE thread.
It looks like now it's also called on IO thread (since we moved the callsite of
this method from MediaFileUtil to IMPP::CreateFileSystemOperation)

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:28: using
base::SequencedTaskRunner;
nit: Some other fileapi header file does the same but in general I think using
'using' in .h file is discouraged unless they are in functions or classes.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces
"You may use a using-declaration anywhere in a .cc file, and in functions,
methods or classes in .h files."

[kmadhusu, 2012-07-31 19:47:23]

kinuko@: Addressed nits. PTAL. Thanks.

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:54: device);
On 2012/07/31 05:35:44, kinuko wrote:
> Maybe GetMediaDevice() can simply return the device ptr so that we could do:
> 
> return MediaDeviceMapService::GetInstance()->GetMediaDevice(root_path);

Done.

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:25: const FilePath::CharType
kIsolatedMediaFileSystemDir[] =
On 2012/07/31 05:35:44, kinuko wrote:
> nit: Similarly kIsolatedMediaFileSystemDir should be probably
> kDeviceMediaFileSystemDir?  Or since it's file-local simply 'kFileUtilTempDir'
> or something might be ok.

kIsolatedMediaFileSystemDir -> kDeviceMediaFileUtilTempDir

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:68:
scoped_refptr<MediaDeviceInterfaceImpl> device(context->media_device());
On 2012/07/31 05:35:44, kinuko wrote:
> nit: to make sure: since now we keep a ref in OperationContext we can safely
use
> the rawptr as is, i.e. code like following would be also safe (and simpler):
> 
> DCHECK(context->media_device());
> return context->media_device()->GetFileInfo(url.path(), file_info);

Done.

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:169: *result =
base::PLATFORM_FILE_ERROR_INVALID_URL;
On 2012/07/31 05:35:44, kinuko wrote:
> nit: maybe we can simply initialize this to PLATFORM_FILE_ERROR_FAILED and
> remove line 181 and 189?

Done.

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:193: if
(device->CreateSnapshotFile(url.path(), *local_path, file_info))
On 2012/07/31 05:35:44, kinuko wrote:
> I think you've changed this method to return PlatformFileError, therefore this
> code should be:
> 
> *result = device->CreateSnapshotFile(...)
> 
> ?

Fixed.

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.cc:49: return true;
On 2012/07/31 05:35:44, kinuko wrote:
> I think this could also just return MediaDeviceInterfaceImpl instead of
boolean
> since this basically looks to be a simple getter.  Then the callsite doesn't
> need to check both the return value and the out device ptr. (And the latest
> patch doesn't seem to check the return value either)

Done.

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.h:28: void AddMediaDevice(const
FilePath::StringType& device_id,
On 2012/07/31 05:35:44, kinuko wrote:
> nit: AddMediaDevice -> MayAddMediaDevice if this may not add a new one if it
> already exists?

AddMediaDevice => MaybeAddMediaDevice

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.h:34: // This function is called
on FILE thread.
On 2012/07/31 05:35:44, kinuko wrote:
> It looks like now it's also called on IO thread (since we moved the callsite
of
> this method from MediaFileUtil to IMPP::CreateFileSystemOperation)

fixed comment.

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/26032/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:28: using
base::SequencedTaskRunner;
On 2012/07/31 05:35:44, kinuko wrote:
> nit: Some other fileapi header file does the same but in general I think using
> 'using' in .h file is discouraged unless they are in functions or classes.
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces
> "You may use a using-declaration anywhere in a .cc file, and in functions,
> methods or classes in .h files."

sure. Done.

[kmadhusu, 2012-07-31 19:49:48]

brettw: Can you do OWNER review for files in base directory? Thanks.

[Lei Zhang, 2012-07-31 20:04:37]

more nits...

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/fil...
File webkit/fileapi/file_system_operation_context.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/fil...
webkit/fileapi/file_system_operation_context.h:62: 
nit: extra blank line

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:43: 
nit: blank line

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.h:27:
IsolatedMountPointProvider(const FilePath& profile_path);
nit: explicit

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:7: #include
"base/message_loop_proxy.h"
nit: alphabetical order in this block

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.h:24: DeviceMediaFileUtil(const
FilePath& profile_path);
nit: explicit

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.h:97: private:
nit: need blank line before private:

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/media_device_interface_impl.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_device_interface_impl.h:16: //TODO(kmadhusu):
Implement mtp device interface on other platforms.
nit: space after //

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/media_file_system_config.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_file_system_config.h:12: #if defined(OS_POSIX) &&
!defined(OS_MACOSX)
Actually, we should probably do OS_LINUX (which includes OS_CHROMEOS) since I
have no idea if libmtp works on other Unices.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:31: class
MtpDeviceInterfaceImplLinux :
nit:

class VeryLongClass
    : public Foo...

[kinuko, 2012-07-31 20:32:31]

fileapi related lgtm

I hope we can get rid of the ifdefs shortly!

http://codereview.chromium.org/10781014/diff/36001/webkit/fileapi/isolated_mo...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

http://codereview.chromium.org/10781014/diff/36001/webkit/fileapi/isolated_mo...
webkit/fileapi/isolated_mount_point_provider.cc:137:
operation_context->set_media_device(device);
nit: Could this be null?  Should we DCHECK?

http://codereview.chromium.org/10781014/diff/36001/webkit/fileapi/media/mtp_d...
File webkit/fileapi/media/mtp_device_interface_impl_linux.cc (right):

http://codereview.chromium.org/10781014/diff/36001/webkit/fileapi/media/mtp_d...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:56:
MtpDeviceInterfaceImplLinux::CreateFileEnumerator(
style-nit:

LongReturnType*
LongClassName::LongMethod(
    ...

http://codereview.chromium.org/10781014/diff/36001/webkit/fileapi/media/mtp_d...
File webkit/fileapi/media/mtp_device_interface_impl_linux.h (right):

http://codereview.chromium.org/10781014/diff/36001/webkit/fileapi/media/mtp_d...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:44: 
nit: no new lines between overrides from the same base class

[Lei Zhang, 2012-07-31 20:48:17]

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:120: const
FilePath::StringType& location) {
I don't think you need to change SystemMonitor to have the location parameter in
all its detach methods.

The location parameter is already stored in |device_id_map_|.

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:187:
fileapi::FileSystemType type = fileapi::kFileSystemTypeIsolated;
This block probably should be a switch statement.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.cc:44: MediaDeviceMap::iterator it
= media_device_map_.find(device_id);
nit: this can be a const_iterator

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.h:39: MediaDeviceMapService();
Add a note to say "access through GetInstance()"

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.h:42: friend struct
DefaultSingletonTraits<MediaDeviceMapService>;
nit: friends and typedefs go above the ctor/dtor

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:38:
base::SequencedTaskRunner* media_task_runner) OVERRIDE;
I don't think the ctor needs OVERRIDE.

[brettw, 2012-07-31 21:47:19]

base LGTM

[kinuko, 2012-07-31 23:29:54]

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:187:
fileapi::FileSystemType type = fileapi::kFileSystemTypeIsolated;
On 2012/07/31 20:48:18, Lei Zhang wrote:
> This block probably should be a switch statement.

I'm afraid if we change this to a switch statement we may need to update this
file every time we add other completely unrelated file system types (just to
avoid getting warnings), which may not be really desired.

[Lei Zhang, 2012-07-31 23:34:41]

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:187:
fileapi::FileSystemType type = fileapi::kFileSystemTypeIsolated;
On 2012/07/31 23:29:54, kinuko wrote:
> On 2012/07/31 20:48:18, Lei Zhang wrote:
> > This block probably should be a switch statement.
> 
> I'm afraid if we change this to a switch statement we may need to update this
> file every time we add other completely unrelated file system types (just to
> avoid getting warnings), which may not be really desired.

I meant a switch for SystemMonitor::MediaDeviceType, not
fileapi::FileSystemType. We very likely would need to change this code when we
add a new media device type.

[kmadhusu, 2012-08-01 01:43:59]

kinuko, thesitg: Addressed nits. PTAL. Thanks.

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:120: const
FilePath::StringType& location) {
On 2012/07/31 20:48:18, Lei Zhang wrote:
> I don't think you need to change SystemMonitor to have the location parameter
in
> all its detach methods.
> 
> The location parameter is already stored in |device_id_map_|.

Fixed.  I realized that I also need type information. Therefore, I modified
device_id_map_ to store MediaDeviceInfo.

https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:187:
fileapi::FileSystemType type = fileapi::kFileSystemTypeIsolated;
On 2012/07/31 20:48:18, Lei Zhang wrote:
> This block probably should be a switch statement.

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/fil...
File webkit/fileapi/file_system_operation_context.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/fil...
webkit/fileapi/file_system_operation_context.h:62: 
On 2012/07/31 20:04:37, Lei Zhang wrote:
> nit: extra blank line

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:43: 
On 2012/07/31 20:04:37, Lei Zhang wrote:
> nit: blank line

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:137:
operation_context->set_media_device(device);
On 2012/07/31 20:32:31, kinuko wrote:
> nit: Could this be null?  Should we DCHECK?

Yes, it can be null. As we discussed, I am leaving it as it is and returning
NULL on  failure.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.h:27:
IsolatedMountPointProvider(const FilePath& profile_path);
On 2012/07/31 20:04:37, Lei Zhang wrote:
> nit: explicit

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:7: #include
"base/message_loop_proxy.h"
On 2012/07/31 20:04:37, Lei Zhang wrote:
> nit: alphabetical order in this block

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.h:24: DeviceMediaFileUtil(const
FilePath& profile_path);
On 2012/07/31 20:04:37, Lei Zhang wrote:
> nit: explicit

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.h:97: private:
On 2012/07/31 20:04:37, Lei Zhang wrote:
> nit: need blank line before private:

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/media_device_interface_impl.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_device_interface_impl.h:16: //TODO(kmadhusu):
Implement mtp device interface on other platforms.
On 2012/07/31 20:04:37, Lei Zhang wrote:
> nit: space after //

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.cc:44: MediaDeviceMap::iterator it
= media_device_map_.find(device_id);
On 2012/07/31 20:48:18, Lei Zhang wrote:
> nit: this can be a const_iterator

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.h:39: MediaDeviceMapService();
On 2012/07/31 20:48:18, Lei Zhang wrote:
> Add a note to say "access through GetInstance()"

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.h:42: friend struct
DefaultSingletonTraits<MediaDeviceMapService>;
On 2012/07/31 20:48:18, Lei Zhang wrote:
> nit: friends and typedefs go above the ctor/dtor

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/media_file_system_config.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/media_file_system_config.h:12: #if defined(OS_POSIX) &&
!defined(OS_MACOSX)
On 2012/07/31 20:04:37, Lei Zhang wrote:
> Actually, we should probably do OS_LINUX (which includes OS_CHROMEOS) since I
> have no idea if libmtp works on other Unices.

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.cc:56:
MtpDeviceInterfaceImplLinux::CreateFileEnumerator(
On 2012/07/31 20:32:31, kinuko wrote:
> style-nit:
> 
> LongReturnType*
> LongClassName::LongMethod(
>     ...

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:31: class
MtpDeviceInterfaceImplLinux :
On 2012/07/31 20:04:37, Lei Zhang wrote:
> nit:
> 
> class VeryLongClass
>     : public Foo...

Done.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:38:
base::SequencedTaskRunner* media_task_runner) OVERRIDE;
On 2012/07/31 20:48:18, Lei Zhang wrote:
> I don't think the ctor needs OVERRIDE.

Fixed.

https://chromiumcodereview.appspot.com/10781014/diff/36001/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:44: 
On 2012/07/31 20:32:31, kinuko wrote:
> nit: no new lines between overrides from the same base class

Done.

[kinuko, 2012-08-01 02:41:25]

On 2012/07/31 23:34:41, Lei Zhang wrote:
>
https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
> File chrome/browser/media_gallery/media_file_system_registry.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10781014/diff/36001/chrome/browser/med...
> chrome/browser/media_gallery/media_file_system_registry.cc:187:
> fileapi::FileSystemType type = fileapi::kFileSystemTypeIsolated;
> On 2012/07/31 23:29:54, kinuko wrote:
> > On 2012/07/31 20:48:18, Lei Zhang wrote:
> > > This block probably should be a switch statement.
> > 
> > I'm afraid if we change this to a switch statement we may need to update
this
> > file every time we add other completely unrelated file system types (just to
> > avoid getting warnings), which may not be really desired.
> 
> I meant a switch for SystemMonitor::MediaDeviceType, not
> fileapi::FileSystemType. We very likely would need to change this code when we
> add a new media device type.

Oh sure... sounds good.

[Lei Zhang, 2012-08-01 03:06:30]

LGTM with nits below.

https://chromiumcodereview.appspot.com/10781014/diff/27040/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:89: std::string
RegisterPathAsFileSystem(
Can we do (type, path) instead? Same order as struct MediaDeviceInfo and
OnMediaDeviceAttached().

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:186: *local_path,
nit: funny indent

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.h:9: #include
"base/file_util_proxy.h"
nit: not needed

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.cc:9: #include
"base/message_loop.h"
nit: not needed

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/media_file_system_config.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/media_file_system_config.h:10: namespace fileapi {
nit: #defines don't need to be in a namespace.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:10: #include
"base/platform_file.h"
nit: probably not needed.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:12: #include
"base/timer.h"
nit: not needed

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:18: class
SequencedTaskRunner;
nit: not needed since you already have the header above.

[Lei Zhang, 2012-08-01 03:13:50]

https://chromiumcodereview.appspot.com/10781014/diff/27040/base/system_monito...
File base/system_monitor/system_monitor_unittest.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/base/system_monito...
base/system_monitor/system_monitor_unittest.cc:134:
.InSequence(mock_sequencer[index]).Times(0);
And this should be .Times(0).InSequence(mock_sequencer[index]);

[kinuko, 2012-08-01 04:24:27]

taking another look... and got some questions.

https://chromiumcodereview.appspot.com/10781014/diff/27040/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:138:
RevokeMediaFileSystem(path);
For the file system we register in DeviceAttachedIntentSource when do we revoke?

I might be wrong, but unless we revoke ALL the file system IDs for the device we
may alway return a non-NULL device pointer from GetDeviceForUrl() in IMPP (for
the filesystem the app gets by device intent) for the remaining file system
don't we?  I mean, RemoveMediaDevice removes the entry but MaybeAddMediaDevice
seems to just create a new one if it doesn't exist.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:52: root_path.value());
Actually if we always call these two (Add and Get) together we might be able to
unify them? (And if we do so it would never return NULL?)

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:136: return NULL;
It looks returning NULL from this method isn't safe since many callsites assume
this returns non-null.  Apologies for the oversight, but could we change back
not to return NULL here and replace media_device() DCHECK's in
DeviceMediaFileUtils like:

DCHECK(context->media_device());

to

if (!context->media_device())
   return appropriate_error_code;  // not found or something

so that the operation doesn't crash even after the device's just removed?

[kmadhusu, 2012-08-01 23:39:35]

kinuko, thestig: Addressed review comments. PTAL at the new patch. I have
modified device intent source notification class to handle device detach
notification message.

Thanks.

https://chromiumcodereview.appspot.com/10781014/diff/27040/base/system_monito...
File base/system_monitor/system_monitor_unittest.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/base/system_monito...
base/system_monitor/system_monitor_unittest.cc:134:
.InSequence(mock_sequencer[index]).Times(0);
On 2012/08/01 03:13:50, Lei Zhang wrote:
> And this should be .Times(0).InSequence(mock_sequencer[index]);

Done.

https://chromiumcodereview.appspot.com/10781014/diff/27040/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:138:
RevokeMediaFileSystem(path);
On 2012/08/01 04:24:27, kinuko wrote:
> For the file system we register in DeviceAttachedIntentSource when do we
revoke?

As we discussed,  modified device intent source notification to handle device
detach notifications.

> 
> I might be wrong, but unless we revoke ALL the file system IDs for the device
we
> may alway return a non-NULL device pointer from GetDeviceForUrl() in IMPP (for
> the filesystem the app gets by device intent) for the remaining file system
> don't we?  I mean, RemoveMediaDevice removes the entry but MaybeAddMediaDevice
> seems to just create a new one if it doesn't exist.

https://chromiumcodereview.appspot.com/10781014/diff/27040/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:89: std::string
RegisterPathAsFileSystem(
On 2012/08/01 03:06:30, Lei Zhang wrote:
> Can we do (type, path) instead? Same order as struct MediaDeviceInfo and
> OnMediaDeviceAttached().

Done.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/iso...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:52: root_path.value());
On 2012/08/01 04:24:27, kinuko wrote:
> Actually if we always call these two (Add and Get) together we might be able
to
> unify them? (And if we do so it would never return NULL?)

As we discussed, I have unified them. New function can still return NULL. We
will handle the failure in DeviceMediaFileUtil.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/iso...
webkit/fileapi/isolated_mount_point_provider.cc:136: return NULL;
On 2012/08/01 04:24:27, kinuko wrote:
> It looks returning NULL from this method isn't safe since many callsites
assume
> this returns non-null.  Apologies for the oversight, but could we change back
> not to return NULL here and replace media_device() DCHECK's in
> DeviceMediaFileUtils like:
> 
> DCHECK(context->media_device());
> 
> to
> 
> if (!context->media_device())
>    return appropriate_error_code;  // not found or something
> 
> so that the operation doesn't crash even after the device's just removed?
> 

Done.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:186: *local_path,
On 2012/08/01 03:06:30, Lei Zhang wrote:
> nit: funny indent

Done.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.h:9: #include
"base/file_util_proxy.h"
On 2012/08/01 03:06:30, Lei Zhang wrote:
> nit: not needed

Done.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/media_device_map_service.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/media_device_map_service.cc:9: #include
"base/message_loop.h"
On 2012/08/01 03:06:30, Lei Zhang wrote:
> nit: not needed

Done.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/media_file_system_config.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/media_file_system_config.h:10: namespace fileapi {
On 2012/08/01 03:06:30, Lei Zhang wrote:
> nit: #defines don't need to be in a namespace.

Done.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
File webkit/fileapi/media/mtp_device_interface_impl_linux.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:10: #include
"base/platform_file.h"
On 2012/08/01 03:06:30, Lei Zhang wrote:
> nit: probably not needed.

Done.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:12: #include
"base/timer.h"
On 2012/08/01 03:06:30, Lei Zhang wrote:
> nit: not needed

Done.

https://chromiumcodereview.appspot.com/10781014/diff/27040/webkit/fileapi/med...
webkit/fileapi/media/mtp_device_interface_impl_linux.h:18: class
SequencedTaskRunner;
On 2012/08/01 03:06:30, Lei Zhang wrote:
> nit: not needed since you already have the header above.

I did not include the header. So this declaration is required.

https://chromiumcodereview.appspot.com/10781014/diff/25022/chrome/browser/int...
File chrome/browser/intents/device_attached_intent_source.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/25022/chrome/browser/int...
chrome/browser/intents/device_attached_intent_source.h:40: DeviceIdToInfoMap;
thestig@:  Since the detach notification didn't have the necessary information,
I have to maintain this map here :(. Lets discuss tomorrow, if we can do better.

[Lei Zhang, 2012-08-01 23:49:05]

https://chromiumcodereview.appspot.com/10781014/diff/25022/chrome/browser/int...
File chrome/browser/intents/device_attached_intent_source.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/25022/chrome/browser/int...
chrome/browser/intents/device_attached_intent_source.cc:8: #include
"base/system_monitor/system_monitor.h"
nit: It's very unlikely you'll ever remove this include from
device_attached_intent_source.h, so you probably don't need it here.

https://chromiumcodereview.appspot.com/10781014/diff/25022/chrome/browser/int...
chrome/browser/intents/device_attached_intent_source.cc:92: case
SystemMonitor::TYPE_MTP:
This is currently dead code, since you've returned early on line 53 above in the
attach method.

https://chromiumcodereview.appspot.com/10781014/diff/25022/chrome/browser/int...
chrome/browser/intents/device_attached_intent_source.cc:94:
MediaDeviceMapService::GetInstance()->RemoveMediaDevice(
Also, why do this here? Don't you already call RemoveMediaDevice() in
MediaFileSystemRegistry::RevokeMediaFileSystem()?

https://chromiumcodereview.appspot.com/10781014/diff/25022/chrome/browser/int...
chrome/browser/intents/device_attached_intent_source.cc:98: default:
You should use TYPE_PATH here, so if anyone ever add a new device type, they
would have to actively consider what to do here.

[kinuko, 2012-08-02 04:27:05]

I'll take another look tomorrow but device map service / fileapi related changes
looking good (along with what we talked).

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
File webkit/fileapi/media/media_device_map_service.cc (right):

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
webkit/fileapi/media/media_device_map_service.cc:24: NOTREACHED();
Please remove this NOTREACHED(), this could return false if the filesystem id
has been already revoked.  Also maybe we don't need this function since this is
called only from one place?

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
webkit/fileapi/media/media_device_map_service.cc:49: media_task_runner)));
nit: weird indentation

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
File webkit/fileapi/media/media_device_map_service.h (right):

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
webkit/fileapi/media/media_device_map_service.h:28: // Return NULL on failure.
This function is called on IO thread.
nit: 'Return NULL if the filesystem_id is no longer valid (e.g. because the
corresponding device is detached etc).'

[kmadhusu, 2012-08-02 16:59:03]

kinuko, thestig: Addressed comments. PTAL. Thanks.

http://codereview.chromium.org/10781014/diff/25022/chrome/browser/intents/dev...
File chrome/browser/intents/device_attached_intent_source.cc (right):

http://codereview.chromium.org/10781014/diff/25022/chrome/browser/intents/dev...
chrome/browser/intents/device_attached_intent_source.cc:8: #include
"base/system_monitor/system_monitor.h"
On 2012/08/01 23:49:05, Lei Zhang wrote:
> nit: It's very unlikely you'll ever remove this include from
> device_attached_intent_source.h, so you probably don't need it here.

Done.

http://codereview.chromium.org/10781014/diff/25022/chrome/browser/intents/dev...
chrome/browser/intents/device_attached_intent_source.cc:92: case
SystemMonitor::TYPE_MTP:
On 2012/08/01 23:49:05, Lei Zhang wrote:
> This is currently dead code, since you've returned early on line 53 above in
the
> attach method.

I understand that this is a dead code now, but I am sure we will handle mtp
devices very soon. That is the reason I added a TODO @ line 51. These changes
will make this CL complete.

http://codereview.chromium.org/10781014/diff/25022/chrome/browser/intents/dev...
chrome/browser/intents/device_attached_intent_source.cc:94:
MediaDeviceMapService::GetInstance()->RemoveMediaDevice(
On 2012/08/01 23:49:05, Lei Zhang wrote:
> Also, why do this here? Don't you already call RemoveMediaDevice() in
> MediaFileSystemRegistry::RevokeMediaFileSystem()?

MediaFileSystemRegistry::RevokeMediaFileSystem handles only the devices
registered using MediaFileSystemRegistry::GetMediaFileSystems function.

For eg: When an extension request for media file systems, we  create entries for
the existing devices and media paths. Until, we submit another request, this map
is not updated with the new device details. In the mean time, when a new device
is attached, we register its file path here. So when the device is removed, the
device_id_map_ in MediaFileSystemRegistry does not have an entry for the new
device and hence we don't remove the mtp device interface entry associated with
that device.

http://codereview.chromium.org/10781014/diff/25022/chrome/browser/intents/dev...
chrome/browser/intents/device_attached_intent_source.cc:98: default:
On 2012/08/01 23:49:05, Lei Zhang wrote:
> You should use TYPE_PATH here, so if anyone ever add a new device type, they
> would have to actively consider what to do here.

Done.

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
File webkit/fileapi/media/media_device_map_service.cc (right):

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
webkit/fileapi/media/media_device_map_service.cc:24: NOTREACHED();
On 2012/08/02 04:27:05, kinuko wrote:
> Please remove this NOTREACHED(), this could return false if the filesystem id
> has been already revoked.  Also maybe we don't need this function since this
is
> called only from one place?

Fixed and removed the helper functions.

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
webkit/fileapi/media/media_device_map_service.cc:49: media_task_runner)));
On 2012/08/02 04:27:05, kinuko wrote:
> nit: weird indentation

Done.

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
File webkit/fileapi/media/media_device_map_service.h (right):

http://codereview.chromium.org/10781014/diff/25022/webkit/fileapi/media/media...
webkit/fileapi/media/media_device_map_service.h:28: // Return NULL on failure.
This function is called on IO thread.
On 2012/08/02 04:27:05, kinuko wrote:
> nit: 'Return NULL if the filesystem_id is no longer valid (e.g. because the
> corresponding device is detached etc).'

Done.

[kinuko, 2012-08-02 20:33:44]

lgtm

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/intents/dev...
File chrome/browser/intents/device_attached_intent_source.cc (right):

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/intents/dev...
chrome/browser/intents/device_attached_intent_source.cc:100: break;
nit: no need to have default (with NOTREACHED)? (here and everywhere)

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.cc:187:
fileapi::FileSystemType type;
nit: Please initialize this to kFileSystemTypeUnknown

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.cc:194: type =
fileapi::kFileSystemTypeIsolated;
I believe now you can use kFileSystemTypeNativeMedia.

[Lei Zhang, 2012-08-02 20:54:50]

https://chromiumcodereview.appspot.com/10781014/diff/27071/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27071/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:221: // Always revoke
the file sytem and then do the clean up operation.
nit: indentation is off here.

[Lei Zhang, 2012-08-02 20:57:19]

https://chromiumcodereview.appspot.com/10781014/diff/27071/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

https://chromiumcodereview.appspot.com/10781014/diff/27071/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.h:90: const
base::SystemMonitor::MediaDeviceType& type,
nit: Please change this and RevokeMediaFileSystem()'s |type| parameter to
|device_type| so the .h and .cc files match.

[Lei Zhang, 2012-08-02 21:32:03]

https://chromiumcodereview.appspot.com/10781014/diff/27071/webkit/fileapi/med...
File webkit/fileapi/media/device_media_file_util.cc (right):

https://chromiumcodereview.appspot.com/10781014/diff/27071/webkit/fileapi/med...
webkit/fileapi/media/device_media_file_util.cc:178: FilePath
isolated_media_file_system_dir_path =
I forgot to mention this problem because I hacked around it locally. At some
point, you need to grant permission to read the files in this directory.
Otherwise you'll fail the security check in
FileAPIMessageFilter::OnAppendBlobDataItem() and everything after that.

[kmadhusu, 2012-08-02 22:05:39]

Addressed review comments. I am currently waiting for
http://codereview.chromium.org/10829136/ to commit. PTAL. Thanks.

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/intents/dev...
File chrome/browser/intents/device_attached_intent_source.cc (right):

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/intents/dev...
chrome/browser/intents/device_attached_intent_source.cc:100: break;
On 2012/08/02 20:33:44, kinuko wrote:
> nit: no need to have default (with NOTREACHED)? (here and everywhere)

NOTREACHED() is not required here. We don't need to do any clean up tasks for
other types.

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.cc:187:
fileapi::FileSystemType type;
On 2012/08/02 20:33:44, kinuko wrote:
> nit: Please initialize this to kFileSystemTypeUnknown

Done.

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.cc:194: type =
fileapi::kFileSystemTypeIsolated;
On 2012/08/02 20:33:44, kinuko wrote:
> I believe now you can use kFileSystemTypeNativeMedia.

Done.

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.cc:221: // Always revoke
the file sytem and then do the clean up operation.
On 2012/08/02 20:54:50, Lei Zhang wrote:
> nit: indentation is off here.

Done.

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_file_system_registry.h (right):

http://codereview.chromium.org/10781014/diff/27071/chrome/browser/media_galle...
chrome/browser/media_gallery/media_file_system_registry.h:90: const
base::SystemMonitor::MediaDeviceType& type,
On 2012/08/02 20:57:19, Lei Zhang wrote:
> nit: Please change this and RevokeMediaFileSystem()'s |type| parameter to
> |device_type| so the .h and .cc files match.

Done.

http://codereview.chromium.org/10781014/diff/27071/webkit/fileapi/media/devic...
File webkit/fileapi/media/device_media_file_util.cc (right):

http://codereview.chromium.org/10781014/diff/27071/webkit/fileapi/media/devic...
webkit/fileapi/media/device_media_file_util.cc:178: FilePath
isolated_media_file_system_dir_path =
On 2012/08/02 21:32:03, Lei Zhang wrote:
> I forgot to mention this problem because I hacked around it locally. At some
> point, you need to grant permission to read the files in this directory.
> Otherwise you'll fail the security check in
> FileAPIMessageFilter::OnAppendBlobDataItem() and everything after that.

kinuko@ is working on the fix.

[commit-bot: I haz the power, 2012-08-03 05:06:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10781014/24085

[commit-bot: I haz the power, 2012-08-03 07:31:15]

Change committed as 149807

[kinuko, 2012-08-13 16:41:03]

http://codereview.chromium.org/10781014/diff/27071/webkit/fileapi/media/devic...
File webkit/fileapi/media/device_media_file_util.cc (right):

http://codereview.chromium.org/10781014/diff/27071/webkit/fileapi/media/devic...
webkit/fileapi/media/device_media_file_util.cc:178: FilePath
isolated_media_file_system_dir_path =
On 2012/08/02 22:05:39, kmadhusu wrote:
> On 2012/08/02 21:32:03, Lei Zhang wrote:
> > I forgot to mention this problem because I hacked around it locally. At some
> > point, you need to grant permission to read the files in this directory.
> > Otherwise you'll fail the security check in
> > FileAPIMessageFilter::OnAppendBlobDataItem() and everything after that.
> 
> kinuko@ is working on the fix.

I believe this has been fixed after http://crrev.com/149798.
Apologies for not having cc'ed you.

[kmadhusu, 2012-08-13 16:43:06]

http://codereview.chromium.org/10781014/diff/27071/webkit/fileapi/media/devic...
File webkit/fileapi/media/device_media_file_util.cc (right):

http://codereview.chromium.org/10781014/diff/27071/webkit/fileapi/media/devic...
webkit/fileapi/media/device_media_file_util.cc:178: FilePath
isolated_media_file_system_dir_path =
On 2012/08/13 16:41:03, kinuko wrote:
> On 2012/08/02 22:05:39, kmadhusu wrote:
> > On 2012/08/02 21:32:03, Lei Zhang wrote:
> > > I forgot to mention this problem because I hacked around it locally. At
some
> > > point, you need to grant permission to read the files in this directory.
> > > Otherwise you'll fail the security check in
> > > FileAPIMessageFilter::OnAppendBlobDataItem() and everything after that.
> > 
> > kinuko@ is working on the fix.
> 
> I believe this has been fixed after http://crrev.com/149798.
> Apologies for not having cc'ed you.

Np. Thanks for the info.