[LargeIconService] Make check_seen param optional for fetching

components/favicon/core/large_icon_service_unittest.cc

Index: components/favicon/core/large_icon_service_unittest.cc
diff --git a/components/favicon/core/large_icon_service_unittest.cc b/components/favicon/core/large_icon_service_unittest.cc
index 000df89bfc0d2fa0cee2ff75c4a0e0b9f797d8e9..8292d3d01c1e3946699bebc0e2ed22e54fe3f4ee 100644
--- a/components/favicon/core/large_icon_service_unittest.cc
+++ b/components/favicon/core/large_icon_service_unittest.cc
@@ -159,7 +159,8 @@ TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServer) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           GURL(kDummyUrl), /*min_source_size_in_pixel=*/42,
-          /*desired_size_in_pixel=*/61, callback.Get());
+          /*desired_size_in_pixel=*/61, /*may_page_url_be_private=*/true,
+          callback.Get());
 
   EXPECT_CALL(callback, Run(true));
   base::RunLoop().RunUntilIdle();
@@ -172,13 +173,13 @@ TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServerWithCustomUrl) {
       "LargeIconServiceFetching",
       {{"request_format",
         "https://t0.gstatic.com/"
-        "faviconV2?size=%d&min_size=%d&max_size=%d&url=%s"},
+        "faviconV2?%ssize=%d&min_size=%d&max_size=%d&url=%s"},
        {"enforced_min_size_in_pixel", "43"},
        {"desired_to_max_size_factor", "1.5"}},
       {"LargeIconServiceFetching"});
   const GURL kExpectedServerUrl(
-      "https://t0.gstatic.com/"
-      "faviconV2?size=61&min_size=43&max_size=91&url=http://www.example.com/");
+      "https://t0.gstatic.com/faviconV2?check_seen=true&"
+      "size=61&min_size=43&max_size=91&url=http://www.example.com/");
 
   EXPECT_CALL(mock_favicon_service_, UnableToDownloadFavicon(_)).Times(0);
 
@@ -195,7 +196,8 @@ TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServerWithCustomUrl) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           GURL(kDummyUrl), /*min_source_size_in_pixel=*/42,
-          /*desired_size_in_pixel=*/61, callback.Get());
+          /*desired_size_in_pixel=*/61, /*may_page_url_be_private=*/true,
+          callback.Get());
 
   EXPECT_CALL(callback, Run(true));
   base::RunLoop().RunUntilIdle();
@@ -225,7 +227,8 @@ TEST_F(LargeIconServiceTest, ShouldGetFromGoogleServerWithOriginalUrl) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           GURL(kDummyUrl), /*min_source_size_in_pixel=*/42,
-          /*desired_size_in_pixel=*/61, callback.Get());
+          /*desired_size_in_pixel=*/61, /*may_page_url_be_private=*/true,
+          callback.Get());
 
   EXPECT_CALL(callback, Run(true));
   base::RunLoop().RunUntilIdle();
@@ -249,7 +252,32 @@ TEST_F(LargeIconServiceTest, ShouldTrimQueryParametersForGoogleServer) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           GURL(kDummyUrlWithQuery), /*min_source_size_in_pixel=*/42,
-          /*desired_size_in_pixel=*/61, base::Callback<void(bool success)>());
+          /*desired_size_in_pixel=*/61, /*may_page_url_be_private=*/true,
+          base::Callback<void(bool success)>());
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(LargeIconServiceTest, ShouldNotCheckOnPublicUrls) {

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

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]

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.
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]

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).

+  const GURL kExpectedServerUrlWithoutCheck(
+      "https://t0.gstatic.com/faviconV2?client=chrome&drop_404_icon=true"
+      "&size=61&min_size=42&max_size=122"
+      "&fallback_opts=TYPE,SIZE,URL&url=http://www.example.com/");
+
+  EXPECT_CALL(
+      *mock_image_fetcher_,
+      StartOrQueueNetworkRequest(_, kExpectedServerUrlWithoutCheck, _, _))
+      .WillOnce(PostFetchReply(gfx::Image::CreateFrom1xBitmap(
+          CreateTestSkBitmap(64, 64, kTestColor))));
+  EXPECT_CALL(
+      mock_favicon_service_,
+      SetLastResortFavicons(_, kExpectedServerUrlWithoutCheck, _, _, _));
+
+  large_icon_service_
+      .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
+          GURL(kDummyUrl), /*min_source_size_in_pixel=*/42,
+          /*desired_size_in_pixel=*/61, /*may_page_url_be_private=*/false,
+          base::Callback<void(bool success)>());
 
   base::RunLoop().RunUntilIdle();
 }
@@ -265,7 +293,8 @@ TEST_F(LargeIconServiceTest, ShouldNotQueryGoogleServerIfInvalidScheme) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           GURL(kDummyFtpUrl), /*min_source_size_in_pixel=*/42,
-          /*desired_size_in_pixel=*/61, callback.Get());
+          /*desired_size_in_pixel=*/61, /*may_page_url_be_private=*/true,
+          callback.Get());
 
   EXPECT_CALL(callback, Run(false));
   base::RunLoop().RunUntilIdle();
@@ -294,7 +323,8 @@ TEST_F(LargeIconServiceTest, ShouldReportUnavailableIfFetchFromServerFails) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           kDummyUrlWithQuery, /*min_source_size_in_pixel=*/42,
-          /*desired_size_in_pixel=*/61, callback.Get());
+          /*desired_size_in_pixel=*/61, /*may_page_url_be_private=*/true,
+          callback.Get());
 
   EXPECT_CALL(callback, Run(false));
   base::RunLoop().RunUntilIdle();
@@ -322,7 +352,8 @@ TEST_F(LargeIconServiceTest, ShouldNotGetFromGoogleServerIfUnavailable) {
   large_icon_service_
       .GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
           GURL(kDummyUrl), /*min_source_size_in_pixel=*/42,
-          /*desired_size_in_pixel=*/61, callback.Get());
+          /*desired_size_in_pixel=*/61, /*may_page_url_be_private=*/true,
+          callback.Get());
 
   EXPECT_CALL(callback, Run(false));
   base::RunLoop().RunUntilIdle();