[MediaGalleries] Enable iPhoto gallery

[MediaGalleries] Enable iPhoto gallery

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

[Greg Billock, 2013-12-02 22:14:56]

.

[Greg Billock, 2014-01-04 09:06:50]

On 2013/12/02 22:14:56, Greg Billock wrote:
> .

There's a nasty shutdown race I believe comes in the browser tests. I see it
about 20% of the time. It looks like the RVH is shutting down earlier than the
test, which means a bunch of DCHECKs get triggered. I think the code is
resilient to that, and we need to get rid of several dchecks in the registry.

[Greg Billock, 2014-01-04 17:01:31]

On 2014/01/04 09:06:50, Greg Billock wrote:
> On 2013/12/02 22:14:56, Greg Billock wrote:
> > .
> 
> There's a nasty shutdown race I believe comes in the browser tests. I see it
> about 20% of the time. It looks like the RVH is shutting down earlier than the
> test, which means a bunch of DCHECKs get triggered. I think the code is
> resilient to that, and we need to get rid of several dchecks in the registry.

[24602:1287:0104/090017:52849243040850:INFO:media_file_system_registry.cc(150)]
OnRPT
[24602:1287:0104/090017:52849243113795:INFO:media_file_system_registry.cc(373)]
CLEAN_UP EGH! 0x7e2860c0
[24602:1287:0104/090017:52849243147641:INFO:media_file_system_registry.cc(690)]
EGH empty
[24602:1287:0104/090017:52849256208365:INFO:media_file_system_registry.cc(300)]
pref_id_map_ entry 1
/private/var/folders/00/02nb0000h01000cxqpysvccm000_nc/T/.org.chromium.Chromium.ENidg2/music
/private/var/folders/00/02nb0000h01000cxqpysvccm000_nc/T/.org.chromium.Chromium.ENidg2/music
1 0x7e2860c0
[24602:1287:0104/090017:52849256264452:INFO:media_file_system_registry.cc(300)]
pref_id_map_ entry 2
/private/var/folders/00/02nb0000h01000cxqpysvccm000_nc/T/.org.chromium.Chromium.ENidg2/pictures
/private/var/folders/00/02nb0000h01000cxqpysvccm000_nc/T/.org.chromium.Chromium.ENidg2/pictures
2 0x7e2860c0
[24602:1287:0104/090017:52849256286388:INFO:media_file_system_registry.cc(300)]
pref_id_map_ entry 3
/private/var/folders/00/02nb0000h01000cxqpysvccm000_nc/T/.org.chromium.Chromium.ENidg2/videos
/private/var/folders/00/02nb0000h01000cxqpysvccm000_nc/T/.org.chromium.Chromium.ENidg2/videos
3 0x7e2860c0
[24602:1287:0104/090017:52849256305756:INFO:media_file_system_registry.cc(300)]
pref_id_map_ entry 4 Picasa
/var/folders/00/02nb0000h01000cxqpysvccm000_nc/T/.org.chromium.Chromium.MpDThi/Google/Picasa3/db3
4 0x7e2860c0
[24602:1287:0104/090017:52849256321964:FATAL:media_file_system_registry.cc(304)]
Check failed: pref_id_map_.empty().

[vandebo (ex-Chrome), 2014-01-06 18:45:02]

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:9:
#include "base/mac/scoped_cftyperef.h"
Should all/most of the new includes be in a MAC_OSX ifdef block?

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/iapps_finder_impl_mac.mm (right):

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/iapps_finder_impl_mac.mm:18: 
nit: extra line.

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:151: // Will this
crash if the RPH is navigated to a page on the same host
This this a debug comment?

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:297: // re-populate
pref_id_map_.
So if rph_refs_ is empty, we know that we lost the race and shouldn't do
anything?

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:520: // FS trash.
What kind of trash do you mean?

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.cc (right):

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.cc:170: // Note that
these overrides set a string value, which isn't the right
Can you use NSArrayFromFilePath to fix this?

[Greg Billock, 2014-01-07 16:50:55]

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/extensions...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/extensions...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:9:
#include "base/mac/scoped_cftyperef.h"
On 2014/01/06 18:45:03, vandebo wrote:
> Should all/most of the new includes be in a MAC_OSX ifdef block?

Yeah, the trybots squealed about this.

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/iapps_finder_impl_mac.mm (right):

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/iapps_finder_impl_mac.mm:18: 
On 2014/01/06 18:45:03, vandebo wrote:
> nit: extra line.

Done.

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_file_system_registry.cc (right):

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:151: // Will this
crash if the RPH is navigated to a page on the same host
On 2014/01/06 18:45:03, vandebo wrote:
> This this a debug comment?

I think this is a real hole. Filling.

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:297: // re-populate
pref_id_map_.
On 2014/01/06 18:45:03, vandebo wrote:
> So if rph_refs_ is empty, we know that we lost the race and shouldn't do
> anything?

I also looked, and I agree this is a fine check. Added it.

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_file_system_registry.cc:520: // FS trash.
On 2014/01/06 18:45:03, vandebo wrote:
> What kind of trash do you mean?

Not exactly sure. Is there some cleanup that needs done? I don't really remember
that there's anything to do. I'll remove the question, unless there's some TODO
here.

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.cc (right):

https://codereview.chromium.org/86743002/diff/70001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.cc:170: // Note that
these overrides set a string value, which isn't the right
On 2014/01/06 18:45:03, vandebo wrote:
> Can you use NSArrayFromFilePath to fix this?

Fixed this up a bit.

[vandebo (ex-Chrome), 2014-01-07 18:42:06]

LGTM

[commit-bot: I haz the power, 2014-01-07 19:33:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/86743002/220001

[commit-bot: I haz the power, 2014-01-07 21:40:54]

Change committed as 243389