Verify favicon bitmap content in FaviconHandler tests

Verify favicon bitmap content in FaviconHandler tests

Follow up to the test refactoring series in crbug.com/694312, addressing
two TODOs about verifying the actual content of the bitmaps, which is
done based on the color of one arbitrary pixel.

BUG=700811
Review-Url: https://codereview.chromium.org/2872643002
Cr-Commit-Position: refs/heads/master@{#470489}
Committed: https://chromium.googlesource.com/chromium/src/+/5730d628131f2ac3cd4a9a834a3fe812cc570ade

[mastiz, 2017-05-08 15:25:06]

mastiz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[mastiz, 2017-05-08 15:25:07]

As promised.

[pkotwicz, 2017-05-08 17:11:16]

The CL looks mostly good. A few questions

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:59: // |arg| is a SkBitmap.
Isn't arg a gfx::Image?

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:597:
EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL, _, _));
Nit: We should probably check the color of the gfx::Image passed to
SetFavicons() too

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:655:
AllOf(ImageSizeIs(gfx::kFaviconSize, gfx::kFaviconSize),
Why are we checking the size of the gfx::Image passed to OnFaviconUpdated()

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:1154:
ImageColorIs(kColor1))));
Do we need to check the color of the gfx::Image? We are already checking the
icon URL

[mastiz, 2017-05-09 08:29:46]

Thanks, PTAL.

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:59: // |arg| is a SkBitmap.
On 2017/05/08 17:11:16, pkotwicz wrote:
> Isn't arg a gfx::Image?

Fixed, thanks.

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:597:
EXPECT_CALL(favicon_service_, SetFavicons(_, kIconURL, _, _));
On 2017/05/08 17:11:16, pkotwicz wrote:
> Nit: We should probably check the color of the gfx::Image passed to
> SetFavicons() too

Done.

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:655:
AllOf(ImageSizeIs(gfx::kFaviconSize, gfx::kFaviconSize),
On 2017/05/08 17:11:15, pkotwicz wrote:
> Why are we checking the size of the gfx::Image passed to OnFaviconUpdated()

Removed. I thought it was worth verifying that the size if reported unmangled.

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:1154:
ImageColorIs(kColor1))));
On 2017/05/08 17:11:16, pkotwicz wrote:
> Do we need to check the color of the gfx::Image? We are already checking the
> icon URL

Removed.

[pkotwicz, 2017-05-09 17:48:51]

Mikel, I think that you forgot to upload your updated CL?

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:655:
AllOf(ImageSizeIs(gfx::kFaviconSize, gfx::kFaviconSize),
Thank you for the explanation. I still think that we should not check for the
size.

I am hoping to minimize the number of assertions in the tests so that it is
clear what the tests are testing (as opposed to how the tests used to be circa a
few months ago)

[mastiz, 2017-05-09 18:21:25]

On 2017/05/09 17:48:51, pkotwicz wrote:
> Mikel, I think that you forgot to upload your updated CL?
> 
>
https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
> File components/favicon/core/favicon_handler_unittest.cc (right):
> 
>
https://codereview.chromium.org/2872643002/diff/20001/components/favicon/core...
> components/favicon/core/favicon_handler_unittest.cc:655:
> AllOf(ImageSizeIs(gfx::kFaviconSize, gfx::kFaviconSize),
> Thank you for the explanation. I still think that we should not check for the
> size.
> 
> I am hoping to minimize the number of assertions in the tests so that it is
> clear what the tests are testing (as opposed to how the tests used to be circa
a
> few months ago)

PTAL now, I indeed forgot or failed to upload the latest version!

[pkotwicz, 2017-05-10 04:35:10]

LGTM

[mastiz, 2017-05-10 04:52:53]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2017-05-10 04:53:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 05:42:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494391973569770,
"parent_rev": "9b5ebac3230f3c90782296ae95396e7f512729bc", "commit_rev":
"5730d628131f2ac3cd4a9a834a3fe812cc570ade"}

[commit-bot: I haz the power, 2017-05-10 05:43:08]

Description was changed from

==========
Verify favicon bitmap content in FaviconHandler tests

Follow up to the test refactoring series in crbug.com/694312, addressing
two TODOs about verifying the actual content of the bitmaps, which is
done based on the color of one arbitrary pixel.

BUG=700811
==========

to

==========
Verify favicon bitmap content in FaviconHandler tests

Follow up to the test refactoring series in crbug.com/694312, addressing
two TODOs about verifying the actual content of the bitmaps, which is
done based on the color of one arbitrary pixel.

BUG=700811

Review-Url: https://codereview.chromium.org/2872643002
Cr-Commit-Position: refs/heads/master@{#470489}
Committed:
https://chromium.googlesource.com/chromium/src/+/5730d628131f2ac3cd4a9a834a3f...
==========

[commit-bot: I haz the power, 2017-05-10 05:43:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5730d628131f2ac3cd4a9a834a3f...