Make NativeMediaFileUtil an AsyncFileUtil

Make NativeMediaFileUtil an AsyncFileUtil

Change NativeMediaFileUtil from the synchronous file util interface to AsyncFileUtil in order to support Picasa and iTunes file utils, which have to bounce to the utility process.  Also remove unused write ops from NativeMediaFileUtil.

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

[vandebo (ex-Chrome), 2013-06-02 06:12:45]

Since Tommy will be doing innovation week, I took over his CL. Patch set one is
the same as patch set sixteen of https://codereview.chromium.org/15653004/

In patch set two I addressed all the comments I had, which required removing the
use of PostTaskAndReplyWithResult because it is incompatible with WeakPtr and
adding trampolines for all the methods called that way.  I also removed all the
using clauses with very little spill over (only five extra lines needed
wrapping).

[Lei Zhang, 2013-06-03 10:05:46]

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right):

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:117:
callback.Run(base::PLATFORM_FILE_ERROR_SECURITY, false);
You need to check if the callback is null.

if (!callback.is_null())
  callback.Run(...)
return true;

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:217: bool
NativeMediaFileUtil::DeleteFile(
Seems weird that we can delete directories, but not files. Do we use
DeleteDirectory() to implement some higher level operation, but never have the
need to do so for a file?

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:261:
base::PlatformFileError NativeMediaFileUtil::GetLocalFilePath(
This is identical to IsolatedFileUtil::GetLocalFilePath()

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:287: if (error
== base::PLATFORM_FILE_OK && !file_info.is_directory)
do this after check on line 290

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc (right):

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:170: if (error
!= base::PLATFORM_FILE_OK)
Should check this case first.

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:201:
bool has_more = false;
do you want to make this an out param so the caller can verify the result?

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:434:
ASSERT_FALSE(file_list[i].name.empty());
Not sure this is really neeed. What you do need to check is i not going out of
bound w.r.t. |expected_names|.

[vandebo (ex-Chrome), 2013-06-03 19:15:26]

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right):

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:117:
callback.Run(base::PLATFORM_FILE_ERROR_SECURITY, false);
On 2013/06/03 10:05:46, Lei Zhang wrote:
> You need to check if the callback is null.
> 
> if (!callback.is_null())
>   callback.Run(...)
> return true;

Done.

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:217: bool
NativeMediaFileUtil::DeleteFile(
On 2013/06/03 10:05:46, Lei Zhang wrote:
> Seems weird that we can delete directories, but not files. Do we use
> DeleteDirectory() to implement some higher level operation, but never have the
> need to do so for a file?

I assume this if for Move - we need to recursively delete a dir

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:261:
base::PlatformFileError NativeMediaFileUtil::GetLocalFilePath(
On 2013/06/03 10:05:46, Lei Zhang wrote:
> This is identical to IsolatedFileUtil::GetLocalFilePath()

It's not static and is protected, so there isn't a good way to reuse this small
amount of code.

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:287: if (error
== base::PLATFORM_FILE_OK && !file_info.is_directory)
On 2013/06/03 10:05:46, Lei Zhang wrote:
> do this after check on line 290

Done.

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc (right):

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.cc:170: if (error
!= base::PLATFORM_FILE_OK)
On 2013/06/03 10:05:46, Lei Zhang wrote:
> Should check this case first.

Done.

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:201:
bool has_more = false;
On 2013/06/03 10:05:46, Lei Zhang wrote:
> do you want to make this an out param so the caller can verify the result?

If a caller cares about the result, they can modify the helper function -
deferring until it's needed.

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:434:
ASSERT_FALSE(file_list[i].name.empty());
On 2013/06/03 10:05:46, Lei Zhang wrote:
> Not sure this is really neeed. What you do need to check is i not going out of
> bound w.r.t. |expected_names|.

Done.

[Lei Zhang, 2013-06-03 20:52:05]

Looking good. We still need to discuss whether we need DeleteFile or not.

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:201:
bool has_more = false;
On 2013/06/03 19:15:26, vandebo wrote:
> On 2013/06/03 10:05:46, Lei Zhang wrote:
> > do you want to make this an out param so the caller can verify the result?
> 
> If a caller cares about the result, they can modify the helper function -
> deferring until it's needed.

In that case, why not just remove it altogether and not bother setting it in
ReadDirectoryTestCallback() ?

https://codereview.chromium.org/16304002/diff/11001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right):

https://codereview.chromium.org/16304002/diff/11001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:534: const
StatusCallback& callback) {
All these guys need the callback.is_null() check too?

[vandebo (ex-Chrome), 2013-06-03 22:31:33]

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/16304002/diff/2001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:201:
bool has_more = false;
On 2013/06/03 20:52:05, Lei Zhang wrote:
> On 2013/06/03 19:15:26, vandebo wrote:
> > On 2013/06/03 10:05:46, Lei Zhang wrote:
> > > do you want to make this an out param so the caller can verify the result?
> > 
> > If a caller cares about the result, they can modify the helper function -
> > deferring until it's needed.
> 
> In that case, why not just remove it altogether and not bother setting it in
> ReadDirectoryTestCallback() ?

Done.

https://codereview.chromium.org/16304002/diff/11001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right):

https://codereview.chromium.org/16304002/diff/11001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:534: const
StatusCallback& callback) {
On 2013/06/03 20:52:05, Lei Zhang wrote:
> All these guys need the callback.is_null() check too?

Done.

[Lei Zhang, 2013-06-03 22:42:20]

lgtm with one more comment:

https://codereview.chromium.org/16304002/diff/15003/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/native_media_file_util.h (right):

https://codereview.chromium.org/16304002/diff/15003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.h:14: // This
class handles native file system operations with media type filtering
One last thing: This description is out of date. Can you update it?

[vandebo (ex-Chrome), 2013-06-03 23:13:03]

https://codereview.chromium.org/16304002/diff/15003/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/native_media_file_util.h (right):

https://codereview.chromium.org/16304002/diff/15003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.h:14: // This
class handles native file system operations with media type filtering
On 2013/06/03 22:42:20, Lei Zhang wrote:
> One last thing: This description is out of date. Can you update it?

Done.

[commit-bot: I haz the power, 2013-06-03 23:13:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/16304002/19003

[commit-bot: I haz the power, 2013-06-03 23:38:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-04 05:00:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/16304002/33001

[commit-bot: I haz the power, 2013-06-04 13:07:44]

Change committed as 203954