From 20fca53466d48c3186c80e19e3003d818e411453 Mon Sep 17 00:00:00 2001 From: Paul Adedeji Date: Thu, 26 Jan 2023 01:15:48 +0000 Subject: [PATCH] [ntp] Store collection ids for theme background images This CL concerns the NTP handler. A follow up CL will bring the changes here to the Customize Chrome handler. Prior to this CL collection ids were only stored in user prefs ('prefs::kNtpCustomBackgroundDict') if daily refresh was turned on. Now, collection IDs will be stored anytime a theme is set. This CL obsoletes a test in ntp_custom_background_service_unittest.cc that makes sure collection ID takes priority over background URL. Before and after this CL, there are no calls to SetCustomBackground for daily refresh in the codebase where both URL and collection id are set (https://source.chromium.org/search?q=-file:out%20SetCustomBackgroundInfo%20-file:test). The changes in the CL will allow us to create a metric that stores the collection ID used by the user on NTP load. Change-Id: Ief491945ec77b6b06f56dc4ff42bad2774eb9ca4 Bug: 1384258 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4183244 Reviewed-by: Tibor Goldschwendt Reviewed-by: Tom Sepez Commit-Queue: Paul Adedeji Reviewed-by: Riley Tatum Cr-Commit-Position: refs/heads/main@{#1097166} --- .../new_tab_page/customize_backgrounds.ts | 9 +++-- .../new_tab_page/customize_dialog.ts | 4 +- .../search/background/ntp_background_data.h | 5 ++- .../ntp_custom_background_service.cc | 9 ++++- .../ntp_custom_background_service_unittest.cc | 29 ++------------- .../ui/webui/new_tab_page/new_tab_page.mojom | 10 +++-- .../new_tab_page/new_tab_page_handler.cc | 15 ++++---- .../webui/new_tab_page/new_tab_page_handler.h | 3 +- .../new_tab_page_handler_unittest.cc | 37 ++++++++++++++++++- .../customize_backgrounds_test.ts | 9 ++++- .../new_tab_page/customize_dialog_test.ts | 3 +- .../data/webui/new_tab_page/test_support.ts | 3 +- 12 files changed, 86 insertions(+), 50 deletions(-) diff --git a/chrome/browser/resources/new_tab_page/customize_backgrounds.ts b/chrome/browser/resources/new_tab_page/customize_backgrounds.ts index bfd87033bcb64b..f8c5f048ab5c4d 100644 --- a/chrome/browser/resources/new_tab_page/customize_backgrounds.ts +++ b/chrome/browser/resources/new_tab_page/customize_backgrounds.ts @@ -96,8 +96,7 @@ export class CustomizeBackgroundsElement extends PolymerElement { private getNoBackgroundClass_(): string { return this.theme && (this.theme.backgroundImage && !this.theme.isCustomBackground || - !this.theme.backgroundImage && - !this.theme.dailyRefreshCollectionId) ? + !this.theme.backgroundImage && !this.theme.dailyRefreshEnabled) ? 'selected' : ''; } @@ -106,7 +105,7 @@ export class CustomizeBackgroundsElement extends PolymerElement { const {url} = this.images_[index].imageUrl; return this.theme && this.theme.backgroundImage && this.theme.backgroundImage.url.url === url && - !this.theme.dailyRefreshCollectionId ? + !this.theme.dailyRefreshEnabled ? 'selected' : ''; } @@ -149,9 +148,11 @@ export class CustomizeBackgroundsElement extends PolymerElement { attributionUrl, imageUrl, previewImageUrl, + collectionId, } = image; this.pageHandler_.setBackgroundImage( - attribution1, attribution2, attributionUrl, imageUrl, previewImageUrl); + attribution1, attribution2, attributionUrl, imageUrl, previewImageUrl, + collectionId); } private async onSelectedCollectionChange_() { diff --git a/chrome/browser/resources/new_tab_page/customize_dialog.ts b/chrome/browser/resources/new_tab_page/customize_dialog.ts index 9393992a1b0174..2fc8deaf56ce17 100644 --- a/chrome/browser/resources/new_tab_page/customize_dialog.ts +++ b/chrome/browser/resources/new_tab_page/customize_dialog.ts @@ -167,8 +167,8 @@ export class CustomizeDialogElement extends PolymerElement { if (!this.selectedCollection_) { return false; } - return !!this.theme && - this.selectedCollection_.id === this.theme.dailyRefreshCollectionId; + return !!this.theme && this.theme.dailyRefreshEnabled && + this.selectedCollection_!.id === this.theme.backgroundImageCollectionId; } private computeShowTitleNavigation_(): boolean { diff --git a/chrome/browser/search/background/ntp_background_data.h b/chrome/browser/search/background/ntp_background_data.h index 7d72899ee83f1b..51e6891e1a0ed6 100644 --- a/chrome/browser/search/background/ntp_background_data.h +++ b/chrome/browser/search/background/ntp_background_data.h @@ -135,11 +135,14 @@ struct CustomBackground { // Url to learn more info about the custom background. GURL custom_background_attribution_action_url; - // Id of the collection being used for "daily refresh". + // Id of the collection being used. std::string collection_id; // Main color of the image. absl::optional custom_background_main_color; + + // Whether daily refresh is enabled. + bool daily_refresh_enabled; }; #endif // CHROME_BROWSER_SEARCH_BACKGROUND_NTP_BACKGROUND_DATA_H_ diff --git a/chrome/browser/search/background/ntp_custom_background_service.cc b/chrome/browser/search/background/ntp_custom_background_service.cc index 77a575d85683b7..b518bae2ccd189 100644 --- a/chrome/browser/search/background/ntp_custom_background_service.cc +++ b/chrome/browser/search/background/ntp_custom_background_service.cc @@ -339,7 +339,8 @@ void NtpCustomBackgroundService::SetCustomBackgroundInfo( background_updated_timestamp_ = base::TimeTicks::Now(); - if (!collection_id.empty() && is_backdrop_collection) { + if (!background_url.is_valid() && !collection_id.empty() && + is_backdrop_collection) { background_service_->FetchNextCollectionImage(collection_id, absl::nullopt); } else if (background_url.is_valid() && is_backdrop_url) { if (base::FeatureList::IsEnabled( @@ -350,7 +351,7 @@ void NtpCustomBackgroundService::SetCustomBackgroundInfo( } base::Value::Dict background_info = GetBackgroundInfoAsDict( background_url, attribution_line_1, attribution_line_2, action_url, - absl::nullopt, absl::nullopt, absl::nullopt); + collection_id, absl::nullopt, absl::nullopt); pref_service_->SetDict(prefs::kNtpCustomBackgroundDict, std::move(background_info)); } else { @@ -436,6 +437,8 @@ NtpCustomBackgroundService::GetCustomBackground() { // Set custom background information in theme info (attributions are // optional). + const base::Value* daily_refresh_timestamp = + background_info.Find(kNtpCustomBackgroundRefreshTimestamp); const base::Value* attribution_line_1 = background_info.Find(kNtpCustomBackgroundAttributionLine1); const base::Value* attribution_line_2 = @@ -450,6 +453,8 @@ NtpCustomBackgroundService::GetCustomBackground() { custom_background->custom_background_url = custom_background_url; custom_background->is_uploaded_image = false; custom_background->collection_id = collection_id; + custom_background->daily_refresh_enabled = + daily_refresh_timestamp && daily_refresh_timestamp->GetInt() != 0; std::string custom_background_url_spec = custom_background_url.spec(); size_t image_options_index = custom_background_url_spec.find("="); if (image_options_index != std::string::npos) { diff --git a/chrome/browser/search/background/ntp_custom_background_service_unittest.cc b/chrome/browser/search/background/ntp_custom_background_service_unittest.cc index c57f7a552dc9aa..23bc95d0c1e7cc 100644 --- a/chrome/browser/search/background/ntp_custom_background_service_unittest.cc +++ b/chrome/browser/search/background/ntp_custom_background_service_unittest.cc @@ -101,7 +101,8 @@ TEST_F(NtpCustomBackgroundServiceTest, SetCustomBackgroundURL) { auto custom_background = custom_background_service_->GetCustomBackground(); EXPECT_EQ(kUrl, custom_background->custom_background_url); - EXPECT_EQ(false, custom_background->is_uploaded_image); + EXPECT_FALSE(custom_background->is_uploaded_image); + EXPECT_FALSE(custom_background->daily_refresh_enabled); EXPECT_TRUE(custom_background_service_->IsCustomBackgroundSet()); } @@ -393,6 +394,7 @@ TEST_F(NtpCustomBackgroundServiceTest, SetCustomBackgroundCollectionId) { auto custom_background = custom_background_service_->GetCustomBackground(); EXPECT_EQ(kValidId, custom_background->collection_id); + EXPECT_TRUE(custom_background->daily_refresh_enabled); EXPECT_TRUE(custom_background_service_->IsCustomBackgroundSet()); // An invalid id should clear the pref/background. @@ -407,30 +409,6 @@ TEST_F(NtpCustomBackgroundServiceTest, SetCustomBackgroundCollectionId) { EXPECT_FALSE(custom_background_service_->IsCustomBackgroundSet()); } -TEST_F(NtpCustomBackgroundServiceTest, - CollectionIdTakePriorityOverBackgroundURL) { - EXPECT_CALL(observer_, OnCustomBackgroundImageUpdated).Times(1); - ASSERT_FALSE(custom_background_service_->IsCustomBackgroundSet()); - const std::string kValidId("art"); - const GURL kUrl("https://www.foo.com/"); - - CollectionImage image; - image.collection_id = kValidId; - image.image_url = GURL("https://www.test.com/"); - custom_background_service_->SetNextCollectionImageForTesting(image); - custom_background_service_->AddValidBackdropUrlForTesting(kUrl); - custom_background_service_->AddValidBackdropCollectionForTesting(kValidId); - - custom_background_service_->SetCustomBackgroundInfo(kUrl, GURL(), "", "", - GURL(), kValidId); - task_environment_.RunUntilIdle(); - - auto custom_background = custom_background_service_->GetCustomBackground(); - EXPECT_EQ(kValidId, custom_background->collection_id); - EXPECT_EQ("https://www.test.com/", custom_background->custom_background_url); - EXPECT_TRUE(custom_background_service_->IsCustomBackgroundSet()); -} - TEST_F(NtpCustomBackgroundServiceTest, RefreshesBackgroundAfter24Hours) { EXPECT_CALL(observer_, OnCustomBackgroundImageUpdated).Times(2); ASSERT_FALSE(custom_background_service_->IsCustomBackgroundSet()); @@ -477,6 +455,7 @@ TEST_F(NtpCustomBackgroundServiceTest, RefreshesBackgroundAfter24Hours) { custom_background = custom_background_service_->GetCustomBackground(); EXPECT_EQ(kValidId, custom_background->collection_id); EXPECT_EQ(kImageUrl2, custom_background->custom_background_url); + EXPECT_TRUE(custom_background->daily_refresh_enabled); EXPECT_TRUE(custom_background_service_->IsCustomBackgroundSet()); } diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom b/chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom index 97523d3bad4ae6..64c2677c8f118e 100644 --- a/chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom +++ b/chrome/browser/ui/webui/new_tab_page/new_tab_page.mojom @@ -29,6 +29,8 @@ struct CollectionImage { url.mojom.Url image_url; // URL to a preview of the image. Can point to untrusted content. url.mojom.Url preview_image_url; + // Collection id of the image; + string collection_id; }; // The background image URL and styling. @@ -55,14 +57,16 @@ struct Theme { skia.mojom.SkColor background_color; // True if the background is custom. bool is_custom_background; + // True if daily refresh is enabled. + bool daily_refresh_enabled; // True if the theme is dark (e.g. NTP background color is dark). bool is_dark; // True if the realbox icons should be themed based on the background color. bool theme_realbox_icons; // Color of Google logo. If not set show the logo multi-colored. skia.mojom.SkColor? logo_color; - // Selected collection for daily refresh. - string? daily_refresh_collection_id; + // Collection id of the background image. + string? background_image_collection_id; // The background image. BackgroundImage? background_image; // Human readable attributions of the background image. @@ -234,7 +238,7 @@ interface PageHandler { // Sets the background image and notifies all NTPs of the change. SetBackgroundImage(string attribution_1, string attribution_2, url.mojom.Url attribution_url, url.mojom.Url image_url, - url.mojom.Url thumbnail_url); + url.mojom.Url thumbnail_url, string collection_id); // Sets collection id for daily refresh. When |collection_id| is empty, the // daily refresh is turned off. SetDailyRefreshCollectionId(string collection_id); diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc index 30318fe3088655..4323091088b21d 100644 --- a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc +++ b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.cc @@ -302,7 +302,8 @@ new_tab_page::mojom::ThemePtr MakeTheme( custom_background->custom_background_attribution_line_2; theme->background_image_attribution_url = custom_background->custom_background_attribution_action_url; - theme->daily_refresh_collection_id = custom_background->collection_id; + theme->background_image_collection_id = custom_background->collection_id; + theme->daily_refresh_enabled = custom_background->daily_refresh_enabled; } theme->most_visited = std::move(most_visited); @@ -546,19 +547,18 @@ void NewTabPageHandler::SetBackgroundImage(const std::string& attribution_1, const std::string& attribution_2, const GURL& attribution_url, const GURL& image_url, - const GURL& thumbnail_url) { - // Populating the |collection_id| turns on refresh daily which overrides the - // the selected image. + const GURL& thumbnail_url, + const std::string& collection_id) { ntp_custom_background_service_->SetCustomBackgroundInfo( image_url, thumbnail_url, attribution_1, attribution_2, attribution_url, - /* collection_id= */ ""); + collection_id); LogEvent(NTP_BACKGROUND_IMAGE_SET); } void NewTabPageHandler::SetDailyRefreshCollectionId( const std::string& collection_id) { - // Populating the |collection_id| turns on refresh daily which overrides the - // the selected image. + // Only populating the |collection_id| turns on refresh daily which overrides + // the the selected image. ntp_custom_background_service_->SetCustomBackgroundInfo( /* image_url */ GURL(), /* thumbnail_url */ GURL(), /* attribution_line_1= */ "", /* attribution_line_2= */ "", @@ -1138,6 +1138,7 @@ void NewTabPageHandler::OnCollectionImagesAvailable() { image->attribution_url = info.attribution_action_url; image->image_url = info.image_url; image->preview_image_url = info.thumbnail_image_url; + image->collection_id = collection_id; images.push_back(std::move(image)); } std::move(background_images_callback_).Run(std::move(images)); diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h index 2e3b12b6351e4d..13692819897318 100644 --- a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h +++ b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler.h @@ -91,7 +91,8 @@ class NewTabPageHandler : public new_tab_page::mojom::PageHandler, const std::string& attribution_2, const GURL& attribution_url, const GURL& image_url, - const GURL& thumbnail_url) override; + const GURL& thumbnail_ur, + const std::string& collection_id) override; void SetDailyRefreshCollectionId(const std::string& collection_id) override; void SetNoBackgroundImage() override; void RevertBackgroundChanges() override; diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc index d1124049816e4c..91b1edb6d565cd 100644 --- a/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc +++ b/chrome/browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc @@ -420,7 +420,7 @@ TEST_P(NewTabPageHandlerThemeTest, SetTheme) { EXPECT_EQ(SkColorSetRGB(0, 0, 1), theme->background_color); EXPECT_FALSE(theme->is_custom_background); EXPECT_FALSE(theme->is_dark); - EXPECT_FALSE(theme->daily_refresh_collection_id.has_value()); + EXPECT_FALSE(theme->daily_refresh_enabled); ASSERT_TRUE(theme->background_image); EXPECT_EQ("chrome-untrusted://theme/IDR_THEME_NTP_BACKGROUND?bar", theme->background_image->url); @@ -447,6 +447,7 @@ TEST_P(NewTabPageHandlerThemeTest, SetTheme) { EXPECT_FALSE(theme->background_image_attribution_1.has_value()); EXPECT_FALSE(theme->background_image_attribution_2.has_value()); EXPECT_FALSE(theme->background_image_attribution_url.has_value()); + EXPECT_FALSE(theme->background_image_collection_id.has_value()); ASSERT_TRUE(theme->most_visited); EXPECT_EQ(SkColorSetRGB(0, 0, 8), theme->most_visited->background_color); if (RemoveScrim()) { @@ -497,6 +498,7 @@ TEST_P(NewTabPageHandlerThemeTest, SetCustomBackground) { EXPECT_FALSE(theme->background_image_attribution_1.has_value()); EXPECT_FALSE(theme->background_image_attribution_2.has_value()); EXPECT_FALSE(theme->background_image_attribution_url.has_value()); + EXPECT_FALSE(theme->background_image_collection_id.has_value()); } else { ASSERT_TRUE(theme); EXPECT_TRUE(theme->is_custom_background); @@ -508,7 +510,8 @@ TEST_P(NewTabPageHandlerThemeTest, SetCustomBackground) { EXPECT_EQ("bar line", theme->background_image_attribution_2); EXPECT_EQ("https://foo.com/action", theme->background_image_attribution_url); - EXPECT_EQ("baz collection", theme->daily_refresh_collection_id); + EXPECT_FALSE(theme->daily_refresh_enabled); + EXPECT_EQ("baz collection", theme->background_image_collection_id); } if (RemoveScrim()) { EXPECT_TRUE(theme->background_image->scrim_display.has_value()); @@ -524,6 +527,36 @@ TEST_P(NewTabPageHandlerThemeTest, SetCustomBackground) { } } +TEST_P(NewTabPageHandlerThemeTest, SetDailyRefresh) { + new_tab_page::mojom::ThemePtr theme; + EXPECT_CALL(mock_page_, SetTheme) + .Times(1) + .WillOnce(testing::Invoke([&theme](new_tab_page::mojom::ThemePtr arg) { + theme = std::move(arg); + })); + CustomBackground custom_background; + custom_background.daily_refresh_enabled = true; + custom_background.collection_id = "baz collection"; + ON_CALL(mock_ntp_custom_background_service_, GetCustomBackground()) + .WillByDefault(testing::Return(absl::make_optional(custom_background))); + ON_CALL(mock_theme_provider_, HasCustomImage(IDR_THEME_NTP_BACKGROUND)) + .WillByDefault(testing::Return(true)); + + ntp_custom_background_service_observer_->OnCustomBackgroundImageUpdated(); + mock_page_.FlushForTesting(); + + ASSERT_TRUE(theme); + if (CustomizeChromeSidePanel()) { + EXPECT_FALSE(theme->is_custom_background); + EXPECT_FALSE(theme->background_image_collection_id.has_value()); + } else { + ASSERT_TRUE(theme); + EXPECT_TRUE(theme->is_custom_background); + EXPECT_TRUE(theme->daily_refresh_enabled); + EXPECT_EQ("baz collection", theme->background_image_collection_id); + } +} + INSTANTIATE_TEST_SUITE_P(All, NewTabPageHandlerThemeTest, ::testing::Combine(::testing::Bool(), diff --git a/chrome/test/data/webui/new_tab_page/customize_backgrounds_test.ts b/chrome/test/data/webui/new_tab_page/customize_backgrounds_test.ts index 1f3fe21128eec4..fd7a2a59d0df39 100644 --- a/chrome/test/data/webui/new_tab_page/customize_backgrounds_test.ts +++ b/chrome/test/data/webui/new_tab_page/customize_backgrounds_test.ts @@ -113,6 +113,7 @@ suite('NewTabPageCustomizeBackgroundsTest', () => { previewImageUrl: {url: 'https://a.com/p.png'}, attributionUrl: {url: ''}, attribution2: '', + collectionId: '', }; handler.setResultFor('getBackgroundImages', Promise.resolve({ images: [image], @@ -144,6 +145,7 @@ suite('NewTabPageCustomizeBackgroundsTest', () => { previewImageUrl: {url: 'https://example.com/image.png'}, attributionUrl: {url: ''}, attribution2: '', + collectionId: '', }; const customizeBackgrounds = await createCustomizeBackgrounds(); handler.setResultFor('getBackgroundImages', Promise.resolve({ @@ -168,6 +170,7 @@ suite('NewTabPageCustomizeBackgroundsTest', () => { previewImageUrl: {url: 'https://example.com/image.png'}, attributionUrl: {url: ''}, attribution2: '', + collectionId: '', }; const customizeBackgrounds = await createCustomizeBackgrounds(); handler.setResultFor('getBackgroundImages', Promise.resolve({ @@ -195,6 +198,7 @@ suite('NewTabPageCustomizeBackgroundsTest', () => { previewImageUrl: {url: 'https://example.com/image.png'}, attributionUrl: {url: ''}, attribution2: '', + collectionId: '', }; const customizeBackgrounds = await createCustomizeBackgrounds(); handler.setResultFor('getBackgroundImages', Promise.resolve({ @@ -219,6 +223,7 @@ suite('NewTabPageCustomizeBackgroundsTest', () => { previewImageUrl: {url: 'https://example.com/image.png'}, attributionUrl: {url: ''}, attribution2: '', + collectionId: '', }; const customizeBackgrounds = await createCustomizeBackgrounds(); customizeBackgrounds.theme.backgroundImage = @@ -240,6 +245,7 @@ suite('NewTabPageCustomizeBackgroundsTest', () => { previewImageUrl: {url: 'https://example.com/image.png'}, attributionUrl: {url: ''}, attribution2: '', + collectionId: '', }; const customizeBackgrounds = await createCustomizeBackgrounds(); handler.setResultFor('getBackgroundImages', Promise.resolve({ @@ -292,7 +298,8 @@ suite('NewTabPageCustomizeBackgroundsTest', () => { test('not selected when refresh collection set', () => { const theme = createTheme(); - theme.dailyRefreshCollectionId = 'landscape'; + theme.dailyRefreshEnabled = true; + theme.backgroundImageCollectionId = 'landscape'; customizeBackgrounds.theme = theme; assertSetNoBackgroundImageNotCalled(); }); diff --git a/chrome/test/data/webui/new_tab_page/customize_dialog_test.ts b/chrome/test/data/webui/new_tab_page/customize_dialog_test.ts index 0b1797d30bc4bd..27830d4ab817a7 100644 --- a/chrome/test/data/webui/new_tab_page/customize_dialog_test.ts +++ b/chrome/test/data/webui/new_tab_page/customize_dialog_test.ts @@ -130,7 +130,8 @@ suite('NewTabPageCustomizeDialogTest', () => { suite('backgrounds', () => { setup(() => { const theme = createTheme(); - theme.dailyRefreshCollectionId = 'landscape'; + theme.dailyRefreshEnabled = true; + theme.backgroundImageCollectionId = 'landscape'; theme.backgroundImage = createBackgroundImage('https://example.com/image.png'); customizeDialog.theme = theme; diff --git a/chrome/test/data/webui/new_tab_page/test_support.ts b/chrome/test/data/webui/new_tab_page/test_support.ts index ac021dbb620677..f9aaef9ffc00cc 100644 --- a/chrome/test/data/webui/new_tab_page/test_support.ts +++ b/chrome/test/data/webui/new_tab_page/test_support.ts @@ -70,7 +70,8 @@ export function createTheme(isDark: boolean = false): Theme { backgroundColor: {value: 0xffff0000}, backgroundImageAttribution1: '', backgroundImageAttribution2: '', - dailyRefreshCollectionId: '', + dailyRefreshEnabled: false, + backgroundImageCollectionId: '', isDark, mostVisited: mostVisited, textColor: {value: 0xff0000ff},