Change media galleries to external file system type to add toURL support

Change media galleries to external file system type to add toURL support

BUG=160900
R=creis@chromium.org, joi@chromium.org, thestig@chromium.org, tsepez@chromium.org, tzik@chromium.org

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

[vandebo (ex-Chrome), 2014-03-19 23:20:56]

Please take a first look at this CL.  I'm still working on a browser test for
URL access to media galleries.

[Lei Zhang, 2014-03-20 00:46:25]

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:51: #include
"chrome/browser/media_galleries/fileapi/media_file_system_backend.h"
Already on line 207.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_file_system_backend.cc
(right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.cc:197: if
(components.size() < 1)
nit: .empty() instead

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_file_system_backend.h (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.h:11: #include
"base/files/file.h"
base/files/file_path.h

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.h:30: class
Profile;
not needed?

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.h:45: static
std::string ConstructMountName(const base::FilePath& profile_path,
Can you document the behavior w.r.t. |pref_id| ?

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/imported_media_gallery_registry.h (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/imported_media_gallery_registry.h:88:
base::FilePath imported_root_;
Who deletes this folder?

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry.cc:297: // Checks if
|gallery is attached and if so, registers the file system and
nit: |gallery|

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry.cc:798:
ExtensionGalleriesHostMap::iterator extension_hosts =
Is the returned iterator guaranteed to be valid?

https://codereview.chromium.org/185393012/diff/80001/chrome/renderer/extensio...
File chrome/renderer/extensions/media_galleries_custom_bindings.cc (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/media_galleries_custom_bindings.cc:28: NOTREACHED();
Any reason to do this? In another CL, kalman was saying don't bother with
NOTREACHED() for binding code like this.

[tzik, 2014-03-20 05:52:42]

Looks good.

We can't pass external filesystem to NaCl plugin, while we can't pass isolated.
Doesn't it break things?

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_file_system_backend.cc
(right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.cc:96:
mount_point.compare(0, expected_mount_prefix.size(),
ditto: can be base::StartsWith()?

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.cc:207: if
(mount_point.compare(0, expected_mount_prefix.size(),
can be base::StartsWith()?

[vandebo (ex-Chrome), 2014-03-20 18:08:53]

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:51: #include
"chrome/browser/media_galleries/fileapi/media_file_system_backend.h"
On 2014/03/20 00:46:26, Lei Zhang wrote:
> Already on line 207.

Done.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_file_system_backend.cc
(right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.cc:96:
mount_point.compare(0, expected_mount_prefix.size(),
On 2014/03/20 05:52:43, tzik wrote:
> ditto: can be base::StartsWith()?

Done.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.cc:197: if
(components.size() < 1)
On 2014/03/20 00:46:26, Lei Zhang wrote:
> nit: .empty() instead

Done.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.cc:207: if
(mount_point.compare(0, expected_mount_prefix.size(),
On 2014/03/20 05:52:43, tzik wrote:
> can be base::StartsWith()?

Done.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_file_system_backend.h (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.h:11: #include
"base/files/file.h"
On 2014/03/20 00:46:26, Lei Zhang wrote:
> base/files/file_path.h

Done.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.h:30: class
Profile;
On 2014/03/20 00:46:26, Lei Zhang wrote:
> not needed?

Done.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.h:45: static
std::string ConstructMountName(const base::FilePath& profile_path,
On 2014/03/20 00:46:26, Lei Zhang wrote:
> Can you document the behavior w.r.t. |pref_id| ?

Not sure what you mean specifically wrt to pref_id... Added a comment.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/imported_media_gallery_registry.h (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/imported_media_gallery_registry.h:88:
base::FilePath imported_root_;
On 2014/03/20 00:46:26, Lei Zhang wrote:
> Who deletes this folder?

Added a delete to the destructor - we don't seem to have a scoped_file that
deletes.

I could use a scoped temp dir and put the temp file inside it if you prefer.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry.cc:297: // Checks if
|gallery is attached and if so, registers the file system and
On 2014/03/20 00:46:26, Lei Zhang wrote:
> nit: |gallery|

Done.

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_file_system_registry.cc:798:
ExtensionGalleriesHostMap::iterator extension_hosts =
On 2014/03/20 00:46:26, Lei Zhang wrote:
> Is the returned iterator guaranteed to be valid?

Took awhile to figure out that it is - GetPreferences() initializes it if
needed.  I added a DCHECK with a comment.

https://codereview.chromium.org/185393012/diff/80001/chrome/renderer/extensio...
File chrome/renderer/extensions/media_galleries_custom_bindings.cc (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/media_galleries_custom_bindings.cc:28: NOTREACHED();
On 2014/03/20 00:46:26, Lei Zhang wrote:
> Any reason to do this? In another CL, kalman was saying don't bother with
> NOTREACHED() for binding code like this.

Hmm, did this part of the change a long time ago and don't remember why.  Since
it's a renderer, then a crash seems fine.  Changed it back to a CHECK.

[vandebo (ex-Chrome), 2014-03-20 19:39:02]

Added the test

[Lei Zhang, 2014-03-20 21:37:52]

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_file_system_backend.h (right):

https://codereview.chromium.org/185393012/diff/80001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_system_backend.h:45: static
std::string ConstructMountName(const base::FilePath& profile_path,
On 2014/03/20 18:08:53, vandebo wrote:
> On 2014/03/20 00:46:26, Lei Zhang wrote:
> > Can you document the behavior w.r.t. |pref_id| ?
> 
> Not sure what you mean specifically wrt to pref_id... Added a comment.

The special case of pref_id = kInvalidMediaGalleryPrefId

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:198:
void MakeSingleFakeGallery(MediaGalleryPrefId* pref_id) {
Why not just return the pref id?

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:580:
MakeSingleFakeGallery(&pref_id);
assert |pref_id| is not kInvalidMediaGalleryPrefId?

https://codereview.chromium.org/185393012/diff/140001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/media_galleries/tourl/test.js (right):

https://codereview.chromium.org/185393012/diff/140001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/media_galleries/tourl/test.js:5: function
check_result_factory(expected) {
This might be useful in other tests. Maybe move to common_injected.js and use it
in other places later.

[Lei Zhang, 2014-03-20 21:58:54]

lgtm with some more comments (mostly nits)

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:378:
!MediaStorageUtil::CanCreateFileSystem(device_id, path))
CanCreateFileSystem() probably should do the path.empty() instead?

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:444:
content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
nit: There's already "using content::BrowserThread;" declared, so you can
shorten this. Same below.

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:472: base::FilePath
profile_path_;
might as well make const

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:475: std::string
extension_id_;
const as well.

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:525:
MediaGalleryPrefId pref_id,
Somewhere between here and eventually calling RegisterAttachedMediaFileSystem(),
we should DCHECK that pref_id is valid. All the code in between assumes it is
valid and we probably should have at least 1 sanity check in here.

[tzik, 2014-03-24 03:17:23]

On 2014/03/20 05:52:42, tzik wrote:
> Looks good.
> 
> We can't pass external filesystem to NaCl plugin, while we can't pass
isolated.
> Doesn't it break things?
> 

LGTM.

I recalled that the file passing to NaCl is not yet released to stable channel.

[tzik, 2014-03-24 04:53:20]

On 2014/03/24 03:17:23, tzik wrote:
> On 2014/03/20 05:52:42, tzik wrote:
> > Looks good.
> > 
> > We can't pass external filesystem to NaCl plugin, while we can't pass
> isolated.

Sorry, I mistyped here! I meant:
We *can* pass external filesystem to NaCl plugin, while we can't pass isolated.

[vandebo (ex-Chrome), 2014-03-24 17:50:19]

On 2014/03/24 04:53:20, tzik wrote:
> On 2014/03/24 03:17:23, tzik wrote:
> > On 2014/03/20 05:52:42, tzik wrote:
> > > Looks good.
> > > 
> > > We can't pass external filesystem to NaCl plugin, while we can't pass
> > isolated.
> 
> Sorry, I mistyped here! I meant:
> We *can* pass external filesystem to NaCl plugin, while we can't pass
isolated.

I forgot to reply to this last time. Media galleries doesn't currently work in
NaCl (with isolated file systems). NaCl doesn't have external support currently,
but we're working on that.  So with respect to NaCl there's no change.

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:378:
!MediaStorageUtil::CanCreateFileSystem(device_id, path))
On 2014/03/20 21:58:54, Lei Zhang wrote:
> CanCreateFileSystem() probably should do the path.empty() instead?

Done.

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:444:
content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
On 2014/03/20 21:58:54, Lei Zhang wrote:
> nit: There's already "using content::BrowserThread;" declared, so you can
> shorten this. Same below.

Changed all.

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:472: base::FilePath
profile_path_;
On 2014/03/20 21:58:54, Lei Zhang wrote:
> might as well make const

Done.

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:475: std::string
extension_id_;
On 2014/03/20 21:58:54, Lei Zhang wrote:
> const as well.

Done.

https://codereview.chromium.org/185393012/diff/140001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_file_system_registry.cc:525:
MediaGalleryPrefId pref_id,
On 2014/03/20 21:58:54, Lei Zhang wrote:
> Somewhere between here and eventually calling
RegisterAttachedMediaFileSystem(),
> we should DCHECK that pref_id is valid. All the code in between assumes it is
> valid and we probably should have at least 1 sanity check in here.

Done.

[vandebo (ex-Chrome), 2014-03-24 17:52:13]

+tsepez for CPSP

[Tom Sepez, 2014-03-24 19:36:53]

On 2014/03/24 17:52:13, vandebo wrote:
> +tsepez for CPSP

Adding jschuh@ in case there are windows gotcha's here.

So the question becomes, "What happens to our security model if we grant a
renderer permission to create any file it would like under a specificaly-named
(brower-enforced) subdirectory of a profile directory?"

In any event, you'd need to have tests that prove that directory traversal (eg.
../../) isn't possible once the permission is granted.

[vandebo (ex-Chrome), 2014-03-24 21:16:40]

On 2014/03/24 19:36:53, Tom Sepez wrote:
> On 2014/03/24 17:52:13, vandebo wrote:
> > +tsepez for CPSP
> 
> Adding jschuh@ in case there are windows gotcha's here.
> 
> So the question becomes, "What happens to our security model if we grant a
> renderer permission to create any file it would like under a specificaly-named
> (brower-enforced) subdirectory of a profile directory?"
> 
> In any event, you'd need to have tests that prove that directory traversal
(eg.
> ../../) isn't possible once the permission is granted.

Note, that the endpoint is a file, not a directory.

[jschuh, 2014-03-24 22:23:21]

On 2014/03/24 19:36:53, Tom Sepez wrote:
> On 2014/03/24 17:52:13, vandebo wrote:
> > +tsepez for CPSP
> 
> Adding jschuh@ in case there are windows gotcha's here.
> 
> So the question becomes, "What happens to our security model if we grant a
> renderer permission to create any file it would like under a specificaly-named
> (brower-enforced) subdirectory of a profile directory?"
> 
> In any event, you'd need to have tests that prove that directory traversal
(eg.
> ../../) isn't possible once the permission is granted.

There's no Windows specific gotchas, and as long as the FilePath object is used
there shouldn't be a risk of directory traversals. We've also already discussed
the broader design, so there are no outstanding concerns on that front.

[vandebo (ex-Chrome), 2014-03-25 16:30:07]

On 2014/03/24 22:23:21, Justin Schuh wrote:
> On 2014/03/24 19:36:53, Tom Sepez wrote:
> > On 2014/03/24 17:52:13, vandebo wrote:
> > > +tsepez for CPSP
> > 
> > Adding jschuh@ in case there are windows gotcha's here.
> > 
> > So the question becomes, "What happens to our security model if we grant a
> > renderer permission to create any file it would like under a
specificaly-named
> > (brower-enforced) subdirectory of a profile directory?"
> > 
> > In any event, you'd need to have tests that prove that directory traversal
> (eg.
> > ../../) isn't possible once the permission is granted.
> 
> There's no Windows specific gotchas, and as long as the FilePath object is
used
> there shouldn't be a risk of directory traversals. We've also already
discussed
> the broader design, so there are no outstanding concerns on that front.

Tom, ss there an outstanding question/request here?  It looks like
ChildProcessSecurityPolicyTest.FilePermissions already tests what you requested
anyway.

[Tom Sepez, 2014-03-25 17:02:19]

> Tom, ss there an outstanding question/request here?  It looks like
> ChildProcessSecurityPolicyTest.FilePermissions already tests what you
requested
> anyway.
I guess that the CPSP test is OK here, so I'll give you a LGTM.

I was hoping to get an end-to-end test however; should someone down the road
comment out your call to CPSP, we'd never know that the code is unsafe.

[vandebo (ex-Chrome), 2014-03-25 17:40:36]

The CQ bit was checked by vandebo@chromium.org

[vandebo (ex-Chrome), 2014-03-25 17:41:38]

On 2014/03/25 17:02:19, Tom Sepez wrote:
> > Tom, ss there an outstanding question/request here?  It looks like
> > ChildProcessSecurityPolicyTest.FilePermissions already tests what you
> requested
> > anyway.
> I guess that the CPSP test is OK here, so I'll give you a LGTM.
> 
> I was hoping to get an end-to-end test however; should someone down the road
> comment out your call to CPSP, we'd never know that the code is unsafe.

I'm confused... all new/edited calls to CPSP are for granting permission, so
commenting out any of them will make things fail safe.

[commit-bot: I haz the power, 2014-03-25 17:41:51]

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

[commit-bot: I haz the power, 2014-03-25 18:20:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 18:20:52]

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

[vandebo (ex-Chrome), 2014-03-25 18:29:26]

+joi for content/public

[Jói, 2014-03-25 21:56:33]

//content/public LGTM

[Lei Zhang, 2014-03-26 01:33:51]

https://codereview.chromium.org/185393012/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/185393012/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:198:
void MakeSingleFakeGallery(MediaGalleryPrefId* pref_id) {
nit: Still find this annoying. Why not just return the pref id?

[vandebo (ex-Chrome), 2014-03-26 16:58:36]

lei: had to make a few tweaks for imported file systems if you want to take a
quick look at the 10 to 11 delta.

https://codereview.chromium.org/185393012/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/185393012/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:198:
void MakeSingleFakeGallery(MediaGalleryPrefId* pref_id) {
On 2014/03/26 01:33:52, Lei Zhang wrote:
> nit: Still find this annoying. Why not just return the pref id?

Did I not respond to this? Since there are asserts here, we need to return void.

[vandebo (ex-Chrome), 2014-03-26 16:59:02]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:00:04]

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

[Lei Zhang, 2014-03-26 17:41:30]

https://codereview.chromium.org/185393012/diff/360001/chrome/browser/media_ga...
File chrome/browser/media_galleries/imported_media_gallery_registry.cc (right):

https://codereview.chromium.org/185393012/diff/360001/chrome/browser/media_ga...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:232: if
(!base::CreateTemporaryFile(&imported_root_))
You shouldn't be accessing the file system from the UI thread.

[vandebo (ex-Chrome), 2014-03-26 17:44:40]

The CQ bit was unchecked by vandebo@chromium.org

[Lei Zhang, 2014-03-26 17:50:10]

https://codereview.chromium.org/185393012/diff/360001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/picasa_file_util.cc (right):

https://codereview.chromium.org/185393012/diff/360001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/picasa_file_util.cc:52:
std::vector<std::string> GetVirtualPathComponents(
Might be nice to merge the 3 version of this somehow after M35.

[vandebo (ex-Chrome), 2014-03-26 23:25:24]

lei: please CQ if LG

https://codereview.chromium.org/185393012/diff/360001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/picasa_file_util.cc (right):

https://codereview.chromium.org/185393012/diff/360001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/picasa_file_util.cc:52:
std::vector<std::string> GetVirtualPathComponents(
On 2014/03/26 17:50:11, Lei Zhang wrote:
> Might be nice to merge the 3 version of this somehow after M35.

Yea, there's a general todo to refactor the file utils for these three types to
share code.

https://codereview.chromium.org/185393012/diff/360001/chrome/browser/media_ga...
File chrome/browser/media_galleries/imported_media_gallery_registry.cc (right):

https://codereview.chromium.org/185393012/diff/360001/chrome/browser/media_ga...
chrome/browser/media_galleries/imported_media_gallery_registry.cc:232: if
(!base::CreateTemporaryFile(&imported_root_))
On 2014/03/26 17:41:30, Lei Zhang wrote:
> You shouldn't be accessing the file system from the UI thread.

Done.

[Lei Zhang, 2014-03-26 23:32:45]

lgtm++

https://codereview.chromium.org/185393012/diff/380001/chrome/browser/media_ga...
File chrome/browser/media_galleries/imported_media_gallery_registry.h (right):

https://codereview.chromium.org/185393012/diff/380001/chrome/browser/media_ga...
chrome/browser/media_galleries/imported_media_gallery_registry.h:40: void
Initialize();
nit: comment would be good.

[Lei Zhang, 2014-03-26 23:32:54]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-03-26 23:34:22]

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

[commit-bot: I haz the power, 2014-03-27 00:18:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 00:18:35]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Lei Zhang, 2014-03-27 02:27:19]

https://codereview.chromium.org/185393012/diff/380001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/picasa_file_util.cc (right):

https://codereview.chromium.org/185393012/diff/380001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/picasa_file_util.cc:56: base::FilePath
root = imported_registry->ImportedRoot().Append("picasa");
Append -> AppendASCII, ditto elsewhere.

[vandebo (ex-Chrome), 2014-03-27 05:39:07]

https://codereview.chromium.org/185393012/diff/380001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/picasa_file_util.cc (right):

https://codereview.chromium.org/185393012/diff/380001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/picasa_file_util.cc:56: base::FilePath
root = imported_registry->ImportedRoot().Append("picasa");
On 2014/03/27 02:27:20, Lei Zhang wrote:
> Append -> AppendASCII, ditto elsewhere.

Done x3.

[vandebo (ex-Chrome), 2014-03-27 05:39:57]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-03-27 05:39:59]

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

[commit-bot: I haz the power, 2014-03-27 06:08:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 06:08:35]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[vandebo (ex-Chrome), 2014-03-27 06:33:10]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-03-27 06:33:14]

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

[commit-bot: I haz the power, 2014-03-27 08:00:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 08:00:17]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[vandebo (ex-Chrome), 2014-03-27 20:56:46]

The CQ bit was checked by vandebo@chromium.org

[vandebo (ex-Chrome), 2014-03-27 21:01:33]

The CQ bit was unchecked by vandebo@chromium.org

[vandebo (ex-Chrome), 2014-03-27 21:05:02]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-03-27 21:12:17]

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

[commit-bot: I haz the power, 2014-03-27 21:45:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 21:45:34]

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

[vandebo (ex-Chrome), 2014-03-27 22:44:30]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:47:11]

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

[Charlie Reis, 2014-03-27 22:47:41]

content/public LGTM, since Jói, Justin, and Tom already approved.

[vandebo (ex-Chrome), 2014-03-28 16:08:35]

The CQ bit was unchecked by vandebo@chromium.org

[vandebo (ex-Chrome), 2014-03-28 16:08:47]

The CQ bit was checked by vandebo@chromium.org

[vandebo (ex-Chrome), 2014-03-28 21:06:55]

Committed patchset #18 manually as r260276 (presubmit successful).