Media Galleries: Allow readding blacklisted iApps libraries.

Media Galleries: Allow re-adding blacklisted iApps libraries.

Previously, when a user blacklisted an iPhoto/iTunes library, there was no way to get it back without editing the preferences file by hand.

Now, when a user adds a directory under the iPhoto/iTunes library directory, it adds back the device library.

BUG=375503
TEST=manual tested iTunes on Win

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

[tommycli, 2014-06-10 21:35:58]

vandebo: ptal - tested working on Win, building for mac now

[vandebo (ex-Chrome), 2014-06-10 21:57:17]

https://codereview.chromium.org/329643002/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/fileapi/iapps_finder_impl.cc (right):

https://codereview.chromium.org/329643002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/fileapi/iapps_finder_impl.cc:62: return (path ==
library_root) || library_root.IsParent(path);
Probably don't want to be this generic... iTunes is organized as:

Root/iTunes/library.xml
Root/iTunes/Artwork
Root/iTunes/iTunes Media/
Root/iTunes/iTunes Media/Automatically Added to iTunes/
Root/iTunes/iTunes Media/Music/{Artist}/{Album}/{Songs}

Probably Root/iTunes, Root/iTunes/iTunes Media/, and Root/iTunes/iTunes
Media/Music/ should be treated as the same as the XML file, but not other
things. Just the Album art could be different, a specific artists could be
different...

Haven't looked at iPhoto

https://codereview.chromium.org/329643002/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/media_galleries_permission_controller.cc
(right):

https://codereview.chromium.org/329643002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/media_galleries_permission_controller.cc:326:
gallery.pref_id = GetDialogId(gallery.pref_id);
I think this fixes a bug I just introduced...  Can you add a test for this case?

[tommycli, 2014-06-10 23:59:06]

vandebo: Added test, and narrowed scope of folders for both iTunes and iPhoto.

I tested iTunes case on Mac, works. No iPhoto install on either workstation, so
haven't tested that.

I chose the iPhoto directories based on info here:
http://www.fatcatsoftware.com/iplm/Help/iphoto%20library%20internals.html

I'd say 80% chance it's fine.

Let me know if you want me to try harder to get an iPhoto to test (probably buy
and expense a copy from app store).

https://codereview.chromium.org/329643002/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/fileapi/iapps_finder_impl.cc (right):

https://codereview.chromium.org/329643002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/fileapi/iapps_finder_impl.cc:62: return (path ==
library_root) || library_root.IsParent(path);
On 2014/06/10 21:57:17, vandebo wrote:
> Probably don't want to be this generic... iTunes is organized as:
> 
> Root/iTunes/library.xml
> Root/iTunes/Artwork
> Root/iTunes/iTunes Media/
> Root/iTunes/iTunes Media/Automatically Added to iTunes/
> Root/iTunes/iTunes Media/Music/{Artist}/{Album}/{Songs}
> 
> Probably Root/iTunes, Root/iTunes/iTunes Media/, and Root/iTunes/iTunes
> Media/Music/ should be treated as the same as the XML file, but not other
> things. Just the Album art could be different, a specific artists could be
> different...
> 
> Haven't looked at iPhoto

Done.

https://codereview.chromium.org/329643002/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/media_galleries_permission_controller.cc
(right):

https://codereview.chromium.org/329643002/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/media_galleries_permission_controller.cc:326:
gallery.pref_id = GetDialogId(gallery.pref_id);
On 2014/06/10 21:57:17, vandebo wrote:
> I think this fixes a bug I just introduced...  Can you add a test for this
case?

Done.

[vandebo (ex-Chrome), 2014-06-11 16:40:56]

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_permission_controller.cc
(right):

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_permission_controller.cc:291: if
(web_contents_) {
Is this needed? Why are the existing tests ok?

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/media_galleries_permission_controller_unittest.cc
(right):

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_permission_controller_unittest.cc:188:
EXPECT_EQ(mock_gallery_locations_.num_galleries() + 2U,
You're going to need to rebase.

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:673: for
(MediaGalleriesPrefInfoMap::const_iterator it =
This is where cheating on the iTunes/iPhoto device id hurts us.  If we were
being pure about it, the device id would just be "itunes" etc. and we could just
use LookUpGalleriesByDeviceId()

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:678: if
(!StorageInfo::CrackDeviceId(device_id, &device_type, &unique_id))
Instead of cracking it here, use StorageInfo::IsITunesDevice, etc.  It's more
CPU work, but maintains the abstraction better.

[tommycli, 2014-06-11 18:17:56]

added a unittest to m_g_preferences also, just covering the lookup code path

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_permission_controller.cc
(right):

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_permission_controller.cc:291: if
(web_contents_) {
On 2014/06/11 16:40:56, vandebo wrote:
> Is this needed? Why are the existing tests ok?

Test crashes when the controller calls GetProfile(). The other methods called by
the tests don't get the profile.

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/media_galleries_permission_controller_unittest.cc
(right):

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_permission_controller_unittest.cc:188:
EXPECT_EQ(mock_gallery_locations_.num_galleries() + 2U,
On 2014/06/11 16:40:56, vandebo wrote:
> You're going to need to rebase.

Done.

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:673: for
(MediaGalleriesPrefInfoMap::const_iterator it =
On 2014/06/11 16:40:56, vandebo wrote:
> This is where cheating on the iTunes/iPhoto device id hurts us.  If we were
> being pure about it, the device id would just be "itunes" etc. and we could
just
> use LookUpGalleriesByDeviceId()

Yeah... it's true...

https://codereview.chromium.org/329643002/diff/60001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:678: if
(!StorageInfo::CrackDeviceId(device_id, &device_type, &unique_id))
On 2014/06/11 16:40:56, vandebo wrote:
> Instead of cracking it here, use StorageInfo::IsITunesDevice, etc.  It's more
> CPU work, but maintains the abstraction better.

Done.

[vandebo (ex-Chrome), 2014-06-11 18:36:46]

LGTM

https://codereview.chromium.org/329643002/diff/100001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/iapps_finder.h (right):

https://codereview.chromium.org/329643002/diff/100001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/iapps_finder.h:27: // indicated by
|device_id|.
Given this description, I think you don't need to check the type before you pass
a device id in.  See below.

https://codereview.chromium.org/329643002/diff/100001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/329643002/diff/100001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_preferences.cc:675: const
std::string& device_id = it->second.device_id;
If PathIndicates... checks the device type instead of DCHECKing it, this can
just be:

if (PathIndicatesIPhoto(...) || PathIndicatesITunes(...)) {
  *gallery_info = it->second
  return true;
}

[tommycli, 2014-06-11 19:26:39]

https://codereview.chromium.org/329643002/diff/100001/chrome/browser/media_ga...
File chrome/browser/media_galleries/fileapi/iapps_finder.h (right):

https://codereview.chromium.org/329643002/diff/100001/chrome/browser/media_ga...
chrome/browser/media_galleries/fileapi/iapps_finder.h:27: // indicated by
|device_id|.
On 2014/06/11 18:36:46, vandebo wrote:
> Given this description, I think you don't need to check the type before you
pass
> a device id in.  See below.

Done.

https://codereview.chromium.org/329643002/diff/100001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/329643002/diff/100001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_preferences.cc:675: const
std::string& device_id = it->second.device_id;
On 2014/06/11 18:36:46, vandebo wrote:
> If PathIndicates... checks the device type instead of DCHECKing it, this can
> just be:
> 
> if (PathIndicatesIPhoto(...) || PathIndicatesITunes(...)) {
>   *gallery_info = it->second
>   return true;
> }

Done.

[vandebo (ex-Chrome), 2014-06-11 19:27:56]

LGTM

[tommycli, 2014-06-11 19:28:07]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-06-11 19:34:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/329643002/120001

[commit-bot: I haz the power, 2014-06-11 21:33:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:36:58]

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

[commit-bot: I haz the power, 2014-06-11 21:36:59]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[tommycli, 2014-06-11 21:38:21]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:39:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/329643002/120001

[commit-bot: I haz the power, 2014-06-11 21:54:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 22:20:41]

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

[commit-bot: I haz the power, 2014-06-11 22:20:42]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[tommycli, 2014-06-11 22:56:46]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:58:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/329643002/120001

[commit-bot: I haz the power, 2014-06-11 23:07:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 23:08:28]

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

[commit-bot: I haz the power, 2014-06-11 23:08:29]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[tommycli, 2014-06-12 16:04:47]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2014-06-12 16:06:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tommycli@chromium.org/329643002/120001

[commit-bot: I haz the power, 2014-06-12 19:32:31]

Change committed as 276771