New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 836683 link

Starred by 2 users
Status: Fixed
Owner:
dcheng@chromium.org
Closed: May 2018
Cc:
jyasskin@chromium.org
dominickn@chromium.org
█ gonzalon@chromium.org
dgozman@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

chrome/browser/ui/webui/webapks_handler.cc has a dubious looking cast

Project Member Reported by dcheng@chromium.org, Apr 25 2018 
Current snippet (note: kInvalidOrMissingColor = std::numeric_limits<uint32_t>::max() + 1):

  std::string ColorToString(int64_t color) {
    if (color == blink::Manifest::kInvalidOrMissingColor)
      return std::string();
    return color_utils::SkColorToRgbaString(reinterpret_cast<uint32_t&>(color));
  }

We should probably just use static_cast<> here.

Also, since we're treating the color as a bitfield, it should probably just be defined as a uint64_t.

Comment 1 by dcheng@chromium.org, Apr 25 2018 

Actually, even better would be to treat this as a nullable type and use Optional, but unfortunately, Mojo doesn't allow primitives to be nullable types.
Project Member

Comment 2 by bugdroid1@chromium.org, May 17 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78

commit 8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78
Author: Daniel Cheng <dcheng@chromium.org>
Date: Thu May 17 00:39:27 2018

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}
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/BUILD.gn
[add] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/color_helpers.cc
[add] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/color_helpers.h
[add] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/color_helpers_unittest.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/shortcut_helper.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/shortcut_info.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/shortcut_info.h
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/webapk/webapk_info.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/webapk/webapk_info.h
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/webapk/webapk_installer.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/extensions/bookmark_app_helper.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/browser/ui/webui/webapks_handler.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/chrome/test/BUILD.gn
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/content/public/common/manifest.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/content/public/common/manifest.h
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/content/public/common/manifest_struct_traits.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/content/public/common/manifest_struct_traits.h
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/content/renderer/manifest/manifest_parser.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/content/renderer/manifest/manifest_parser.h
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/content/renderer/manifest/manifest_parser_unittest.cc
[modify] https://crrev.com/8fedab242d38e9dbb6ad7bbf294cfb5da2c30c78/third_party/blink/public/platform/modules/manifest/manifest.mojom

Comment 3 by dcheng@chromium.org, May 17 2018

Owner: dcheng@chromium.org
Status: Fixed (was: Available)
►

Sign in to add a comment