[webui][ntp] Add support for dark doodle logging URLs

Logging for dark image doodles works identical to light image doodles
but with different logging URLs.

+ Remove DoodleLogging feature flag. Doodle logging is enabled since a
  long time and there is other code that assumes doodle logging is
  enabled.

Bug: 688960
Change-Id: If825a29abb8e23c11fa837cac4fd7288e0107fa2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2262057
Commit-Queue: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Auto-Submit: Tibor Goldschwendt <tiborg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782005}

chrome/browser/resources/new_tab_page/logo.js

@@ -167,11 +167,11 @@ class LogoElement extends PolymerElement {
       }
     });
     /** @private {?string} */
-    this.imageClickParams_;
+    this.imageClickParams_ = null;
     /** @private {url.mojom.Url} */
-    this.interactionLogUrl_;
+    this.interactionLogUrl_ = null;
     /** @private {?string} */
-    this.shareId_;
+    this.shareId_ = null;
   }
 
   /** @override */
@@ -230,6 +230,12 @@ class LogoElement extends PolymerElement {
         '--ntp-logo-box-color': null,
       });
     }
+    // Stop the animation (if it is running) and reset logging params since
+    // mode change constitutes a new doodle session.
+    this.showAnimation_ = false;
+    this.imageClickParams_ = null;
+    this.interactionLogUrl_ = null;
+    this.shareId_ = null;
   }
 
   /**
@@ -301,7 +307,7 @@ class LogoElement extends PolymerElement {
       this.logImageRendered_(
           newTabPage.mojom.DoodleImageType.ANIMATION,
           /** @type {!url.mojom.Url} */
-          (this.doodle_.content.imageDoodle.animationImpressionLogUrl));
+          (this.imageDoodle_.animationImpressionLogUrl));
 
       return;
     }
@@ -323,7 +329,7 @@ class LogoElement extends PolymerElement {
     this.logImageRendered_(
         this.isCtaImageShown_() ? newTabPage.mojom.DoodleImageType.CTA :
                                   newTabPage.mojom.DoodleImageType.STATIC,
-        this.doodle_.content.imageDoodle.imageImpressionLogUrl);
+        this.imageDoodle_.imageImpressionLogUrl);
   }
 
   /**

chrome/browser/search_engines/template_url_service_factory_android.cc

@@ -7,7 +7,6 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "components/search_engines/template_url_service.h"
-#include "components/search_provider_logos/features.h"
 #include "components/search_provider_logos/switches.h"
 
 namespace {

chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom

@@ -208,6 +208,9 @@ struct ImageDoodle {
   skia.mojom.SkColor background_color;
   // Specification of the share button.
   DoodleShareButton share_button;
+  // URLs to be pinged when an image has been shown.
+  url.mojom.Url image_impression_log_url;
+  url.mojom.Url? animation_impression_log_url;
 };
 
 // The contents of simple and animated doodles.
@@ -219,9 +222,6 @@ struct ImageDoodleContent {
   url.mojom.Url on_click_url;
   // URL displayed to users, which they can use to share the doodle.
   url.mojom.Url share_url;
-  // URLs to be pinged when an image has been shown.
-  url.mojom.Url image_impression_log_url;
-  url.mojom.Url? animation_impression_log_url;
 };
 
 // The contents of interactive doodles.

chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc

@@ -254,7 +254,9 @@ new_tab_page::mojom::ImageDoodlePtr MakeImageDoodle(
     int share_button_x,
     int share_button_y,
     const std::string& share_button_icon,
-    const std::string& share_button_bg) {
+    const std::string& share_button_bg,
+    GURL log_url,
+    GURL cta_log_url) {
   auto doodle = new_tab_page::mojom::ImageDoodle::New();
   std::string base64;
   base::Base64Encode(data, &base64);
@@ -272,6 +274,12 @@ new_tab_page::mojom::ImageDoodlePtr MakeImageDoodle(
   doodle->share_button->icon_url = GURL(base::StringPrintf(
       "data:image/png;base64,%s", share_button_icon.c_str()));
   doodle->share_button->background_color = ParseHexColor(share_button_bg);
+  if (type == search_provider_logos::LogoType::ANIMATED) {
+    doodle->image_impression_log_url = cta_log_url;
+    doodle->animation_impression_log_url = log_url;
+  } else {
+    doodle->image_impression_log_url = log_url;
+  }
   return doodle;
 }
 
@@ -1259,7 +1267,8 @@ void NewTabPageHandler::OnLogoAvailable(
         logo->metadata.mime_type, logo->metadata.animated_url,
         logo->metadata.width_px, logo->metadata.height_px, "#ffffff",
         logo->metadata.share_button_x, logo->metadata.share_button_y,
-        logo->metadata.share_button_icon, logo->metadata.share_button_bg);
+        logo->metadata.share_button_icon, logo->metadata.share_button_bg,
+        logo->metadata.log_url, logo->metadata.cta_log_url);
     if (logo->dark_encoded_image) {
       image_doodle_content->dark = MakeImageDoodle(
           logo->metadata.type, logo->dark_encoded_image->data(),
@@ -1269,18 +1278,11 @@ void NewTabPageHandler::OnLogoAvailable(
           logo->metadata.dark_share_button_x,
           logo->metadata.dark_share_button_y,
           logo->metadata.dark_share_button_icon,
-          logo->metadata.dark_share_button_bg);
+          logo->metadata.dark_share_button_bg, logo->metadata.dark_log_url,
+          logo->metadata.dark_cta_log_url);
     }
     image_doodle_content->on_click_url = logo->metadata.on_click_url;
     image_doodle_content->share_url = logo->metadata.short_link;
-    if (logo->metadata.type == search_provider_logos::LogoType::ANIMATED) {
-      image_doodle_content->image_impression_log_url =
-          logo->metadata.cta_log_url;
-      image_doodle_content->animation_impression_log_url =
-          logo->metadata.log_url;
-    } else {
-      image_doodle_content->image_impression_log_url = logo->metadata.log_url;
-    }
     doodle->content = new_tab_page::mojom::DoodleContent::NewImageDoodle(
         std::move(image_doodle_content));
   } else if (logo->metadata.type ==

components/search_provider_logos/BUILD.gn

@@ -4,8 +4,6 @@
 
 static_library("search_provider_logos") {
   sources = [
-    "features.cc",
-    "features.h",
     "fixed_logo_api.cc",
     "fixed_logo_api.h",
     "google_logo_api.cc",

components/search_provider_logos/features.cc

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "components/search_provider_logos/features.h"
-
-namespace search_provider_logos {
-namespace features {
-
-const base::Feature kDoodleLogging{"DoodleLogging",
-                                   base::FEATURE_ENABLED_BY_DEFAULT};
-
-}  // namespace features
-}  // namespace search_provider_logos

components/search_provider_logos/features.h

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef COMPONENTS_SEARCH_PROVIDER_LOGOS_FEATURES_H_
-#define COMPONENTS_SEARCH_PROVIDER_LOGOS_FEATURES_H_
-
-#include "base/feature_list.h"
-
-namespace search_provider_logos {
-namespace features {
-
-extern const base::Feature kDoodleLogging;
-
-}  // namespace features
-}  // namespace search_provider_logos
-
-#endif  // COMPONENTS_SEARCH_PROVIDER_LOGOS_FEATURES_H_

components/search_provider_logos/google_logo_api.cc

@@ -21,7 +21,6 @@
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
-#include "components/search_provider_logos/features.h"
 #include "components/search_provider_logos/switches.h"
 #include "url/third_party/mozilla/url_parse.h"
 #include "url/url_constants.h"
@@ -336,10 +335,11 @@ std::unique_ptr<EncodedLogo> ParseDoodleLogoResponse(
   logo->metadata.on_click_url = ParseUrl(*ddljson, "target_url", base_url);
   ddljson->GetString("alt_text", &logo->metadata.alt_text);
 
-  if (base::FeatureList::IsEnabled(features::kDoodleLogging)) {
-    logo->metadata.cta_log_url = ParseUrl(*ddljson, "cta_log_url", base_url);
-    logo->metadata.log_url = ParseUrl(*ddljson, "log_url", base_url);
-  }
+  logo->metadata.cta_log_url = ParseUrl(*ddljson, "cta_log_url", base_url);
+  logo->metadata.dark_cta_log_url =
+      ParseUrl(*ddljson, "dark_cta_log_url", base_url);
+  logo->metadata.log_url = ParseUrl(*ddljson, "log_url", base_url);
+  logo->metadata.dark_log_url = ParseUrl(*ddljson, "dark_log_url", base_url);
 
   ddljson->GetString("fingerprint", &logo->metadata.fingerprint);
 

components/search_provider_logos/google_logo_api_unittest.cc

@@ -430,7 +430,9 @@ TEST(GoogleNewLogoApiTest, ParsesLoggingUrls) {
       "url": "https://www.doodle.com/image.gif"
     },
     "log_url": "/log?a=b",
-    "cta_log_url": "/ctalog?c=d"
+    "dark_log_url": "/log?a=dark",
+    "cta_log_url": "/ctalog?c=d",
+    "dark_cta_log_url": "/ctalog?c=dark"
   }
 })json";
 
@@ -441,7 +443,10 @@ TEST(GoogleNewLogoApiTest, ParsesLoggingUrls) {
   ASSERT_FALSE(failed);
   ASSERT_TRUE(logo);
   EXPECT_EQ(GURL("https://base.doo/log?a=b"), logo->metadata.log_url);
+  EXPECT_EQ(GURL("https://base.doo/log?a=dark"), logo->metadata.dark_log_url);
   EXPECT_EQ(GURL("https://base.doo/ctalog?c=d"), logo->metadata.cta_log_url);
+  EXPECT_EQ(GURL("https://base.doo/ctalog?c=dark"),
+            logo->metadata.dark_cta_log_url);
 }
 
 TEST(GoogleNewLogoApiTest, ParsesImageSize) {

components/search_provider_logos/logo_cache.cc

@@ -37,7 +37,9 @@ const char kDarkNumBytesKey[] = "dark_num_bytes";
 const char kAnimatedUrlKey[] = "animated_url";
 const char kDarkAnimatedUrlKey[] = "dark_animated_url";
 const char kLogUrlKey[] = "log_url";
+const char kDarkLogUrlKey[] = "dark_log_url";
 const char kCtaLogUrlKey[] = "cta_log_url";
+const char kDarkCtaLogUrlKey[] = "dark_cta_log_url";
 const char kShortLinkKey[] = "short_link";
 const char kWidthPx[] = "width_px";
 const char kHeightPx[] = "height_px";
@@ -225,7 +227,9 @@ std::unique_ptr<LogoMetadata> LogoCache::LogoMetadataFromString(
   std::string animated_url;
   std::string dark_animated_url;
   std::string log_url;
+  std::string dark_log_url;
   std::string cta_log_url;
+  std::string dark_cta_log_url;
   std::string short_link;
   if (!dict->GetString(kSourceUrlKey, &source_url) ||
       !dict->GetString(kFingerprintKey, &metadata->fingerprint) ||
@@ -236,7 +240,9 @@ std::unique_ptr<LogoMetadata> LogoCache::LogoMetadataFromString(
       !dict->GetString(kAnimatedUrlKey, &animated_url) ||
       !dict->GetString(kDarkAnimatedUrlKey, &dark_animated_url) ||
       !dict->GetString(kLogUrlKey, &log_url) ||
+      !dict->GetString(kDarkLogUrlKey, &dark_log_url) ||
       !dict->GetString(kCtaLogUrlKey, &cta_log_url) ||
+      !dict->GetString(kDarkCtaLogUrlKey, &dark_cta_log_url) ||
       !dict->GetString(kShortLinkKey, &short_link) ||
       !dict->GetString(kMimeTypeKey, &metadata->mime_type) ||
       !dict->GetString(kDarkMimeTypeKey, &metadata->dark_mime_type) ||
@@ -274,7 +280,9 @@ std::unique_ptr<LogoMetadata> LogoCache::LogoMetadataFromString(
   metadata->animated_url = GURL(animated_url);
   metadata->dark_animated_url = GURL(dark_animated_url);
   metadata->log_url = GURL(log_url);
+  metadata->dark_log_url = GURL(dark_log_url);
   metadata->cta_log_url = GURL(cta_log_url);
+  metadata->dark_cta_log_url = GURL(dark_cta_log_url);
   metadata->short_link = GURL(short_link);
 
   return metadata;
@@ -295,7 +303,9 @@ void LogoCache::LogoMetadataToString(const LogoMetadata& metadata,
   dict.SetString(kAnimatedUrlKey, metadata.animated_url.spec());
   dict.SetString(kDarkAnimatedUrlKey, metadata.dark_animated_url.spec());
   dict.SetString(kLogUrlKey, metadata.log_url.spec());
+  dict.SetString(kDarkLogUrlKey, metadata.dark_log_url.spec());
   dict.SetString(kCtaLogUrlKey, metadata.cta_log_url.spec());
+  dict.SetString(kDarkCtaLogUrlKey, metadata.dark_cta_log_url.spec());
   dict.SetString(kShortLinkKey, metadata.short_link.spec());
   dict.SetString(kMimeTypeKey, metadata.mime_type);
   dict.SetString(kDarkMimeTypeKey, metadata.dark_mime_type);

components/search_provider_logos/logo_cache_unittest.cc

@@ -38,7 +38,9 @@ LogoMetadata GetExampleMetadata() {
   metadata.dark_mime_type = "image/jpeg";
   metadata.dark_background_color = "#ABC123";
   metadata.log_url = GURL("https://www.google.com/ddllog?a=b");
+  metadata.dark_log_url = GURL("https://www.google.com/ddllog?a=dark");
   metadata.cta_log_url = GURL("https://www.google.com/ddllog?c=d");
+  metadata.dark_cta_log_url = GURL("https://www.google.com/ddllog?c=dark");
   metadata.share_button_x = 200;
   metadata.share_button_y = 100;
   metadata.share_button_opacity = 0.5;
@@ -119,7 +121,10 @@ void ExpectMetadataEqual(const LogoMetadata& expected_metadata,
   EXPECT_EQ(expected_metadata.alt_text, actual_metadata.alt_text);
   EXPECT_EQ(expected_metadata.mime_type, actual_metadata.mime_type);
   EXPECT_EQ(expected_metadata.log_url, actual_metadata.log_url);
+  EXPECT_EQ(expected_metadata.dark_log_url, actual_metadata.dark_log_url);
   EXPECT_EQ(expected_metadata.cta_log_url, actual_metadata.cta_log_url);
+  EXPECT_EQ(expected_metadata.dark_cta_log_url,
+            actual_metadata.dark_cta_log_url);
   EXPECT_EQ(expected_metadata.short_link, actual_metadata.short_link);
   EXPECT_EQ(expected_metadata.share_button_x, actual_metadata.share_button_x);
   EXPECT_EQ(expected_metadata.share_button_y, actual_metadata.share_button_y);

components/search_provider_logos/logo_common.h

@@ -79,9 +79,11 @@ struct LogoMetadata {
 
   // The URL to ping when the CTA image is clicked. May be empty.
   GURL cta_log_url;
+  GURL dark_cta_log_url;
   // The URL to ping when the main image is clicked (i.e. the animated image if
   // there is one, or the only image otherwise). May be empty.
   GURL log_url;
+  GURL dark_log_url;
 
   // The URL used for sharing doodles.
   GURL short_link;

components/search_provider_logos/logo_service_impl.cc

@@ -23,7 +23,6 @@
 #include "components/image_fetcher/core/image_decoder.h"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url_service.h"
-#include "components/search_provider_logos/features.h"
 #include "components/search_provider_logos/fixed_logo_api.h"
 #include "components/search_provider_logos/google_logo_api.h"
 #include "components/search_provider_logos/logo_cache.h"

components/search_provider_logos/logo_service_impl_unittest.cc

@@ -32,7 +32,6 @@
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_data.h"
 #include "components/search_engines/template_url_service.h"
-#include "components/search_provider_logos/features.h"
 #include "components/search_provider_logos/fixed_logo_api.h"
 #include "components/search_provider_logos/google_logo_api.h"
 #include "components/search_provider_logos/logo_cache.h"