[NTP] Check timestamp for custom bg color calculation

The color calculated for a custom background should only
be updated if the image hasn't changed in the meantime.
Check this by saving the timestamp when a background is set
and checking that it still matches once the color calculation
is complete.

When possible use the thumbnail url |fetch_image| for the calculation,
the full-sized image was always being used.

Fix a copy-paste error in the definition for onlocalbackgroundselected.

Bug: 982215
Change-Id: Ib02f000add27f10eb23fb50051b44744a4b53323
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1693389Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Commit-Queue: Kyle Milka <kmilka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676069}

chrome/browser/search/instant_service.cc

@@ -190,6 +190,7 @@ InstantService::InstantService(Profile* profile)
       pref_service_(profile_->GetPrefs()),
       theme_observer_(this),
       native_theme_(ui::NativeTheme::GetInstanceForNativeUi()),
+      background_updated_timestamp_(base::TimeTicks::Now()),
       weak_ptr_factory_(this) {
   // The initialization below depends on a typical set of browser threads. Skip
   // it if we are running in a unit test without the full suite.
@@ -441,12 +442,14 @@ void InstantService::SetCustomBackgroundURLWithAttributions(
   pref_service_->SetBoolean(prefs::kNtpCustomBackgroundLocalToDevice, false);
   RemoveLocalBackgroundImageCopy();
 
+  background_updated_timestamp_ = base::TimeTicks::Now();
+
   if (background_url.is_valid() && is_backdrop_url) {
     const GURL& thumbnail_url =
         background_service_->GetThumbnailUrl(background_url);
-    FetchCustomBackground(background_url, thumbnail_url.is_valid()
-                                              ? thumbnail_url
-                                              : background_url);
+    FetchCustomBackground(
+        background_updated_timestamp_,
+        thumbnail_url.is_valid() ? thumbnail_url : background_url);
 
     base::DictionaryValue background_info = GetBackgroundInfoAsDict(
         background_url, attribution_line_1, attribution_line_2, action_url);
@@ -464,6 +467,7 @@ void InstantService::SetCustomBackgroundURLWithAttributions(
 }
 
 void InstantService::SetBackgroundToLocalResource() {
+  background_updated_timestamp_ = base::TimeTicks::Now();
   pref_service_->SetBoolean(prefs::kNtpCustomBackgroundLocalToDevice, true);
   UpdateThemeInfo();
 }
@@ -796,7 +800,7 @@ void InstantService::ResetToDefault() {
 }
 
 void InstantService::UpdateCustomBackgroundColorAsync(
-    const GURL& image_url,
+    base::TimeTicks timestamp,
     const gfx::Image& fetched_image,
     const image_fetcher::RequestMetadata& metadata) {
   // Calculate the bitmap color asynchronously as it is slow (1-2 seconds for
@@ -806,11 +810,11 @@ void InstantService::UpdateCustomBackgroundColorAsync(
         FROM_HERE, {base::TaskPriority::BEST_EFFORT},
         base::BindOnce(&GetBitmapMainColor, *fetched_image.ToSkBitmap()),
         base::BindOnce(&InstantService::UpdateCustomBackgroundPrefsWithColor,
-                       weak_ptr_factory_.GetWeakPtr(), image_url));
+                       weak_ptr_factory_.GetWeakPtr(), timestamp));
   }
 }
 
-void InstantService::FetchCustomBackground(const GURL& image_url,
+void InstantService::FetchCustomBackground(base::TimeTicks timestamp,
                                            const GURL& fetch_url) {
   DCHECK(!fetch_url.is_empty());
 
@@ -837,9 +841,9 @@ void InstantService::FetchCustomBackground(const GURL& image_url,
   image_fetcher::ImageFetcherParams params(traffic_annotation,
                                            kCustomBackgroundsUmaClientName);
   image_fetcher_->FetchImage(
-      image_url,
+      fetch_url,
       base::BindOnce(&InstantService::UpdateCustomBackgroundColorAsync,
-                     weak_ptr_factory_.GetWeakPtr(), image_url),
+                     weak_ptr_factory_.GetWeakPtr(), timestamp),
       std::move(params));
 }
 
@@ -881,8 +885,9 @@ void InstantService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(prefs::kNtpShortcutsVisible, true);
 }
 
-void InstantService::UpdateCustomBackgroundPrefsWithColor(const GURL& image_url,
-                                                          SkColor color) {
+void InstantService::UpdateCustomBackgroundPrefsWithColor(
+    base::TimeTicks timestamp,
+    SkColor color) {
   // Update background color only if the selected background is still the same.
   const base::DictionaryValue* background_info =
       pref_service_->GetDictionary(prefs::kNtpCustomBackgroundDict);
@@ -891,7 +896,7 @@ void InstantService::UpdateCustomBackgroundPrefsWithColor(const GURL& image_url,
 
   GURL current_bg_url(
       background_info->FindKey(kNtpCustomBackgroundURL)->GetString());
-  if (current_bg_url == image_url) {
+  if (timestamp == background_updated_timestamp_) {
     pref_service_->Set(prefs::kNtpCustomBackgroundDict,
                        GetBackgroundInfoWithColor(background_info, color));
   }

chrome/browser/search/instant_service.h

@@ -162,12 +162,12 @@ class InstantService : public KeyedService,
 
   // Calculates the most frequent color of the image and stores it in prefs.
   void UpdateCustomBackgroundColorAsync(
-      const GURL& image_url,
+      base::TimeTicks timestamp,
       const gfx::Image& fetched_image,
       const image_fetcher::RequestMetadata& metadata);
 
   // Fetches the image for the given |fetch_url|.
-  void FetchCustomBackground(const GURL& image_url, const GURL& fetch_url);
+  void FetchCustomBackground(base::TimeTicks timestamp, const GURL& fetch_url);
 
  private:
   class SearchProviderObserver;
@@ -184,6 +184,9 @@ class InstantService : public KeyedService,
   FRIEND_TEST_ALL_PREFIXES(InstantServiceTest, DoesToggleShortcutsVisibility);
   FRIEND_TEST_ALL_PREFIXES(InstantServiceTest, IsCustomLinksEnabled);
   FRIEND_TEST_ALL_PREFIXES(InstantServiceTest, TestNoThemeInfo);
+  FRIEND_TEST_ALL_PREFIXES(InstantServiceTest, TestUpdateCustomBackgroundColor);
+  FRIEND_TEST_ALL_PREFIXES(InstantServiceTest,
+                           LocalImageDoesNotUpdateCustomBackgroundColor);
 
   // KeyedService:
   void Shutdown() override;
@@ -237,12 +240,17 @@ class InstantService : public KeyedService,
   // chrome-search://local-ntp/background.jpg
   void SetBackgroundToLocalResource();
 
-  // Updates custom background prefs with color for the given |image_url|.
-  void UpdateCustomBackgroundPrefsWithColor(const GURL& image_url,
+  // Updates custom background prefs with color if the background hasn't changed
+  // since the calculation started.
+  void UpdateCustomBackgroundPrefsWithColor(base::TimeTicks timestamp,
                                             SkColor color);
 
   void SetImageFetcherForTesting(image_fetcher::ImageFetcher* image_fetcher);
 
+  base::TimeTicks GetBackgroundUpdatedTimestampForTesting() {
+    return background_updated_timestamp_;
+  }
+
   Profile* const profile_;
 
   // The process ids associated with Instant processes.
@@ -279,6 +287,8 @@ class InstantService : public KeyedService,
 
   std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
 
+  base::TimeTicks background_updated_timestamp_;
+
   base::WeakPtrFactory<InstantService> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(InstantService);

chrome/browser/search/instant_service_unittest.cc

@@ -581,7 +581,7 @@ TEST_F(InstantServiceTest, TestUpdateCustomBackgroundColor) {
 
   // Background color will not update if no background is set.
   instant_service_->UpdateCustomBackgroundColorAsync(
-      GURL(), image, image_fetcher::RequestMetadata());
+      base::TimeTicks::Now(), image, image_fetcher::RequestMetadata());
   thread_bundle()->RunUntilIdle();
   EXPECT_FALSE(CheckBackgroundColor(
       SK_ColorRED,
@@ -597,9 +597,9 @@ TEST_F(InstantServiceTest, TestUpdateCustomBackgroundColor) {
   instant_service_->SetCustomBackgroundURLWithAttributions(
       kUrl, kAttributionLine1, kAttributionLine2, kActionUrl);
 
-  // Background color will not update if current background url changed.
+  // Background color will not update if background timestamp has changed.
   instant_service_->UpdateCustomBackgroundColorAsync(
-      GURL("different_url"), image, image_fetcher::RequestMetadata());
+      base::TimeTicks::Now(), image, image_fetcher::RequestMetadata());
   thread_bundle()->RunUntilIdle();
   EXPECT_FALSE(CheckBackgroundColor(
       SK_ColorRED,
@@ -607,9 +607,52 @@ TEST_F(InstantServiceTest, TestUpdateCustomBackgroundColor) {
 
   // Background color should update.
   instant_service_->UpdateCustomBackgroundColorAsync(
-      kUrl, image, image_fetcher::RequestMetadata());
+      instant_service_->GetBackgroundUpdatedTimestampForTesting(), image,
+      image_fetcher::RequestMetadata());
   thread_bundle()->RunUntilIdle();
   EXPECT_TRUE(CheckBackgroundColor(
       SK_ColorRED,
       pref_service->GetDictionary(prefs::kNtpCustomBackgroundDict)));
 }
+
+TEST_F(InstantServiceTest, LocalImageDoesNotUpdateCustomBackgroundColor) {
+  SkBitmap bitmap;
+  bitmap.allocN32Pixels(32, 32);
+  bitmap.eraseColor(SK_ColorRED);
+  gfx::Image image = gfx::Image::CreateFrom1xBitmap(bitmap);
+  sync_preferences::TestingPrefServiceSyncable* pref_service =
+      profile()->GetTestingPrefService();
+
+  base::FilePath profile_path = profile()->GetPath();
+  base::FilePath path(profile_path.AppendASCII("test_file"));
+  base::FilePath copy_path(profile_path.AppendASCII(
+      chrome::kChromeSearchLocalNtpBackgroundFilename));
+  base::WriteFile(path, "background_image", 16);
+
+  instant_service_->SelectLocalBackgroundImage(path);
+
+  ASSERT_FALSE(instant_service_->IsCustomBackgroundSet());
+
+  const GURL kUrl("https://www.foo.com");
+  const std::string kAttributionLine1 = "foo";
+  const std::string kAttributionLine2 = "bar";
+  const GURL kActionUrl("https://www.bar.com");
+
+  SetUserSelectedDefaultSearchProvider("{google:baseURL}");
+  instant_service_->AddValidBackdropUrlForTesting(kUrl);
+  instant_service_->SetCustomBackgroundURLWithAttributions(
+      kUrl, kAttributionLine1, kAttributionLine2, kActionUrl);
+  base::TimeTicks time_set =
+      instant_service_->GetBackgroundUpdatedTimestampForTesting();
+
+  instant_service_->SelectLocalBackgroundImage(path);
+
+  // Background color will not update if a local image was uploaded in the
+  // meantime.
+  instant_service_->UpdateCustomBackgroundColorAsync(
+      time_set, image, image_fetcher::RequestMetadata());
+  thread_bundle()->RunUntilIdle();
+  EXPECT_FALSE(CheckBackgroundColor(
+      SK_ColorRED,
+      pref_service->GetDictionary(prefs::kNtpCustomBackgroundDict)));
+}

chrome/renderer/searchbox/searchbox_extension.cc

@@ -539,8 +539,9 @@ static const char kDispatchLocalBackgroundSelectedScript[] =
     "if (window.chrome &&"
     "    window.chrome.embeddedSearch &&"
     "    window.chrome.embeddedSearch.newTabPage &&"
-    "    window.chrome.embeddedSearch.newTabPage.onthemechange &&"
-    "    typeof window.chrome.embeddedSearch.newTabPage.onthemechange =="
+    "    window.chrome.embeddedSearch.newTabPage.onlocalbackgroundselected &&"
+    "    typeof "
+    "window.chrome.embeddedSearch.newTabPage.onlocalbackgroundselected =="
     "        'function') {"
     "  "
     "window.chrome.embeddedSearch.newTabPage."