Put all scan results in the media galleries scan result dialog.

Put all scan results in the media galleries scan result dialog.

Previously, only galleries of type scan results where added, now any gallery that has media file counts and the app doesn't have permission to is displayed.

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

[vandebo (ex-Chrome), 2014-02-08 00:45:40]

tommy: first pass review
lei: additional comments

[tommycli, 2014-02-10 18:45:52]

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

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:255: if
(gallery.IsBlackListedType() || gallery.audio_count ||
Should this be all blacklisted  galleries, or only kRemovedScan galleries?

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc
(right):

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:231:
if ((gallery.audio_count || gallery.image_count || gallery.video_count) &&
I believe this will cause galleries with zero audio/image/video count to be
hidden. I think this will hide kAutoDetected galleries.

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc
(right):

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:111:
extension_ = AddMediaGalleriesApp("allauto", read_permissions,
why did the name change to 'allauto'?

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:242:
// Start with two scan results.
comment is out of date if you intend to test with kAutodetected also

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:245:
AddGallery("auto_id", MediaGalleryPrefInfo::kAutoDetected, 2, 0, 0);
I think kAutoDetected Galleries need to have 0, 0, 0 counts.

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:282:
// Start with two scan results.
out of date

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:285:
AddGallery("auto_id", MediaGalleryPrefInfo::kAutoDetected, 2, 0, 0);
0, 0, 0

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:450:
AddGallery("second", MediaGalleryPrefInfo::kAutoDetected, 9, 0, 0);
0, 0, 0

[Lei Zhang, 2014-02-10 19:55:22]

Just sending whatever I had queued up on Friday.

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

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:255: if
(gallery.IsBlackListedType() || gallery.audio_count ||
Can you add a comment to explain why we are checking this? The last 3/4s of this
if statement is equivalent to !IsEmptyScanResult() if you change the 3 fields in
MediaGalleryPrefInfo to a struct MediaGalleryScanResult.

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc
(right):

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:155:
int audio_count, int image_count,
Switch to MediaGalleryScanResult?

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:386:
EXPECT_EQ(4U, controller()->GetGalleryList().size());
Why expect?

[vandebo (ex-Chrome), 2014-02-11 19:19:30]

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

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:255: if
(gallery.IsBlackListedType() || gallery.audio_count ||
On 2014/02/10 18:45:53, tommycli wrote:
> Should this be all blacklisted  galleries, or only kRemovedScan galleries?

Blacklisted galleries in general shouldn't have their file counts stored.

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc
(right):

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:231:
if ((gallery.audio_count || gallery.image_count || gallery.video_count) &&
On 2014/02/10 18:45:53, tommycli wrote:
> I believe this will cause galleries with zero audio/image/video count to be
> hidden. I think this will hide kAutoDetected galleries.

Yes, it will hide galleries with no media files.  It won't hide auto detected
galleries in general, only if they don't have any media files when a scan is
done.

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc
(right):

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:111:
extension_ = AddMediaGalleriesApp("allauto", read_permissions,
On 2014/02/10 18:45:53, tommycli wrote:
> why did the name change to 'allauto'?

incompletely reverted change.  Fixed.

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:155:
int audio_count, int image_count,
On 2014/02/10 19:55:22, Lei Zhang wrote:
> Switch to MediaGalleryScanResult?

For the testing code, it's easier to take three ints as args.

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:242:
// Start with two scan results.
On 2014/02/10 18:45:53, tommycli wrote:
> comment is out of date if you intend to test with kAutodetected also

Nope - we haven't done a scan, so all the existing galleries have no media files
in prefs.

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:245:
AddGallery("auto_id", MediaGalleryPrefInfo::kAutoDetected, 2, 0, 0);
On 2014/02/10 18:45:53, tommycli wrote:
> I think kAutoDetected Galleries need to have 0, 0, 0 counts.

Why do you say that?

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller_unittest.cc:386:
EXPECT_EQ(4U, controller()->GetGalleryList().size());
On 2014/02/10 19:55:22, Lei Zhang wrote:
> Why expect?

Changed to assert

[tommycli, 2014-02-11 19:32:26]

vandebo: okay after our convo these changes make more sense to me now.

lgtm if you update the comments on 'int audio_count; ...' in
media_galleries_preferences.h

[vandebo (ex-Chrome), 2014-02-11 19:34:54]

On 2014/02/11 19:32:26, tommycli wrote:
> vandebo: okay after our convo these changes make more sense to me now.
> 
> lgtm if you update the comments on 'int audio_count; ...' in
> media_galleries_preferences.h

Done.

Lei, any additional comments?

[vandebo (ex-Chrome), 2014-02-13 18:27:08]

On 2014/02/11 19:34:54, vandebo wrote:
> On 2014/02/11 19:32:26, tommycli wrote:
> > vandebo: okay after our convo these changes make more sense to me now.
> > 
> > lgtm if you update the comments on 'int audio_count; ...' in
> > media_galleries_preferences.h
> 
> Done.
> 
> Lei, any additional comments?

Lei: ping

[Lei Zhang, 2014-02-13 22:08:03]

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

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:255: if
(gallery.IsBlackListedType() || gallery.audio_count ||
On 2014/02/11 19:19:31, vandebo wrote:
> On 2014/02/10 18:45:53, tommycli wrote:
> > Should this be all blacklisted  galleries, or only kRemovedScan galleries?
> 
> Blacklisted galleries in general shouldn't have their file counts stored.

But the code says:

if (gallery.IsBlackListedType() || ...) {
  dict->SetiInteger(...)
}

Isn't that a contradiction? What am I missing? Shouldn't this be:

if (!gallery.IsBlackLsitedType() && (...)) ?

[vandebo (ex-Chrome), 2014-02-13 22:10:01]

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

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:255: if
(gallery.IsBlackListedType() || gallery.audio_count ||
On 2014/02/13 22:08:04, Lei Zhang wrote:
> On 2014/02/11 19:19:31, vandebo wrote:
> > On 2014/02/10 18:45:53, tommycli wrote:
> > > Should this be all blacklisted  galleries, or only kRemovedScan galleries?
> > 
> > Blacklisted galleries in general shouldn't have their file counts stored.
> 
> But the code says:
> 
> if (gallery.IsBlackListedType() || ...) {
>   dict->SetiInteger(...)
> }
> 
> Isn't that a contradiction? What am I missing? Shouldn't this be:
> 
> if (!gallery.IsBlackLsitedType() && (...)) ?

This is at a lower level - when we mark a gallery as blacklisted, this lets us
clear the file count out of preferences.

[Lei Zhang, 2014-02-14 07:34:02]

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

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:255: if
(gallery.IsBlackListedType() || gallery.audio_count ||
On 2014/02/13 22:10:02, vandebo wrote:
> On 2014/02/13 22:08:04, Lei Zhang wrote:
> > On 2014/02/11 19:19:31, vandebo wrote:
> > > On 2014/02/10 18:45:53, tommycli wrote:
> > > > Should this be all blacklisted  galleries, or only kRemovedScan
galleries?
> > > 
> > > Blacklisted galleries in general shouldn't have their file counts stored.
> > 
> > But the code says:
> > 
> > if (gallery.IsBlackListedType() || ...) {
> >   dict->SetiInteger(...)
> > }
> > 
> > Isn't that a contradiction? What am I missing? Shouldn't this be:
> > 
> > if (!gallery.IsBlackLsitedType() && (...)) ?
> 
> This is at a lower level - when we mark a gallery as blacklisted, this lets us
> clear the file count out of preferences.

So you are trying to zero them out if the type is blacklisted?

CreateGalleryPrefInfoDictionary() only has 1 caller, which is
MediaGalleriesPreferences::AddGalleryInternal(). At the very top, it has a
DCHECK() that essentially says gallery.IsBlackListedType() cannot ever be true
here.

https://codereview.chromium.org/148063006/diff/170006/chrome/browser/media_ga...
File
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc
(right):

https://codereview.chromium.org/148063006/diff/170006/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:226:
// access to or isn't in |results_to_remove_|.
nit: |results_to_remove_| -> |ignore_list|

https://codereview.chromium.org/148063006/diff/170006/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:246:
// Remove anything from |scan_results_| that's no longer valid or the user
nit: |scan_results_| -> |scan_results|

https://codereview.chromium.org/148063006/diff/170006/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:282:
const std::string& extension_id, MediaGalleryPrefId pref_id) {
drop |pref_id| since it's no longer used?

[vandebo (ex-Chrome), 2014-02-14 19:11:53]

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

https://codereview.chromium.org/148063006/diff/50001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:255: if
(gallery.IsBlackListedType() || gallery.audio_count ||
On 2014/02/14 07:34:02, Lei Zhang wrote:
> On 2014/02/13 22:10:02, vandebo wrote:
> > On 2014/02/13 22:08:04, Lei Zhang wrote:
> > > On 2014/02/11 19:19:31, vandebo wrote:
> > > > On 2014/02/10 18:45:53, tommycli wrote:
> > > > > Should this be all blacklisted  galleries, or only kRemovedScan
> galleries?
> > > > 
> > > > Blacklisted galleries in general shouldn't have their file counts
stored.
> > > 
> > > But the code says:
> > > 
> > > if (gallery.IsBlackListedType() || ...) {
> > >   dict->SetiInteger(...)
> > > }
> > > 
> > > Isn't that a contradiction? What am I missing? Shouldn't this be:
> > > 
> > > if (!gallery.IsBlackLsitedType() && (...)) ?
> > 
> > This is at a lower level - when we mark a gallery as blacklisted, this lets
us
> > clear the file count out of preferences.
> 
> So you are trying to zero them out if the type is blacklisted?
> 
> CreateGalleryPrefInfoDictionary() only has 1 caller, which is
> MediaGalleriesPreferences::AddGalleryInternal(). At the very top, it has a
> DCHECK() that essentially says gallery.IsBlackListedType() cannot ever be true
> here.

Indeed you are correct - EraseOrBlacklistGalleryById() takes care of zeroing out
the file counts.

https://codereview.chromium.org/148063006/diff/170006/chrome/browser/media_ga...
File
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc
(right):

https://codereview.chromium.org/148063006/diff/170006/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:226:
// access to or isn't in |results_to_remove_|.
On 2014/02/14 07:34:02, Lei Zhang wrote:
> nit: |results_to_remove_| -> |ignore_list|

Done.

https://codereview.chromium.org/148063006/diff/170006/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:246:
// Remove anything from |scan_results_| that's no longer valid or the user
On 2014/02/14 07:34:02, Lei Zhang wrote:
> nit: |scan_results_| -> |scan_results|

Done.

https://codereview.chromium.org/148063006/diff/170006/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:282:
const std::string& extension_id, MediaGalleryPrefId pref_id) {
On 2014/02/14 07:34:02, Lei Zhang wrote:
> drop |pref_id| since it's no longer used?

Done.

[Lei Zhang, 2014-02-14 19:29:32]

lgtm

[Lei Zhang, 2014-02-14 19:29:38]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-02-14 19:30:30]

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

[commit-bot: I haz the power, 2014-02-14 22:25:44]

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

[commit-bot: I haz the power, 2014-02-14 22:25:46]

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

[vandebo (ex-Chrome), 2014-02-14 22:51:32]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-02-14 22:52:04]

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

[commit-bot: I haz the power, 2014-02-15 00:56:38]

Change committed as 251465