Modify a favicon DCHECK.

Modify a favicon DCHECK.

There may be more than one favicon if loading an extension favicon,
which loads for all supported scale factors immediately rather than
in an as-needed fashion.

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

[Daniel Nishi, 2014-07-25 18:09:36]

PTAL.

I suspect that this DCHECK is hit when going to chrome://favicon for
chrome-extension addresses because of
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
adding two images into its |info_list| and then failing the DCHECK on the
callback.

[Daniel Nishi, 2014-07-25 18:40:58]

Actually, thinking about it, this DCHECK was probably intended to guarantee that
at least one element existed so that favicon_bitmap_results[0] existed for
assignment. I've uploaded a new CL with the DCHECK modified as such.

WDYT?

[stevenjb, 2014-07-25 18:45:20]

https://codereview.chromium.org/421633002/diff/20001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/421633002/diff/20001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.cc:406: DCHECK_LE(1u,
favicon_bitmap_results.size());
We early exit above if favicon_bitmap_results.empty(), so this is effectively
the same test.

[stevenjb, 2014-07-25 18:47:31]

https://codereview.chromium.org/421633002/diff/20001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/421633002/diff/20001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.cc:406: DCHECK_LE(1u,
favicon_bitmap_results.size());
On 2014/07/25 18:45:19, stevenjb wrote:
> We early exit above if favicon_bitmap_results.empty(), so this is effectively
> the same test.

I *think* (it's been a long time) was that the expectation was that at this
point there would only be one valid result? That may have changed, so maybe a GE
test would be correct here?
+sky@

[sky, 2014-07-25 19:23:46]

LGTM

[Daniel Nishi, 2014-07-25 20:19:57]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-07-25 20:21:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dhnishi@chromium.org/421633002/40001

[Daniel Nishi, 2014-07-25 21:11:45]

The CQ bit was unchecked by dhnishi@chromium.org

[Daniel Nishi, 2014-07-25 21:13:37]

https://codereview.chromium.org/421633002/diff/20001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/421633002/diff/20001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.cc:406: DCHECK_LE(1u,
favicon_bitmap_results.size());
On 2014/07/25 18:45:19, stevenjb wrote:
> We early exit above if favicon_bitmap_results.empty(), so this is effectively
> the same test.

Yeah, you're right. I've changed it back to the removed version.

[Daniel Nishi, 2014-07-25 21:13:46]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-07-25 21:14:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dhnishi@chromium.org/421633002/60001

[commit-bot: I haz the power, 2014-07-25 23:45:02]

Change committed as 285725