[LargeIconService] Make check_seen param optional for fetching

[LargeIconService] Make check_seen param optional for fetching

LargeIconService allows fetching favicons from a Google favicon server
which has two privacy modes:
 - default - assumes that provided URLs are public and crawls the page
             if needed,
 - catious - checks first whether the provided URL has been previously
             seen by Google and drops the request if not.
Previously, all requests by Chrome used the catious mode (enabled by the
"check_seen" param).

This CL allows choosing the privacy mode per request. It also enables
the default mode for the "Articles for you" application.

BUG=536988
Review-Url: https://codereview.chromium.org/2896803002
Cr-Commit-Position: refs/heads/master@{#474233}
Committed: https://chromium.googlesource.com/chromium/src/+/5de6dffd5e148cbbad905eb6dc2b285cb7b46f2d

[jkrcal, 2017-05-22 09:03:00]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[jkrcal, 2017-05-22 09:03:19]

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

[jkrcal, 2017-05-22 09:03:20]

Peter, could you PTAL?

[commit-bot: I haz the power, 2017-05-22 09:03:20]

Dry run: 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-22 09:11:38]

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

[commit-bot: I haz the power, 2017-05-22 09:11:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jkrcal, 2017-05-22 09:33:37]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 09:33:51]

Dry run: 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-22 10:26:37]

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

[commit-bot: I haz the power, 2017-05-22 10:26:37]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-05-23 15:29:17]

Friendly ping!
(this is the CL I would like to get into M60)

[pkotwicz, 2017-05-24 05:02:25]

LGTM

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

https://codereview.chromium.org/2896803002/diff/20001/components/favicon/core...
components/favicon/core/large_icon_service.cc:47: const char kCheckSeenParam[] =
"check_seen=true&";
Random question: Does "check_seen=false" work?

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

https://codereview.chromium.org/2896803002/diff/20001/components/favicon/core...
components/favicon/core/large_icon_service_unittest.cc:261:
TEST_F(LargeIconServiceTest, ShouldNotCheckOnPublicUrls) {
There is a lot of boiler plate code which is duplicated for each test. It would
be nice if the amount of boiler plate was reduced. (Perhaps for a follow up CL)

A few ideas:
- Make a local test version of
GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() which takes a
struct. That way you only need to specify non default values

- Add a helper function which runs the test.

- Build |kExpectedServerUrlWithoutCheck| from a "default expected URL".

e.g.
const GURL kExpectedServerUrlWithoutCheck =
ReplaceQueryParameter(kDefaultExpectedServerUrl, "check_seen=true", "");

Of course, ReplaceQueryParameter() could be more complicated

[mastiz, 2017-05-24 05:55:16]

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

[mastiz, 2017-05-24 05:55:16]

LGTM.

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

https://codereview.chromium.org/2896803002/diff/20001/components/favicon/core...
components/favicon/core/large_icon_service_unittest.cc:261:
TEST_F(LargeIconServiceTest, ShouldNotCheckOnPublicUrls) {
On 2017/05/24 05:02:25, pkotwicz wrote:
> There is a lot of boiler plate code which is duplicated for each test. It
would
> be nice if the amount of boiler plate was reduced. (Perhaps for a follow up
CL)
> 
> A few ideas:
> - Make a local test version of
> GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() which takes a
> struct. That way you only need to specify non default values
> 
> - Add a helper function which runs the test.

My two cents: I consider this last proposal an anti-pattern, a last-resort
alternative when duplication is out of hands and no better alternative exists. I
think neither of these conditions hold true, and hence I'd favor
self-explanatory, more readable tests over improved code duplication.

> 
> - Build |kExpectedServerUrlWithoutCheck| from a "default expected URL".
> 
> e.g.
> const GURL kExpectedServerUrlWithoutCheck =
> ReplaceQueryParameter(kDefaultExpectedServerUrl, "check_seen=true", "");
> 
> Of course, ReplaceQueryParameter() could be more complicated

This other alternatives sound better to me, since I feel that the most severe
duplication here is not about the test fixture (e.g. irrelevant
|desired_size_in_pixel|) but the redundant test expectations
(|kExpectedServerUrl|). Except perhaps minor and trivial improvements like
CreateTestSkBitmap() taking default values or returning gfx::Image directly
(CreateTestImage()).

I think what we're trying to do here is to verify specifically one URL
parameter, which is "check_seen". Hence, I'd suggest either using
testing::HasSubstr() or preferably introducing a custom matcher,
QueryParameterEq("check_seen", _). This could be combined with the adoption of
ON_CALL for StartOrQueueNetworkRequest(_, _, _, _).

All this can indeed be discussed in later CLs.

[jkrcal, 2017-05-24 08:54:48]

Thanks!

Will look more into the tests in another CL.

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

https://codereview.chromium.org/2896803002/diff/20001/components/favicon/core...
components/favicon/core/large_icon_service.cc:47: const char kCheckSeenParam[] =
"check_seen=true&";
On 2017/05/24 05:02:25, pkotwicz wrote:
> Random question: Does "check_seen=false" work?

Yes. Any non-"true" value of check_seen is equivalent to skipping the parameter.

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

https://codereview.chromium.org/2896803002/diff/20001/components/favicon/core...
components/favicon/core/large_icon_service_unittest.cc:261:
TEST_F(LargeIconServiceTest, ShouldNotCheckOnPublicUrls) {
On 2017/05/24 05:55:16, mastiz wrote:
> On 2017/05/24 05:02:25, pkotwicz wrote:
> > There is a lot of boiler plate code which is duplicated for each test. It
> would
> > be nice if the amount of boiler plate was reduced. (Perhaps for a follow up
> CL)
> > 
> > A few ideas:
> > - Make a local test version of
> > GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() which takes
a
> > struct. That way you only need to specify non default values
> > 
> > - Add a helper function which runs the test.
> 
> My two cents: I consider this last proposal an anti-pattern, a last-resort
> alternative when duplication is out of hands and no better alternative exists.
I
> think neither of these conditions hold true, and hence I'd favor
> self-explanatory, more readable tests over improved code duplication.
> 
> > 
> > - Build |kExpectedServerUrlWithoutCheck| from a "default expected URL".
> > 
> > e.g.
> > const GURL kExpectedServerUrlWithoutCheck =
> > ReplaceQueryParameter(kDefaultExpectedServerUrl, "check_seen=true", "");
> > 
> > Of course, ReplaceQueryParameter() could be more complicated
> 
> This other alternatives sound better to me, since I feel that the most severe
> duplication here is not about the test fixture (e.g. irrelevant
> |desired_size_in_pixel|) but the redundant test expectations
> (|kExpectedServerUrl|). Except perhaps minor and trivial improvements like
> CreateTestSkBitmap() taking default values or returning gfx::Image directly
> (CreateTestImage()).
> 
> I think what we're trying to do here is to verify specifically one URL
> parameter, which is "check_seen". Hence, I'd suggest either using
> testing::HasSubstr() or preferably introducing a custom matcher,
> QueryParameterEq("check_seen", _). This could be combined with the adoption of
> ON_CALL for StartOrQueueNetworkRequest(_, _, _, _).
> 
> All this can indeed be discussed in later CLs.

You are right, the tests can be made crisper.
I started with shortening only this test.
I delay more work for further CLs (see the added bug).

[jkrcal, 2017-05-24 08:55:10]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-05-24 08:55:10]

The patchset sent to the CQ was uploaded after l-g-t-m from mastiz@chromium.org,
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2896803002/#ps40001
(title: "Comments on unit-test")

[commit-bot: I haz the power, 2017-05-24 08:55:27]

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-24 09:59:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495616110283590,
"parent_rev": "0ebf7116bec925d8d6dd90b5760953ad5be6a383", "commit_rev":
"5de6dffd5e148cbbad905eb6dc2b285cb7b46f2d"}

[commit-bot: I haz the power, 2017-05-24 10:00:00]

Description was changed from

==========
[LargeIconService] Make check_seen param optional for fetching

LargeIconService allows fetching favicons from a Google favicon server
which has two privacy modes:
 - default - assumes that provided URLs are public and crawls the page
             if needed,
 - catious - checks first whether the provided URL has been previously
             seen by Google and drops the request if not.
Previously, all requests by Chrome used the catious mode (enabled by the
"check_seen" param).

This CL allows choosing the privacy mode per request. It also enables
the default mode for the "Articles for you" application.

BUG=536988
==========

to

==========
[LargeIconService] Make check_seen param optional for fetching

LargeIconService allows fetching favicons from a Google favicon server
which has two privacy modes:
 - default - assumes that provided URLs are public and crawls the page
             if needed,
 - catious - checks first whether the provided URL has been previously
             seen by Google and drops the request if not.
Previously, all requests by Chrome used the catious mode (enabled by the
"check_seen" param).

This CL allows choosing the privacy mode per request. It also enables
the default mode for the "Articles for you" application.

BUG=536988

Review-Url: https://codereview.chromium.org/2896803002
Cr-Commit-Position: refs/heads/master@{#474233}
Committed:
https://chromium.googlesource.com/chromium/src/+/5de6dffd5e148cbbad905eb6dc2b...
==========

[commit-bot: I haz the power, 2017-05-24 10:00:01]

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