WebManifest: plumb around Optional<SkColor> to represent nullable colors

Rather than plumbing around an int64_t for color, using a magic value to
represent an invalid/null color, just use base::Optional.

This simplifies the code in a number of ways:
- No more magic values
- No need to assume that all non-magic int64_t values fit in a uint32_t
- No more reinterpret_cast<uint32_t&>

The conversion helpers are consolidated into color_helpers.h and shared
as well.

Bug: 836683
Change-Id: I09ac717f3c852c63680a2c1c561ee8e2fed98777
Reviewed-on: https://chromium-review.googlesource.com/1050994
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559366}

chrome/browser/BUILD.gn

@@ -1898,6 +1898,8 @@ jumbo_split_static_library("browser") {
       "android/chrome_feature_list.h",
       "android/chrome_startup_flags.cc",
       "android/chrome_startup_flags.h",
+      "android/color_helpers.cc",
+      "android/color_helpers.h",
       "android/compositor/compositor_view.cc",
       "android/compositor/compositor_view.h",
       "android/compositor/decoration_title.cc",

chrome/browser/android/color_helpers.cc

+// Copyright 2018 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 "chrome/browser/android/color_helpers.h"
+
+#include "base/numerics/safe_math.h"
+#include "ui/gfx/color_utils.h"
+
+std::string OptionalSkColorToString(const base::Optional<SkColor>& color) {
+  if (!color)
+    return std::string();
+  return color_utils::SkColorToRgbaString(*color);
+}
+
+int64_t OptionalSkColorToJavaColor(const base::Optional<SkColor>& skcolor) {
+  if (!skcolor)
+    return kInvalidJavaColor;
+  return static_cast<int32_t>(*skcolor);
+}
+
+base::Optional<SkColor> JavaColorToOptionalSkColor(int64_t java_color) {
+  if (java_color == kInvalidJavaColor)
+    return base::nullopt;
+  DCHECK(base::IsValueInRangeForNumericType<int32_t>(java_color));
+  return static_cast<SkColor>(java_color);
+}

chrome/browser/android/color_helpers.h

+// Copyright 2018 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 CHROME_BROWSER_ANDROID_COLOR_HELPERS_H_
+#define CHROME_BROWSER_ANDROID_COLOR_HELPERS_H_
+
+#include <stdint.h>
+
+#include <limits>
+#include <string>
+
+#include "base/optional.h"
+#include "third_party/skia/include/core/SkColor.h"
+
+constexpr int64_t kInvalidJavaColor =
+    static_cast<int64_t>(std::numeric_limits<int32_t>::max()) + 1;
+
+// Converts |color| to a CSS color string. If |color| is null, the empty string
+// is returned.
+std::string OptionalSkColorToString(const base::Optional<SkColor>& color);
+
+// Conversions between a Java color and an Optional<SkColor>. Java colors are
+// represented as 64-bit signed integers. Valid colors are in the range
+// [std::numeric_limits<int32_t>::min(), std::numeric_limits<int32_t>::max()].
+// while |kInvalidJavaColor| is reserved for representing a null/unset color.
+int64_t OptionalSkColorToJavaColor(const base::Optional<SkColor>& skcolor);
+base::Optional<SkColor> JavaColorToOptionalSkColor(int64_t java_color);
+
+#endif  // CHROME_BROWSER_ANDROID_COLOR_HELPERS_H_

chrome/browser/android/color_helpers_unittest.cc

+// Copyright 2018 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 "chrome/browser/android/color_helpers.h"
+
+#include <stdint.h>
+
+#include <limits>
+
+#include "base/optional.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkColor.h"
+
+namespace {
+
+// Intentionally avoids reusing the constant defined in color_helpers.h to catch
+// mistakes that accidentally change the value.
+constexpr int64_t kAndroidInvalidColor =
+    static_cast<int64_t>(std::numeric_limits<int32_t>::max()) + 1;
+
+// https://developer.android.com/reference/android/graphics/Color.html defines
+// the various color constants.
+constexpr int kAndroidBlack = -16777216;
+constexpr int kAndroidWhite = -1;
+constexpr int kAndroidRed = -65536;
+constexpr int kAndroidDkgray = -12303292;
+constexpr int kAndroidTransparent = 0;
+
+}  // namespace
+
+TEST(ColorHelpersTest, Null) {
+  EXPECT_EQ(kAndroidInvalidColor, OptionalSkColorToJavaColor(base::nullopt));
+  EXPECT_FALSE(JavaColorToOptionalSkColor(kAndroidInvalidColor).has_value());
+}
+
+TEST(ColorHelpersTest, Roundtrip) {
+  EXPECT_EQ(kAndroidBlack, OptionalSkColorToJavaColor(SK_ColorBLACK));
+  EXPECT_EQ(SK_ColorBLACK, JavaColorToOptionalSkColor(kAndroidBlack));
+
+  EXPECT_EQ(kAndroidWhite, OptionalSkColorToJavaColor(SK_ColorWHITE));
+  EXPECT_EQ(SK_ColorWHITE, JavaColorToOptionalSkColor(kAndroidWhite));
+
+  EXPECT_EQ(kAndroidRed, OptionalSkColorToJavaColor(SK_ColorRED));
+  EXPECT_EQ(SK_ColorRED, JavaColorToOptionalSkColor(kAndroidRed));
+
+  EXPECT_EQ(kAndroidDkgray, OptionalSkColorToJavaColor(SK_ColorDKGRAY));
+  EXPECT_EQ(SK_ColorDKGRAY, JavaColorToOptionalSkColor(kAndroidDkgray));
+
+  EXPECT_EQ(kAndroidTransparent,
+            OptionalSkColorToJavaColor(SK_ColorTRANSPARENT));
+  EXPECT_EQ(SK_ColorTRANSPARENT,
+            JavaColorToOptionalSkColor(kAndroidTransparent));
+}

chrome/browser/android/shortcut_helper.cc

@@ -13,9 +13,11 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/guid.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
+#include "chrome/browser/android/color_helpers.h"
 #include "chrome/browser/android/webapk/chrome_webapk_host.h"
 #include "chrome/browser/android/webapk/webapk_install_service.h"
 #include "chrome/browser/android/webapk/webapk_metrics.h"
@@ -26,6 +28,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/manifest.h"
 #include "jni/ShortcutHelper_jni.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/color_analysis.h"
 #include "url/gurl.h"
@@ -110,8 +113,10 @@ void AddWebappWithSkBitmap(const ShortcutInfo& info,
   Java_ShortcutHelper_addWebapp(
       env, java_webapp_id, java_url, java_scope_url, java_user_title, java_name,
       java_short_name, java_best_primary_icon_url, java_bitmap, info.display,
-      info.orientation, info.source, info.theme_color, info.background_color,
-      java_splash_screen_url, callback_pointer);
+      info.orientation, info.source,
+      OptionalSkColorToJavaColor(info.theme_color),
+      OptionalSkColorToJavaColor(info.background_color), java_splash_screen_url,
+      callback_pointer);
 }
 
 // Adds a shortcut which opens in a browser tab to the launcher.
@@ -430,7 +435,8 @@ void JNI_ShortcutHelper_OnWebApksRetrieved(
         std::move(manifest_start_urls[i]),
         static_cast<blink::WebDisplayMode>(display_modes[i]),
         static_cast<blink::WebScreenOrientationLockType>(orientations[i]),
-        theme_colors[i], background_colors[i],
+        JavaColorToOptionalSkColor(theme_colors[i]),
+        JavaColorToOptionalSkColor(background_colors[i]),
         base::Time::FromJavaTime(last_update_check_times_ms[i]),
         relax_updates[i]));
   }

chrome/browser/android/shortcut_info.cc

@@ -10,8 +10,6 @@ ShortcutInfo::ShortcutInfo(const GURL& shortcut_url)
       display(blink::kWebDisplayModeBrowser),
       orientation(blink::kWebScreenOrientationLockDefault),
       source(SOURCE_ADD_TO_HOMESCREEN_SHORTCUT),
-      theme_color(content::Manifest::kInvalidOrMissingColor),
-      background_color(content::Manifest::kInvalidOrMissingColor),
       ideal_splash_image_size_in_px(0),
       minimum_splash_image_size_in_px(0) {}
 
@@ -57,11 +55,11 @@ void ShortcutInfo::UpdateFromManifest(const content::Manifest& manifest) {
   }
 
   // Set the theme color based on the manifest value, if any.
-  if (manifest.theme_color != content::Manifest::kInvalidOrMissingColor)
+  if (manifest.theme_color)
     theme_color = manifest.theme_color;
 
   // Set the background color based on the manifest value, if any.
-  if (manifest.background_color != content::Manifest::kInvalidOrMissingColor)
+  if (manifest.background_color)
     background_color = manifest.background_color;
 
   // Sets the URL of the HTML splash screen, if any.

chrome/browser/android/shortcut_info.h

@@ -9,9 +9,11 @@
 
 #include <vector>
 
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "content/public/common/manifest.h"
 #include "third_party/blink/public/common/screen_orientation/web_screen_orientation_lock_type.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "url/gurl.h"
 
 // Information needed to create a shortcut via ShortcutHelper.
@@ -85,8 +87,8 @@ struct ShortcutInfo {
   blink::WebDisplayMode display;
   blink::WebScreenOrientationLockType orientation;
   Source source;
-  int64_t theme_color;
-  int64_t background_color;
+  base::Optional<SkColor> theme_color;
+  base::Optional<SkColor> background_color;
   GURL splash_screen_url;
   int ideal_splash_image_size_in_px;
   int minimum_splash_image_size_in_px;

chrome/browser/android/webapk/webapk_info.cc

@@ -17,8 +17,8 @@ WebApkInfo::WebApkInfo(std::string name,
                        std::string manifest_start_url,
                        blink::WebDisplayMode display,
                        blink::WebScreenOrientationLockType orientation,
-                       int64_t theme_color,
-                       int64_t background_color,
+                       base::Optional<SkColor> theme_color,
+                       base::Optional<SkColor> background_color,
                        base::Time last_update_check_time,
                        bool relax_updates)
     : name(std::move(name)),
@@ -39,5 +39,5 @@ WebApkInfo::WebApkInfo(std::string name,
 
 WebApkInfo::~WebApkInfo() {}
 
-WebApkInfo& WebApkInfo::operator=(WebApkInfo&& rhs) = default;
-WebApkInfo::WebApkInfo(WebApkInfo&& other) = default;
+WebApkInfo& WebApkInfo::operator=(WebApkInfo&& rhs) noexcept = default;
+WebApkInfo::WebApkInfo(WebApkInfo&& other) noexcept = default;

chrome/browser/android/webapk/webapk_info.h

@@ -11,6 +11,7 @@
 #include "base/time/time.h"
 #include "content/public/common/manifest.h"
 #include "third_party/blink/public/common/screen_orientation/web_screen_orientation_lock_type.h"
+#include "third_party/skia/include/core/SkColor.h"
 
 // Structure with information about a WebAPK.
 //
@@ -30,14 +31,14 @@ struct WebApkInfo {
              std::string manifest_start_url,
              blink::WebDisplayMode display,
              blink::WebScreenOrientationLockType orientation,
-             int64_t theme_color,
-             int64_t background_color,
+             base::Optional<SkColor> theme_color,
+             base::Optional<SkColor> background_color,
              base::Time last_update_check_time,
              bool relax_updates);
   ~WebApkInfo();
 
-  WebApkInfo& operator=(WebApkInfo&& other);
-  WebApkInfo(WebApkInfo&& other);
+  WebApkInfo& operator=(WebApkInfo&& other) noexcept;
+  WebApkInfo(WebApkInfo&& other) noexcept;
 
   // Short name of the WebAPK.
   std::string name;
@@ -60,8 +61,8 @@ struct WebApkInfo {
   std::string manifest_start_url;
   blink::WebDisplayMode display;
   blink::WebScreenOrientationLockType orientation;
-  int64_t theme_color;
-  int64_t background_color;
+  base::Optional<SkColor> theme_color;
+  base::Optional<SkColor> background_color;
   base::Time last_update_check_time;
   bool relax_updates;
 

chrome/browser/android/webapk/webapk_installer.cc

@@ -26,6 +26,7 @@
 #include "base/task_scheduler/post_task.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/timer/elapsed_timer.h"
+#include "chrome/browser/android/color_helpers.h"
 #include "chrome/browser/android/shortcut_helper.h"
 #include "chrome/browser/android/webapk/chrome_webapk_host.h"
 #include "chrome/browser/android/webapk/webapk.pb.h"
@@ -47,7 +48,6 @@
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/codec/png_codec.h"
-#include "ui/gfx/color_utils.h"
 #include "url/gurl.h"
 
 namespace {
@@ -118,15 +118,6 @@ GURL GetScope(const ShortcutInfo& info) {
                                  : ShortcutHelper::GetScopeFromURL(info.url);
 }
 
-// Converts a color from the format specified in content::Manifest to a CSS
-// string.
-std::string ColorToString(int64_t color) {
-  if (color == content::Manifest::kInvalidOrMissingColor)
-    return "";
-  SkColor sk_color = reinterpret_cast<uint32_t&>(color);
-  return color_utils::SkColorToRgbaString(sk_color);
-}
-
 webapk::WebApk_UpdateReason ConvertUpdateReasonToProtoEnum(
     WebApkUpdateReason update_reason) {
   switch (update_reason) {
@@ -216,8 +207,9 @@ std::unique_ptr<std::string> BuildProtoInBackground(
   web_app_manifest->set_display_mode(
       content::WebDisplayModeToString(shortcut_info.display));
   web_app_manifest->set_background_color(
-      ColorToString(shortcut_info.background_color));
-  web_app_manifest->set_theme_color(ColorToString(shortcut_info.theme_color));
+      OptionalSkColorToString(shortcut_info.background_color));
+  web_app_manifest->set_theme_color(
+      OptionalSkColorToString(shortcut_info.theme_color));
 
   std::string* scope = web_app_manifest->add_scopes();
   scope->assign(GetScope(shortcut_info).spec());

chrome/browser/android/webapk/webapk_update_data_fetcher.cc

@@ -9,6 +9,7 @@
 
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
+#include "chrome/browser/android/color_helpers.h"
 #include "chrome/browser/android/shortcut_helper.h"
 #include "chrome/browser/android/webapk/webapk_icon_hasher.h"
 #include "chrome/browser/android/webapk/webapk_web_manifest_checker.h"
@@ -223,8 +224,9 @@ void WebApkUpdateDataFetcher::OnDataAvailable(
 
   Java_WebApkUpdateDataFetcher_onDataAvailable(
       env, java_ref_, java_url, java_scope, java_name, java_short_name,
-      java_primary_icon_url, java_primary_icon_murmur2_hash,
-      java_primary_icon, java_badge_icon_url, java_badge_icon_murmur2_hash,
-      java_badge_icon, java_icon_urls, info_.display, info_.orientation,
-      info_.theme_color, info_.background_color);
+      java_primary_icon_url, java_primary_icon_murmur2_hash, java_primary_icon,
+      java_badge_icon_url, java_badge_icon_murmur2_hash, java_badge_icon,
+      java_icon_urls, info_.display, info_.orientation,
+      OptionalSkColorToJavaColor(info_.theme_color),
+      OptionalSkColorToJavaColor(info_.background_color));
 }

chrome/browser/extensions/bookmark_app_helper.cc

@@ -361,8 +361,8 @@ void BookmarkAppHelper::UpdateWebAppInfoFromManifest(
       web_app_info->scope = manifest.start_url.Resolve(".");
   }
 
-  if (manifest.theme_color != content::Manifest::kInvalidOrMissingColor)
-    web_app_info->theme_color = static_cast<SkColor>(manifest.theme_color);
+  if (manifest.theme_color)
+    web_app_info->theme_color = *manifest.theme_color;
 
   // If any icons are specified in the manifest, they take precedence over any
   // we picked up from the web_app stuff.

chrome/browser/ui/webui/webapks_handler.cc

@@ -9,21 +9,12 @@
 #include "base/callback_forward.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
+#include "chrome/browser/android/color_helpers.h"
 #include "chrome/browser/android/shortcut_helper.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/common/manifest_util.h"
 #include "ui/gfx/color_utils.h"
 
-namespace {
-// Converts a color from the format documented in content::Manifest to a
-// rgba() CSS string.
-std::string ColorToString(int64_t color) {
-  if (color == content::Manifest::kInvalidOrMissingColor)
-    return std::string();
-  return color_utils::SkColorToRgbaString(reinterpret_cast<uint32_t&>(color));
-}
-}  // namespace
-
 WebApksHandler::WebApksHandler() : weak_ptr_factory_(this) {}
 
 WebApksHandler::~WebApksHandler() {}
@@ -62,9 +53,10 @@ void WebApksHandler::OnWebApkInfoRetrieved(
     result->SetString(
         "orientation",
         content::WebScreenOrientationLockTypeToString(webapk_info.orientation));
-    result->SetString("themeColor", ColorToString(webapk_info.theme_color));
+    result->SetString("themeColor",
+                      OptionalSkColorToString(webapk_info.theme_color));
     result->SetString("backgroundColor",
-                      ColorToString(webapk_info.background_color));
+                      OptionalSkColorToString(webapk_info.background_color));
     result->SetDouble("lastUpdateCheckTimeMs",
                       webapk_info.last_update_check_time.ToJsTime());
     result->SetBoolean("relaxUpdates", webapk_info.relax_updates);

chrome/test/BUILD.gn

@@ -2211,6 +2211,7 @@ test("unit_tests") {
     "../browser/active_use_util_unittest.cc",
     "../browser/after_startup_task_utils_unittest.cc",
     "../browser/android/bookmarks/partner_bookmarks_shim_unittest.cc",
+    "../browser/android/color_helpers_unittest.cc",
     "../browser/android/compositor/layer/tab_layer_unittest.cc",
     "../browser/android/contextualsearch/contextual_search_delegate_unittest.cc",
     "../browser/android/contextualsearch/contextual_search_field_trial_unittest.cc",

content/public/common/manifest.cc

@@ -6,12 +6,6 @@
 
 namespace content {
 
-// We need to provide a value here which is out of the range of a 32-bit integer
-// since otherwise we would not be able to check whether a theme color was valid
-// or not. The simplest way to do this is to simply add one to the maximum
-// possible 32-bit integer.
-const int64_t Manifest::kInvalidOrMissingColor =
-    static_cast<int64_t>(std::numeric_limits<int32_t>::max()) + 1;
 const size_t Manifest::kMaxIPCStringLength = 4 * 1024;
 
 Manifest::Icon::Icon() = default;
@@ -35,11 +29,7 @@ Manifest::RelatedApplication::~RelatedApplication() = default;
 Manifest::Manifest()
     : display(blink::kWebDisplayModeUndefined),
       orientation(blink::kWebScreenOrientationLockDefault),
-      prefer_related_applications(false),
-      theme_color(Manifest::kInvalidOrMissingColor),
-      background_color(Manifest::kInvalidOrMissingColor) {
-  share_target = base::nullopt;
-}
+      prefer_related_applications(false) {}
 
 Manifest::Manifest(const Manifest& other) = default;
 
@@ -51,10 +41,8 @@ bool Manifest::IsEmpty() const {
          orientation == blink::kWebScreenOrientationLockDefault &&
          icons.empty() && !share_target.has_value() &&
          related_applications.empty() && !prefer_related_applications &&
-         theme_color == Manifest::kInvalidOrMissingColor &&
-         background_color == Manifest::kInvalidOrMissingColor &&
-         splash_screen_url.is_empty() && gcm_sender_id.is_null() &&
-         scope.is_empty();
+         !theme_color && !background_color && splash_screen_url.is_empty() &&
+         gcm_sender_id.is_null() && scope.is_empty();
 }
 
 } // namespace content

content/public/common/manifest.h

@@ -16,6 +16,7 @@
 #include "content/common/content_export.h"
 #include "third_party/blink/public/common/screen_orientation/web_screen_orientation_lock_type.h"
 #include "third_party/blink/public/platform/web_display_mode.h"
+#include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/geometry/size.h"
 #include "url/gurl.h"
 
@@ -136,19 +137,11 @@ struct CONTENT_EXPORT Manifest {
   // or there is a parsing failure.
   bool prefer_related_applications;
 
-  // This is a 64 bit integer because we need to represent an error state. The
-  // color itself should only be 32 bits long if the value is not
-  // kInvalidOrMissingColor and can be safely cast to SkColor if is valid.
-  // Set to kInvalidOrMissingColor if parsing failed or field is not
-  // present.
-  int64_t theme_color;
+  // Null if field is not present or parsing failed.
+  base::Optional<SkColor> theme_color;
 
-  // This is a 64 bit integer because we need to represent an error state. The
-  // color itself should only be 32 bits long if the value is not
-  // kInvalidOrMissingColor and can be safely cast to SkColor if is valid.
-  // Set to kInvalidOrMissingColor if parsing failed or field is not
-  // present.
-  int64_t background_color;
+  // Null if field is not present or parsing failed.
+  base::Optional<SkColor> background_color;
 
   // A URL of the HTML splash screen.
   // Empty if the parsing failed or the field was not present.
@@ -166,10 +159,6 @@ struct CONTENT_EXPORT Manifest {
   // IPC. The renderer process should truncate the strings before sending the
   // Manifest and the browser process must do the same when receiving it.
   static const size_t kMaxIPCStringLength;
-
-  // Constant representing an invalid color. Set to a value outside the
-  // range of a 32-bit integer.
-  static const int64_t kInvalidOrMissingColor;
 };
 
 } // namespace content

content/public/common/manifest_struct_traits.cc

@@ -13,12 +13,6 @@
 namespace mojo {
 namespace {
 
-bool ValidateColor(int64_t color) {
-  return color >= std::numeric_limits<int32_t>::min() ||
-         color <= std::numeric_limits<int32_t>::max() ||
-         color == content::Manifest::kInvalidOrMissingColor;
-}
-
 // A wrapper around base::Optional<base::string16> so a custom StructTraits
 // specialization can enforce maximum string length.
 struct TruncatedString16 {
@@ -77,13 +71,12 @@ bool StructTraits<blink::mojom::ManifestDataView, content::Manifest>::Read(
     return false;
 
   out->prefer_related_applications = data.prefer_related_applications();
-  out->theme_color = data.theme_color();
-  if (!ValidateColor(out->theme_color))
-    return false;
 
-  out->background_color = data.background_color();
-  if (!ValidateColor(out->background_color))
-    return false;
+  if (data.has_theme_color())
+    out->theme_color = data.theme_color();
+
+  if (data.has_background_color())
+    out->background_color = data.background_color();
 
   if (!data.ReadSplashScreenUrl(&out->splash_screen_url))
     return false;

content/public/common/manifest_struct_traits.h

@@ -63,12 +63,20 @@ struct StructTraits<blink::mojom::ManifestDataView, content::Manifest> {
     return m.orientation;
   }
 
-  static int64_t theme_color(const content::Manifest& m) {
-    return m.theme_color;
+  static bool has_theme_color(const content::Manifest& m) {
+    return m.theme_color.has_value();
   }
 
-  static int64_t background_color(const content::Manifest& m) {
-    return m.background_color;
+  static uint32_t theme_color(const content::Manifest& m) {
+    return m.theme_color.value_or(0);
+  }
+
+  static bool has_background_color(const content::Manifest& m) {
+    return m.background_color.has_value();
+  }
+
+  static uint32_t background_color(const content::Manifest& m) {
+    return m.background_color.value_or(0);
   }
 
   static const GURL& splash_screen_url(const content::Manifest& m) {

content/renderer/manifest/manifest_parser.cc

@@ -130,12 +130,12 @@ base::NullableString16 ManifestParser::ParseString(
   return base::NullableString16(value, false);
 }
 
-int64_t ManifestParser::ParseColor(
+base::Optional<SkColor> ManifestParser::ParseColor(
     const base::DictionaryValue& dictionary,
     const std::string& key) {
   base::NullableString16 parsed_color = ParseString(dictionary, key, Trim);
   if (parsed_color.is_null())
-    return Manifest::kInvalidOrMissingColor;
+    return base::nullopt;
 
   SkColor color;
   if (!blink::WebCSSParser::ParseColor(
@@ -143,15 +143,10 @@ int64_t ManifestParser::ParseColor(
     AddErrorInfo("property '" + key + "' ignored, '" +
                  base::UTF16ToUTF8(parsed_color.string()) + "' is not a " +
                  "valid color.");
-      return Manifest::kInvalidOrMissingColor;
+    return base::nullopt;
   }
 
-  // We do this here because Java does not have an unsigned int32_t type so
-  // colors with high alpha values will be negative. Instead of doing the
-  // conversion after we pass over to Java, we do it here as it is easier and
-  // clearer.
-  int32_t signed_color = reinterpret_cast<int32_t&>(color);
-  return static_cast<int64_t>(signed_color);
+  return color;
 }
 
 GURL ManifestParser::ParseURL(const base::DictionaryValue& dictionary,
@@ -442,12 +437,12 @@ bool ManifestParser::ParsePreferRelatedApplications(
   return ParseBoolean(dictionary, "prefer_related_applications", false);
 }
 
-int64_t ManifestParser::ParseThemeColor(
+base::Optional<SkColor> ManifestParser::ParseThemeColor(
     const base::DictionaryValue& dictionary) {
   return ParseColor(dictionary, "theme_color");
 }
 
-int64_t ManifestParser::ParseBackgroundColor(
+base::Optional<SkColor> ManifestParser::ParseBackgroundColor(
     const base::DictionaryValue& dictionary) {
   return ParseColor(dictionary, "background_color");
 }

content/renderer/manifest/manifest_parser.h

@@ -16,6 +16,7 @@
 #include "content/common/content_export.h"
 #include "content/public/common/manifest.h"
 #include "third_party/blink/public/platform/modules/manifest/manifest.mojom.h"
+#include "third_party/skia/include/core/SkColor.h"
 
 class GURL;
 
@@ -72,11 +73,10 @@ class CONTENT_EXPORT ManifestParser {
                                      TrimType trim);
 
   // Helper function to parse colors present on a given |dictionary| in a given
-  // field identified by its |key|.
-  // Returns the parsed color as an int64_t if any,
-  // Manifest::kInvalidOrMissingColor if the parsing failed.
-  int64_t ParseColor(const base::DictionaryValue& dictionary,
-                     const std::string& key);
+  // field identified by its |key|. Returns a null optional if the value is not
+  // present or is not a valid color.
+  base::Optional<SkColor> ParseColor(const base::DictionaryValue& dictionary,
+                                     const std::string& key);
 
   // Helper function to parse URLs present on a given |dictionary| in a given
   // field identified by its |key|. The URL is first parsed as a string then
@@ -201,15 +201,15 @@ class CONTENT_EXPORT ManifestParser {
 
   // Parses the 'theme_color' field of the manifest, as defined in:
   // https://w3c.github.io/manifest/#dfn-steps-for-processing-the-theme_color-member
-  // Returns the parsed theme color if any,
-  // Manifest::kInvalidOrMissingColor if the parsing failed.
-  int64_t ParseThemeColor(const base::DictionaryValue& dictionary);
+  // Returns the parsed theme color if any, or a null optional otherwise.
+  base::Optional<SkColor> ParseThemeColor(
+      const base::DictionaryValue& dictionary);
 
   // Parses the 'background_color' field of the manifest, as defined in:
   // https://w3c.github.io/manifest/#dfn-steps-for-processing-the-background_color-member
-  // Returns the parsed background color if any,
-  // Manifest::kInvalidOrMissingColor if the parsing failed.
-  int64_t ParseBackgroundColor(const base::DictionaryValue& dictionary);
+  // Returns the parsed background color if any, or a null optional otherwise.
+  base::Optional<SkColor> ParseBackgroundColor(
+      const base::DictionaryValue& dictionary);
 
   // Parses the 'splash_screen_url' field of the manifest.
   // Returns the parsed GURL if any, an empty GURL if the parsing failed.

third_party/blink/public/platform/modules/manifest/manifest.mojom

@@ -40,11 +40,13 @@ struct Manifest {
   // or there is a parsing failure.
   bool prefer_related_applications;
 
-  // A 32 bit color value or kInvalidOrMissingColor if not set.
-  int64 theme_color;
+  // TODO(https://crbug.com/657632): Numeric types are not nullable. =(
+  bool has_theme_color;
+  uint32 theme_color;
 
-  // A 32 bit color value or kInvalidOrMissingColor if not set.
-  int64 background_color;
+  // TODO(https://crbug.com/657632): Numeric types are not nullable. =(
+  bool has_background_color;
+  uint32 background_color;
 
   // A URL of the HTML splash screen.
   url.mojom.Url? splash_screen_url;