Picasa import: Make NativeMediaFileUtil an AsyncFileUtil

Picasa import: Make NativeMediaFileUtil an AsyncFileUtil. Removes some write ops.

NativeMediaFileUtil is now:
 - An AsyncFileUtil
 - Supports a smaller subset of the write ops.

BUG=151701

[tommycli, 2013-05-29 21:53:28]

kinuko/vandebo:

Here is a version that just makes NativeMediaFileUtil an AsyncFileUtil. This
completely divorces the NativeMediaFileUtil from the IsolatedFileUtil chain.

I had to copy paste some code from LocalFileUtil and some other code from
IsolatedFileUtil into NativeMediaFileUtil to make this work.

It's not a very large volume of code, but it does cause some duplication.

Feedback/thoughts?

[tommycli, 2013-05-30 01:16:57]

+cc thestig, gbillock

[vandebo (ex-Chrome), 2013-05-30 18:39:18]

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/media_path_filter.cc (left):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:83:
StringToLowerASCII(path.Extension()));
I think we want to ensure that the extension is lowercase so that we can do
proper matching.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/media_path_filter.cc (right):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:79: bool
MediaPathFilter::Match(const base::FilePath::StringType& base_name) {
Since this method really wants a FilePath, it should take one and let the caller
convert if needed

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (left):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:74: return
NativeFileUtil::EnsureFileExists(file_path, created);
This was for write support?

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

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:250: // We
should not follow symbolic links in sandboxed file system.
This isn't a sandboxed file system.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:259:
GetMediaPathFilter(context)->Match(platform_path->BaseName().value())) {
Just pass the whole platform_path

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:310:
fileapi::VirtualPath::BaseName(platform_path).value();
platform_path is a a platform path not a virtual path, why not just
platform_path.BaseName() like FilteringFileEnumerator?

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:328: entry.name
= base_name;
There's a lot of mechanism to get this value in the current flow, can you walk
me through how this value is the same?

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/native_media_file_util.h (right):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.h:89: virtual
base::PlatformFileError GetFileInfoSync(
These methods should be documented.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.h (right):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.h:24: //         
       be overriding the async public methods instead.
nit: indent.

[tommycli, 2013-05-30 19:35:20]

Also moved AsyncFileUtilTestHelper from webkit/fileapi to chrome/browser.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/media_path_filter.cc (left):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:83:
StringToLowerASCII(path.Extension()));
On 2013/05/30 18:39:19, vandebo wrote:
> I think we want to ensure that the extension is lowercase so that we can do
> proper matching.

Done.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/media_path_filter.cc (right):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/media_path_filter.cc:79: bool
MediaPathFilter::Match(const base::FilePath::StringType& base_name) {
On 2013/05/30 18:39:19, vandebo wrote:
> Since this method really wants a FilePath, it should take one and let the
caller
> convert if needed

Done.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (left):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:74: return
NativeFileUtil::EnsureFileExists(file_path, created);
On 2013/05/30 18:39:19, vandebo wrote:
> This was for write support?

Yes, this function must make an empty file if one doesn't already exist. To
purely check for existence, caller should use GetFileinfo.

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

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:250: // We
should not follow symbolic links in sandboxed file system.
On 2013/05/30 18:39:19, vandebo wrote:
> This isn't a sandboxed file system.

Done.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:259:
GetMediaPathFilter(context)->Match(platform_path->BaseName().value())) {
On 2013/05/30 18:39:19, vandebo wrote:
> Just pass the whole platform_path

Done. There was a reason I changed this, but that reason doesn't seem to exist
anymore. Changed back.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:310:
fileapi::VirtualPath::BaseName(platform_path).value();
On 2013/05/30 18:39:19, vandebo wrote:
> platform_path is a a platform path not a virtual path, why not just
> platform_path.BaseName() like FilteringFileEnumerator?

Done.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:328: entry.name
= base_name;
On 2013/05/30 18:39:19, vandebo wrote:
> There's a lot of mechanism to get this value in the current flow, can you walk
> me through how this value is the same?

Done. Should be clearer in new version.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/native_media_file_util.h (right):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/native_media_file_util.h:89: virtual
base::PlatformFileError GetFileInfoSync(
On 2013/05/30 18:39:19, vandebo wrote:
> These methods should be documented.

Done.

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.h (right):

https://codereview.chromium.org/15653004/diff/8001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.h:24: //         
       be overriding the async public methods instead.
On 2013/05/30 18:39:19, vandebo wrote:
> nit: indent.

Done.

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

Seems good.  Over to Lei for a second look since it's kind of a big change.

[Lei Zhang, 2013-05-30 21:17:58]

Can we make the remove write ups (a big change) a separate CL from async
(another big change) ? Jamming the two into one CL is a great way to slow
reviewers down.

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.h (right):

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.h:17: protected:
This can be private: instead?

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.h:18: virtual
base::PlatformFileError GetFileInfoSync(
Preserve the comment indicating what this is overriding.

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:5:
#include <map>
not obvious where this is needed

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:394:
test_file_util.ReadDirectorySync(operation_context.get(),
check the return result?

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:399:
for (unsigned int i = 0; i < file_list.size(); ++i) {
size_t, not unsigned int

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:400:
ASSERT_TRUE(!file_list[i].name.empty());
ASSERT_FALSE(foo) instead of ASSERT_TRUE(!foo) ?

[Lei Zhang, 2013-05-30 21:27:17]

Either I'm out of the loop on The Plan (TM) or this is not going the right way.
How about we remove the write capability in NativeMediaFileUtil that we are not
going to use in a separate CL, and then talk about the async conversion?

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

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:140:
GetFileInfoSync(context, url, &file_info, &platform_path);
This async file util doesn't seem very async??

[tommycli, 2013-05-30 21:52:32]

Hi thestig,

Yes, combining two changes is pretty confusing. The reason we did it in one is:

- All the lines changed by removing write-capability would have to be rewritten
during the async port.
- Porting write-capability to async would be also wasteful.
- This is a blocker for both Picasa and ITunes.

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.h (right):

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.h:17: protected:
On 2013/05/30 21:17:58, Lei Zhang wrote:
> This can be private: instead?

Done.

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/itunes/itunes_file_util.h:18: virtual
base::PlatformFileError GetFileInfoSync(
On 2013/05/30 21:17:58, Lei Zhang wrote:
> Preserve the comment indicating what this is overriding.

Done.

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

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:140:
GetFileInfoSync(context, url, &file_info, &platform_path);
On 2013/05/30 21:27:17, Lei Zhang wrote:
> This async file util doesn't seem very async??

Done.

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:5:
#include <map>
On 2013/05/30 21:17:58, Lei Zhang wrote:
> not obvious where this is needed

Done.

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:394:
test_file_util.ReadDirectorySync(operation_context.get(),
On 2013/05/30 21:17:58, Lei Zhang wrote:
> check the return result?

Done.

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:399:
for (unsigned int i = 0; i < file_list.size(); ++i) {
On 2013/05/30 21:17:58, Lei Zhang wrote:
> size_t, not unsigned int

Done.

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:400:
ASSERT_TRUE(!file_list[i].name.empty());
On 2013/05/30 21:17:58, Lei Zhang wrote:
> ASSERT_FALSE(foo) instead of ASSERT_TRUE(!foo) ?

Done.

[kinuko, 2013-05-31 03:58:55]

https://codereview.chromium.org/15653004/diff/23002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h
(right):

https://codereview.chromium.org/15653004/diff/23002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h:12:
namespace fileapi {
namespace chrome?

If you keep this class in fileapi namespace can you move this back to
webkit/browser/fileapi?

(I'm fine with either way, this class doesn't seem media-specific)

https://codereview.chromium.org/15653004/diff/23002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h:18: class
AsyncFileUtilTestHelper : public FileSystemFileUtil {
IIUC this class doesn't need to inherit from FileSystemFileUtil... am I correct?

[tommycli, 2013-05-31 20:57:24]

hi: ready for a second round of reviews.

https://codereview.chromium.org/15653004/diff/23002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h
(right):

https://codereview.chromium.org/15653004/diff/23002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h:12:
namespace fileapi {
On 2013/05/31 03:58:56, kinuko wrote:
> namespace chrome?
> 
> If you keep this class in fileapi namespace can you move this back to
> webkit/browser/fileapi?
> 
> (I'm fine with either way, this class doesn't seem media-specific)

Done.

https://codereview.chromium.org/15653004/diff/23002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h:18: class
AsyncFileUtilTestHelper : public FileSystemFileUtil {
On 2013/05/31 03:58:56, kinuko wrote:
> IIUC this class doesn't need to inherit from FileSystemFileUtil... am I
correct?

Done.

[vandebo (ex-Chrome), 2013-05-31 23:22:17]

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h
(right):

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h:18: class
AsyncFileUtilTestHelper : public FileSystemFileUtil {
Since this no longer implements the interface, just remove any functions you
haven't usefully implemented (i.e. all those that just return an error).

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h
(right):

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h:19: //
FileSystemFileUtil overrides.
Out of date comment.

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

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:125: void
FinishStatusCallback(
This isn't needed.

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:168:
Bind(&FinishStatusCallback, callback));
Just pass callback as the reply callback argument.

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:177: return
base::PostTaskAndReplyWithResult(
This will be cleaner if you just pass the callback down instead of using
PostTaskAndReply.

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:460:
*platform_path = file_path;
if (platform_path) and don't pass platform_path if you don't need it. (I think
there's someplace where this is the case.)

[tommycli, 2013-06-01 01:40:54]

vandebo: This version removes the dirty memory allocs, removes
AsyncFileUtilTestHelper, preserves the purely synchronous overriding ability,
and passes all tests.

I go home now. Good luck.

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h
(right):

https://codereview.chromium.org/15653004/diff/36003/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h:18: class
AsyncFileUtilTestHelper : public FileSystemFileUtil {
On 2013/05/31 23:22:17, vandebo wrote:
> Since this no longer implements the interface, just remove any functions you
> haven't usefully implemented (i.e. all those that just return an error).

Done.

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h
(right):

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/async_file_util_test_helper.h:19: //
FileSystemFileUtil overrides.
On 2013/05/31 23:22:17, vandebo wrote:
> Out of date comment.

Done.

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

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:125: void
FinishStatusCallback(
On 2013/05/31 23:22:17, vandebo wrote:
> This isn't needed.

See below.

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:168:
Bind(&FinishStatusCallback, callback));
On 2013/05/31 23:22:17, vandebo wrote:
> Just pass callback as the reply callback argument.

I tried to keep all the Sync methods purely synchronous so that Picasa and
Itunes can override them without putting PostTask stuff everywhere.

I think a PostTaskAndReply is not a big price to pay given that it doesn't
involve any ugly memory allocs.

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:177: return
base::PostTaskAndReplyWithResult(
On 2013/05/31 23:22:17, vandebo wrote:
> This will be cleaner if you just pass the callback down instead of using
> PostTaskAndReply.

Done.

https://codereview.chromium.org/15653004/diff/54001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:460:
*platform_path = file_path;
On 2013/05/31 23:22:17, vandebo wrote:
> if (platform_path) and don't pass platform_path if you don't need it. (I think
> there's someplace where this is the case.)

Done.

[tommycli, 2013-06-01 03:33:45]

vandebo: Actually I thought about it, and the callback-passing interface you
suggested might be better, esp. for chaining calls.

Whatever we do, I'm sure we'll end up refactoring it when we actually try to use
it for the Picasa/ITunes subclasses.

[vandebo (ex-Chrome), 2013-06-02 04:30:24]

On 2013/06/01 03:33:45, tommycli wrote:
> vandebo: Actually I thought about it, and the callback-passing interface you
> suggested might be better, esp. for chaining calls.
> 
> Whatever we do, I'm sure we'll end up refactoring it when we actually try to
use
> it for the Picasa/ITunes subclasses.

We can change our mind later, this seems fine for now.

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

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:102: void
FinishStatusCallback(
You don't need this callback.

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:146:
Bind(&NativeMediaFileUtil::CreateDirectorySync, Unretained(this),
I don't think Unretained(this) is safe, use a weak pointer.

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.cc:148:
Bind(&FinishStatusCallback, callback));
No Bind, just callback

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

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.h:106: //
Synchronous for convenience reasons. Call only on the task runner thread.
All these methods should only called on the task runner right?  Maybe a comment
at the top of the block instead?

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.h:118: // Posted
by ReadDirectory onto the task runner thread.
Comments should be about what the methods does, not who calls it.

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/native_media_file_util.h:155: // Methods
to perform asynchronous methods without heap allocations.
nit: this comment isn't helpful to the future reader.

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.h (right):

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util.h:23: //
TODO(tommycli): Eventually, all of the below methods will have to be
I'm not sure this is true, but that'll be addressed later either way.

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc
(right):

https://codereview.chromium.org/15653004/diff/71001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/picasa/picasa_file_util_unittest.cc:410:
base::RunLoop run_loop;
Probably should pull this into a helper function, it might be reusable.