Media galleries preferences also needs to track file counts for scan results.

Media galleries preferences also needs to track file counts for scan results.

We need the file counts in order to send the completion event if we return a cached scan.  Also, in the scan result dialog we want to present the most interesting results first.

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

[Lei Zhang, 2014-01-15 22:13:43]

Can you briefly mention why we need this in the CL description?

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/media_galleries_preferences.cc (left):

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/media_galleries_preferences.cc:847: if
(volume_metadata_valid) {
Why is this changing?

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/media_galleries_preferences.h (right):

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/media_galleries_preferences.h:215: int
image_count,
The number of parameters is too damn high!

[vandebo (ex-Chrome), 2014-01-15 22:19:44]

On 2014/01/15 22:13:43, Lei Zhang wrote:
> Can you briefly mention why we need this in the CL description?

Done.

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/media_galleries_preferences.cc (left):

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/media_galleries_preferences.cc:847: if
(volume_metadata_valid) {
On 2014/01/15 22:13:43, Lei Zhang wrote:
> Why is this changing?

The thing below this conditionally sets the fields, so it seems like a minor
clean up to just set all of the things here and let the thing below do what it
wants.  Instead of doing it multiple times, or in the worse case, doing
different things in the two places.

[Lei Zhang, 2014-01-15 22:38:23]

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/media_galleries_preferences.cc (left):

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/media_galleries_preferences.cc:847: if
(volume_metadata_valid) {
On 2014/01/15 22:19:44, vandebo wrote:
> On 2014/01/15 22:13:43, Lei Zhang wrote:
> > Why is this changing?
> 
> The thing below this conditionally sets the fields, so it seems like a minor
> clean up to just set all of the things here and let the thing below do what it
> wants.  Instead of doing it multiple times, or in the worse case, doing
> different things in the two places.

Right now this change is a no-op because every place where we pass in
|volume_metadata_valid| as false, we also pass dummy values for the parameters
in the if block. However, if a caller does not pass in dummy values, then the
behavior changes in a way that does not make sense.

I'd either preserve the current behavior, or get rid of |volume_metadata_valid|
and infer it from the other parameters.

[vandebo (ex-Chrome), 2014-01-15 22:43:04]

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
File chrome/browser/media_galleries/media_galleries_preferences.cc (left):

https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
chrome/browser/media_galleries/media_galleries_preferences.cc:847: if
(volume_metadata_valid) {
On 2014/01/15 22:38:23, Lei Zhang wrote:
> On 2014/01/15 22:19:44, vandebo wrote:
> > On 2014/01/15 22:13:43, Lei Zhang wrote:
> > > Why is this changing?
> > 
> > The thing below this conditionally sets the fields, so it seems like a minor
> > clean up to just set all of the things here and let the thing below do what
it
> > wants.  Instead of doing it multiple times, or in the worse case, doing
> > different things in the two places.
> 
> Right now this change is a no-op because every place where we pass in
> |volume_metadata_valid| as false, we also pass dummy values for the parameters
> in the if block. However, if a caller does not pass in dummy values, then the
> behavior changes in a way that does not make sense.
> 
> I'd either preserve the current behavior, or get rid of
|volume_metadata_valid|
> and infer it from the other parameters.

I don't see that: in CreateGalleryPrefInfoDictionary, if volume_metadata_valid
is true, we set the values in preferences, if not, we don't. line 240

[Lei Zhang, 2014-01-15 22:49:14]

On 2014/01/15 22:43:04, vandebo wrote:
>
https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
> File chrome/browser/media_galleries/media_galleries_preferences.cc (left):
> 
>
https://codereview.chromium.org/140033003/diff/1/chrome/browser/media_galleri...
> chrome/browser/media_galleries/media_galleries_preferences.cc:847: if
> (volume_metadata_valid) {
> On 2014/01/15 22:38:23, Lei Zhang wrote:
> > On 2014/01/15 22:19:44, vandebo wrote:
> > > On 2014/01/15 22:13:43, Lei Zhang wrote:
> > > > Why is this changing?
> > > 
> > > The thing below this conditionally sets the fields, so it seems like a
minor
> > > clean up to just set all of the things here and let the thing below do
what
> it
> > > wants.  Instead of doing it multiple times, or in the worse case, doing
> > > different things in the two places.
> > 
> > Right now this change is a no-op because every place where we pass in
> > |volume_metadata_valid| as false, we also pass dummy values for the
parameters
> > in the if block. However, if a caller does not pass in dummy values, then
the
> > behavior changes in a way that does not make sense.
> > 
> > I'd either preserve the current behavior, or get rid of
> |volume_metadata_valid|
> > and infer it from the other parameters.
> 
> I don't see that: in CreateGalleryPrefInfoDictionary, if volume_metadata_valid
> is true, we set the values in preferences, if not, we don't. line 240

Ah, I see what you mean. |volume_metadata_valid| gets passed on to the
consumers, so it's ultimately up to them to interpret it correctly.

lgtm

[commit-bot: I haz the power, 2014-01-15 23:24:55]

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

[commit-bot: I haz the power, 2014-01-16 00:37:56]

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

[vandebo (ex-Chrome), 2014-01-16 16:39:59]

One more call site

[commit-bot: I haz the power, 2014-01-16 16:40:28]

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

[commit-bot: I haz the power, 2014-01-16 19:33:27]

Change committed as 245281