Allow background color to dynamically change download shelf for PWAs

The current logic use background color for tab color in a tabbed PWA.
So the current logic ignores any dynamic changes to bg for non-tabbed
apps.

But bg color also sets other things such as download shelf color,
so we should still update themes for non-tabbed apps when bg color
changes.

Moved color change detection logic for theme_color / background_color,
and UserChangedTheme notifications into UpdateThemePack to simplify
its callsites.

Bug: 1134112
Change-Id: Ibe68911b733b0eb7760cd2d2f4be96e3eb2c7a3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2451989Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Auto-Submit: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814569}

chrome/browser/ui/web_applications/app_browser_controller.cc

@@ -398,27 +398,15 @@ void AppBrowserController::DidStartNavigation(
 void AppBrowserController::DOMContentLoaded(
     content::RenderFrameHost* render_frame_host) {
   // We hold off changing theme color for a new tab until the page is loaded.
-  DidChangeThemeColor();
+  UpdateThemePack();
 }
 
 void AppBrowserController::DidChangeThemeColor() {
-  base::Optional<SkColor> theme_color = GetThemeColor();
-  if (theme_color == last_theme_color_)
-    return;
-  last_theme_color_ = theme_color;
   UpdateThemePack();
-  browser_->window()->UserChangedTheme(BrowserThemeChangeType::kWebAppTheme);
 }
 
 void AppBrowserController::OnBackgroundColorChanged() {
-  if (!has_tab_strip_)
-    return;
-  base::Optional<SkColor> background_color = GetBackgroundColor();
-  if (background_color == last_background_color_)
-    return;
-  last_background_color_ = background_color;
   UpdateThemePack();
-  browser_->window()->UserChangedTheme(BrowserThemeChangeType::kWebAppTheme);
 }
 
 base::Optional<SkColor> AppBrowserController::GetThemeColor() const {
@@ -469,7 +457,7 @@ void AppBrowserController::OnTabStripModelChanged(
     // until page loads. See |DOMContentLoaded|.
     if (change.type() != TabStripModelChange::kInserted ||
         tab_strip_model->count() == 1) {
-      DidChangeThemeColor();
+      UpdateThemePack();
     }
   }
   if (change.type() == TabStripModelChange::kInserted) {
@@ -545,8 +533,21 @@ void AppBrowserController::UpdateThemePack() {
   // TODO(crbug.com/1053823): Add tests for theme properties being set in this
   // branch.
   base::Optional<SkColor> background_color = GetBackgroundColor();
-  if (!theme_color && !background_color) {
+  if (theme_color == last_theme_color_ &&
+      background_color == last_background_color_) {
+    return;
+  }
+  last_theme_color_ = theme_color;
+  last_background_color_ = background_color;
+
+  bool no_custom_colors = !theme_color && !background_color;
+  bool non_tabbed_no_frame_color = !has_tab_strip_ && !theme_color;
+  if (no_custom_colors || non_tabbed_no_frame_color) {
     theme_pack_ = nullptr;
+    if (browser_->window()) {
+      browser_->window()->UserChangedTheme(
+          BrowserThemeChangeType::kWebAppTheme);
+    }
     return;
   }
 
@@ -569,6 +570,8 @@ void AppBrowserController::UpdateThemePack() {
   theme_pack_ = base::MakeRefCounted<BrowserThemePack>(
       CustomThemeSupplier::AUTOGENERATED);
   BrowserThemePack::BuildFromColors(colors, theme_pack_.get());
+  if (browser_->window())
+    browser_->window()->UserChangedTheme(BrowserThemeChangeType::kWebAppTheme);
 }
 
 }  // namespace web_app

chrome/browser/ui/web_applications/web_app_browsertest.cc

@@ -13,6 +13,8 @@
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
+#include "chrome/browser/themes/custom_theme_supplier.h"
+#include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/browser.h"
@@ -45,6 +47,7 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/sessions/core/tab_restore_service.h"
 #include "content/public/common/content_features.h"
+#include "content/public/test/background_color_change_waiter.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
@@ -214,6 +217,42 @@ IN_PROC_BROWSER_TEST_P(WebAppBrowserTest, BackgroundColor) {
   EXPECT_EQ(provider->registrar().GetAppBackgroundColor(app_id), SK_ColorBLUE);
 }
 
+IN_PROC_BROWSER_TEST_P(WebAppBrowserTest, BackgroundColorChange) {
+  const GURL app_url = GetSecureAppURL();
+  auto web_app_info = std::make_unique<WebApplicationInfo>();
+  web_app_info->start_url = app_url;
+  web_app_info->scope = app_url.GetWithoutFilename();
+  web_app_info->theme_color = SK_ColorBLUE;
+  const AppId app_id = InstallWebApp(std::move(web_app_info));
+
+  Browser* const app_browser = LaunchWebAppBrowser(app_id);
+  content::WebContents* const web_contents =
+      app_browser->tab_strip_model()->GetActiveWebContents();
+
+  // Wait for original background color (not cyan) to load.
+  {
+    content::BackgroundColorChangeWaiter waiter(web_contents);
+    waiter.Wait();
+    EXPECT_NE(app_browser->app_controller()->GetBackgroundColor().value(),
+              SK_ColorCYAN);
+  }
+  content::AwaitDocumentOnLoadCompleted(web_contents);
+
+  // Changing background color should update download shelf theme.
+  {
+    content::BackgroundColorChangeWaiter waiter(web_contents);
+    EXPECT_TRUE(content::ExecJs(
+        web_contents, "document.body.style.backgroundColor = 'cyan';"));
+    waiter.Wait();
+    EXPECT_EQ(app_browser->app_controller()->GetBackgroundColor().value(),
+              SK_ColorCYAN);
+    SkColor download_shelf_color;
+    app_browser->app_controller()->GetThemeSupplier()->GetColor(
+        ThemeProperties::COLOR_DOWNLOAD_SHELF, &download_shelf_color);
+    EXPECT_EQ(download_shelf_color, SK_ColorCYAN);
+  }
+}
+
 // This tests that we don't crash when launching a PWA window with an
 // autogenerated user theme set.
 IN_PROC_BROWSER_TEST_P(WebAppBrowserTest, AutoGeneratedUserThemeCrash) {