desktop-pwas: Consolidate theme_color opaquening to UpdateWebAppInfoFromManifest()

This CL moves the logic where we make a web app's theme_color opaque to
UpdateWebAppInfoFromManifest() instead of at installation (BMO) or at
read (bookmark apps).
This is important for manifest update checks that needs to compare
the transformed installed web app manifest data with the current page's
manifest data under the same transformations.

Bug: 1103590
Change-Id: Ib243227999ea13c1f00e146b20bb55c39482978d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2288719Reviewed-by: Jeevan Shikaram <jshikaram@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788509}

chrome/browser/chromeos/apps/apk_web_app_installer.cc

@@ -90,8 +90,8 @@ void ApkWebAppInstaller::Start(arc::mojom::WebAppInfoPtr web_app_info,
   DCHECK(web_app_info_->scope.is_valid());
 
   if (web_app_info->theme_color != kInvalidColor) {
-    web_app_info_->theme_color =
-        static_cast<SkColor>(web_app_info->theme_color);
+    web_app_info_->theme_color = SkColorSetA(
+        static_cast<SkColor>(web_app_info->theme_color), SK_AlphaOPAQUE);
   }
   web_app_info_->display_mode = blink::mojom::DisplayMode::kStandalone;
   web_app_info_->open_as_window = true;

chrome/browser/chromeos/apps/apk_web_app_installer_unittest.cc

@@ -24,7 +24,7 @@ namespace {
 arc::mojom::WebAppInfoPtr GetWebAppInfo() {
   return arc::mojom::WebAppInfo::New("Fake App Title",
                                      "https://www.google.com/index.html",
-                                     "https://www.google.com/", 10000);
+                                     "https://www.google.com/", 0xFFAABBCC);
 }
 
 constexpr int kGeneratedIconSize = 128;
@@ -118,8 +118,8 @@ TEST_F(ApkWebAppInstallerTest, IconDecodeCallsWebAppInstallManager) {
             apk_web_app_installer.web_app_info().app_url);
   EXPECT_EQ(GURL("https://www.google.com/"),
             apk_web_app_installer.web_app_info().scope);
-  EXPECT_EQ(10000,
-            static_cast<int32_t>(
+  EXPECT_EQ(0xFFAABBCC,
+            static_cast<uint32_t>(
                 apk_web_app_installer.web_app_info().theme_color.value()));
 
   EXPECT_EQ(1u, apk_web_app_installer.web_app_info().icon_bitmaps_any.size());

chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/ui/views/location_bar/custom_tab_bar_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
+#include "chrome/browser/web_applications/components/web_app_install_utils.h"
 #include "chrome/common/web_application_info.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -24,6 +25,7 @@
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/theme_change_waiter.h"
+#include "third_party/blink/public/common/manifest/manifest.h"
 #include "third_party/blink/public/mojom/frame/fullscreen.mojom.h"
 #include "ui/base/theme_provider.h"
 
@@ -48,20 +50,20 @@ class BrowserNonClientFrameViewBrowserTest
   // longer be hosted apps when BMO ships.
   void InstallAndLaunchBookmarkApp(
       base::Optional<GURL> app_url = base::nullopt) {
-    if (!app_url)
-      app_url = GetAppURL();
+    blink::Manifest manifest;
+    manifest.start_url = app_url.value_or(GetAppURL());
+    manifest.scope = manifest.start_url.GetWithoutFilename();
+    manifest.theme_color = app_theme_color_;
+
     auto web_app_info = std::make_unique<WebApplicationInfo>();
-    web_app_info->app_url = *app_url;
-    web_app_info->scope = app_url->GetWithoutFilename();
-    if (app_theme_color_)
-      web_app_info->theme_color = *app_theme_color_;
+    web_app::UpdateWebAppInfoFromManifest(manifest, web_app_info.get());
 
     web_app::AppId app_id =
         web_app::InstallWebApp(profile(), std::move(web_app_info));
     app_browser_ = web_app::LaunchWebAppBrowser(profile(), app_id);
     web_contents_ = app_browser_->tab_strip_model()->GetActiveWebContents();
     // Ensure the main page has loaded and is ready for ExecJs DOM manipulation.
-    ASSERT_TRUE(content::NavigateToURL(web_contents_, *app_url));
+    ASSERT_TRUE(content::NavigateToURL(web_contents_, manifest.start_url));
 
     app_browser_view_ = BrowserView::GetBrowserViewForBrowser(app_browser_);
     app_frame_view_ = app_browser_view_->frame()->GetFrameView();
@@ -127,6 +129,21 @@ IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
             app_frame_view_->GetFrameColor(BrowserFrameActiveState::kActive));
 }
 
+// Tests that an opaque frame color is used for a web app with a transparent
+// theme color.
+IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
+                       OpaqueFrameColorForTransparentWebAppThemeColor) {
+  // Ensure we're not using the system theme on Linux.
+  ThemeService* theme_service =
+      ThemeServiceFactory::GetForProfile(browser()->profile());
+  theme_service->UseDefaultTheme();
+
+  app_theme_color_ = SkColorSetA(SK_ColorBLUE, 0x88);
+  InstallAndLaunchBookmarkApp();
+  EXPECT_EQ(app_frame_view_->GetFrameColor(BrowserFrameActiveState::kActive),
+            SK_ColorBLUE);
+}
+
 // Tests the frame color for a bookmark app when the system theme is applied.
 IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest,
                        BookmarkAppFrameColorSystemTheme) {

chrome/browser/ui/web_applications/web_app_browsertest.cc

@@ -118,10 +118,13 @@ using WebAppTabRestoreBrowserTest = WebAppBrowserTest;
 IN_PROC_BROWSER_TEST_P(WebAppBrowserTest, ThemeColor) {
   {
     const SkColor theme_color = SkColorSetA(SK_ColorBLUE, 0xF0);
+    blink::Manifest manifest;
+    manifest.start_url = GURL(kExampleURL);
+    manifest.scope = GURL(kExampleURL);
+    manifest.theme_color = theme_color;
     auto web_app_info = std::make_unique<WebApplicationInfo>();
-    web_app_info->app_url = GURL(kExampleURL);
-    web_app_info->scope = GURL(kExampleURL);
-    web_app_info->theme_color = theme_color;
+    web_app::UpdateWebAppInfoFromManifest(manifest, web_app_info.get());
+
     AppId app_id = InstallWebApp(std::move(web_app_info));
     Browser* app_browser = LaunchWebAppBrowser(app_id);
 

chrome/browser/web_applications/components/web_app_install_utils.cc

@@ -144,8 +144,10 @@ void UpdateWebAppInfoFromManifest(const blink::Manifest& manifest,
   if (manifest.scope.is_valid())
     web_app_info->scope = manifest.scope;
 
-  if (manifest.theme_color)
-    web_app_info->theme_color = *manifest.theme_color;
+  if (manifest.theme_color) {
+    web_app_info->theme_color =
+        SkColorSetA(*manifest.theme_color, SK_AlphaOPAQUE);
+  }
 
   if (manifest.display != DisplayMode::kUndefined)
     web_app_info->display_mode = manifest.display;

chrome/browser/web_applications/extensions/bookmark_app_registrar.cc

@@ -124,12 +124,7 @@ base::Optional<SkColor> BookmarkAppRegistrar::GetAppThemeColor(
   if (!extension)
     return base::nullopt;
 
-  base::Optional<SkColor> extension_theme_color =
-      AppThemeColorInfo::GetThemeColor(extension);
-  if (extension_theme_color)
-    return SkColorSetA(*extension_theme_color, SK_AlphaOPAQUE);
-
-  return base::nullopt;
+  return AppThemeColorInfo::GetThemeColor(extension);
 }
 
 const GURL& BookmarkAppRegistrar::GetAppLaunchURL(

chrome/browser/web_applications/manifest_update_manager_browsertest.cc

@@ -631,13 +631,17 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest,
   install_observer.SetWebAppUninstalledDelegate(
       base::BindLambdaForTesting([](const AppId& app_id) { NOTREACHED(); }));
 
-  OverrideManifest(kManifestTemplate, {kInstallableIconList, "red"});
+  // CSS #RRGGBBAA syntax.
+  OverrideManifest(kManifestTemplate, {kInstallableIconList, "#00FF00F0"});
   EXPECT_EQ(GetResultAfterPageLoad(GetAppURL(), &app_id),
             ManifestUpdateResult::kAppUpdated);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,
                                       ManifestUpdateResult::kAppUpdated, 1);
   AwaitShortcutsUpdated(kInstallableIconTopLeftColor);
-  EXPECT_EQ(GetProvider().registrar().GetAppThemeColor(app_id), SK_ColorRED);
+
+  // Updated theme_color loses any transparency.
+  EXPECT_EQ(GetProvider().registrar().GetAppThemeColor(app_id),
+            SkColorSetARGB(0xFF, 0x00, 0xFF, 0x00));
 }
 
 IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest, CheckKeepsSameName) {

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -431,8 +431,8 @@ void WebAppInstallFinalizer::SetWebAppManifestFieldsAndWriteData(
   web_app->SetDescription(base::UTF16ToUTF8(web_app_info.description));
   web_app->SetScope(web_app_info.scope);
   if (web_app_info.theme_color) {
-    web_app->SetThemeColor(
-        SkColorSetA(*web_app_info.theme_color, SK_AlphaOPAQUE));
+    DCHECK_EQ(SkColorGetA(*web_app_info.theme_color), SK_AlphaOPAQUE);
+    web_app->SetThemeColor(web_app_info.theme_color);
   }
 
   WebApp::SyncFallbackData sync_fallback_data;

chrome/browser/web_applications/web_app_install_task_unittest.cc

@@ -391,8 +391,7 @@ TEST_F(WebAppInstallTaskTest, InstallFromWebContents) {
   const std::string manifest_name = "Manifest Name";
   const std::string description = "Description";
   const GURL scope = GURL("https://example.com/scope");
-  const base::Optional<SkColor> theme_color = 0xAABBCCDD;
-  const base::Optional<SkColor> expected_theme_color = 0xFFBBCCDD;  // Opaque.
+  const base::Optional<SkColor> theme_color = 0xFFAABBCC;
 
   const AppId app_id = GenerateAppIdFromURL(url);
 
@@ -435,7 +434,7 @@ TEST_F(WebAppInstallTaskTest, InstallFromWebContents) {
   EXPECT_EQ(description, web_app->description());
   EXPECT_EQ(url, web_app->launch_url());
   EXPECT_EQ(scope, web_app->scope());
-  EXPECT_EQ(expected_theme_color, web_app->theme_color());
+  EXPECT_EQ(theme_color, web_app->theme_color());
 }
 
 TEST_F(WebAppInstallTaskTest, ForceReinstall) {
@@ -1199,8 +1198,7 @@ TEST_F(WebAppInstallTaskWithRunOnOsLoginTest,
   const std::string name = "Name";
   const std::string description = "Description";
   const GURL scope = GURL("https://example.com/scope");
-  const base::Optional<SkColor> theme_color = 0xAABBCCDD;
-  const base::Optional<SkColor> expected_theme_color = 0xFFBBCCDD;  // Opaque.
+  const base::Optional<SkColor> theme_color = 0xFFAABBCC;
 
   const AppId app_id = GenerateAppIdFromURL(url);
 
@@ -1233,7 +1231,7 @@ TEST_F(WebAppInstallTaskWithRunOnOsLoginTest,
   EXPECT_EQ(description, web_app->description());
   EXPECT_EQ(url, web_app->launch_url());
   EXPECT_EQ(scope, web_app->scope());
-  EXPECT_EQ(expected_theme_color, web_app->theme_color());
+  EXPECT_EQ(theme_color, web_app->theme_color());
   EXPECT_EQ(1u, test_shortcut_manager().num_register_run_on_os_login_calls());
 }
 
@@ -1245,8 +1243,7 @@ TEST_F(WebAppInstallTaskWithRunOnOsLoginTest,
   const std::string name = "Name";
   const std::string description = "Description";
   const GURL scope = GURL("https://example.com/scope");
-  const base::Optional<SkColor> theme_color = 0xAABBCCDD;
-  const base::Optional<SkColor> expected_theme_color = 0xFFBBCCDD;  // Opaque.
+  const base::Optional<SkColor> theme_color = 0xFFAABBCC;
 
   const AppId app_id = GenerateAppIdFromURL(url);
 
@@ -1279,7 +1276,7 @@ TEST_F(WebAppInstallTaskWithRunOnOsLoginTest,
   EXPECT_EQ(description, web_app->description());
   EXPECT_EQ(url, web_app->launch_url());
   EXPECT_EQ(scope, web_app->scope());
-  EXPECT_EQ(expected_theme_color, web_app->theme_color());
+  EXPECT_EQ(theme_color, web_app->theme_color());
   EXPECT_EQ(0u, test_shortcut_manager().num_register_run_on_os_login_calls());
 }
 
@@ -1432,8 +1429,8 @@ class WebAppInstallTaskTestWithShortcutsMenu : public WebAppInstallTaskTest {
 
   static constexpr char kShortcutItemName[] = "shortcut item";
   static constexpr SquareSizePx kIconSize = 128;
-  static constexpr SkColor kInitialThemeColor = 0x000000;
-  static constexpr SkColor kFinalThemeColor = 0xFFFFFF;
+  static constexpr SkColor kInitialThemeColor = 0xFF000000;
+  static constexpr SkColor kFinalThemeColor = 0xFFFFFFFF;
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;