Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714633003:
Adds more metadata to the about:webapks page (Closed)
|
Created:
3 years, 10 months ago by gonzalon Modified:
3 years, 9 months ago CC:
chromium-reviews, arv+watch_chromium.org, dominickn+watch_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org, agrieve+watch_chromium.org, Yaron Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds more metadata to the about:webapks page
Adds the following fields to the about:webapks page fore each WebAPK:
- URI
- Scope
- Manifest URL
- Start URL
- Display mode
- Orientation
- Theme color
- Background color
BUG=696562
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2714633003
Cr-Commit-Position: refs/heads/master@{#454717}
Committed: https://chromium.googlesource.com/chromium/src/+/85e97545954dc8828c9e76c1ba3ead10e9a6007e
Patch Set 1 #
Total comments: 12
Patch Set 2 : Adds more metadata to the about:webapks page #
Total comments: 7
Patch Set 3 : Adds more metadata to the about:webapks page #
Total comments: 10
Patch Set 4 : Adds more metadata to the about:webapks page #
Total comments: 4
Patch Set 5 : Adds more metadata to the about:webapks page #
Total comments: 10
Patch Set 6 : Adds more metadata to the about:webapks page #
Total comments: 8
Patch Set 7 : Adds more metadata to the about:webapks page #Patch Set 8 : Adds more metadata to the about:webapks page #
Total comments: 4
Patch Set 9 : Adds more metadata to the about:webapks page #
Total comments: 4
Patch Set 10 : Adds more metadata to the about:webapks page #Patch Set 11 : Adds more metadata to the about:webapks page #Patch Set 12 : Adds more metadata to the about:webapks page #Messages
Total messages: 52 (19 generated)
Description was changed from ========== Adds more metadata to the about:webapks page Adds the following fields to the about:webapks page fore each WebAPK: - URI - Scope - Manifest URL - Start URL - Display mode - Orientation - Theme color - Background color BUG= ========== to ========== Adds more metadata to the about:webapks page Adds the following fields to the about:webapks page fore each WebAPK: - URI - Scope - Manifest URL - Start URL - Display mode - Orientation - Theme color - Background color BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
gonzalon@chromium.org changed reviewers: + hartmanng@chromium.org, pkotwicz@chromium.org
Hi! This CL adds a few fields to have more information about the installed WebAPKs on the device. Would you mind taking a look? Thanks!
lgtm
gonzalon@chromium.org changed reviewers: + dbeam@chromium.org, dominickn@chromium.org
dbeam@chromium.org: Please review changes in chrome/browser/resources/webapks/about_webapks.js and chrome/browser/ui/webui/webapks_handler.cc dominickn@chromium.org: Please review changes in the rest of the files. Hi! Would you mind taking a look at this CL please? Thanks a lot for your time!
https://codereview.chromium.org/2714633003/diff/1/chrome/browser/resources/we... File chrome/browser/resources/webapks/about_webapks.js (right): https://codereview.chromium.org/2714633003/diff/1/chrome/browser/resources/we... chrome/browser/resources/webapks/about_webapks.js:51: can you add @param documentation here? https://codereview.chromium.org/2714633003/diff/1/chrome/browser/resources/we... chrome/browser/resources/webapks/about_webapks.js:53: webApkList.appendChild(document.createElement('br')); instead of <br><span>, why not just <div>? https://codereview.chromium.org/2714633003/diff/1/chrome/browser/resources/we... chrome/browser/resources/webapks/about_webapks.js:80: addWebApkField(webApkList, 'Manifest Start URL: ', webApkInfo.manifestStartUrl); 80 col wrap https://codereview.chromium.org/2714633003/diff/1/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2714633003/diff/1/chrome/browser/ui/webui/web... chrome/browser/ui/webui/webapks_handler.cc:45: } can this share code with https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/ntp/ntp_resource...
Thanks for reviewing! https://codereview.chromium.org/2714633003/diff/1/chrome/browser/resources/we... File chrome/browser/resources/webapks/about_webapks.js (right): https://codereview.chromium.org/2714633003/diff/1/chrome/browser/resources/we... chrome/browser/resources/webapks/about_webapks.js:51: On 2017/02/24 18:36:16, Dan Beam wrote: > can you add @param documentation here? Done. https://codereview.chromium.org/2714633003/diff/1/chrome/browser/resources/we... chrome/browser/resources/webapks/about_webapks.js:53: webApkList.appendChild(document.createElement('br')); On 2017/02/24 18:36:16, Dan Beam wrote: > instead of <br><span>, why not just <div>? Done. https://codereview.chromium.org/2714633003/diff/1/chrome/browser/resources/we... chrome/browser/resources/webapks/about_webapks.js:80: addWebApkField(webApkList, 'Manifest Start URL: ', webApkInfo.manifestStartUrl); On 2017/02/24 18:36:16, Dan Beam wrote: > 80 col wrap Done. https://codereview.chromium.org/2714633003/diff/1/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2714633003/diff/1/chrome/browser/ui/webui/web... chrome/browser/ui/webui/webapks_handler.cc:45: } On 2017/02/24 18:36:16, Dan Beam wrote: > can this share code with > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/ntp/ntp_resource... Yes, we probably can. https://cs.chromium.org/chromium/src/chrome/browser/android/webapk/webapk_ins... also uses similar code. My question is where you think this code should live? There are so many directories in this codebase, so I have no idea what the correct place would be. Or should I just leave it in one of these classes?
https://codereview.chromium.org/2714633003/diff/1/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2714633003/diff/1/chrome/browser/ui/webui/web... chrome/browser/ui/webui/webapks_handler.cc:45: } On 2017/02/24 22:30:52, gonzalon wrote: > On 2017/02/24 18:36:16, Dan Beam wrote: > > can this share code with > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/ntp/ntp_resource... > > Yes, we probably can. > https://cs.chromium.org/chromium/src/chrome/browser/android/webapk/webapk_ins... > also uses similar code. My question is where you think this code should live? > There are so many directories in this codebase, so I have no idea what the > correct place would be. Or should I just leave it in one of these classes? i think they both share chrome/browser/ui/webui as a common ancestor, so maybe in some util that already exists there or a new one if they're all different?
Can you please ensure the BUG= line is populated? https://codereview.chromium.org/2714633003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2714633003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:685: List<Long> themeColors = new ArrayList<>(); Not sure if the URI, theme color, and background color are useful pieces of information? Is there a bug for this which discusses the need for those pieces of data? https://codereview.chromium.org/2714633003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_info.cc (right): https://codereview.chromium.org/2714633003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_info.cc:5: #include "chrome/browser/android/webapk/webapk_info.h" #include <utility> for std::move() https://codereview.chromium.org/2714633003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_info.cc:20: : name(name), Every string in here should have std::move(), i.e. name(std::move(name));
Description was changed from ========== Adds more metadata to the about:webapks page Adds the following fields to the about:webapks page fore each WebAPK: - URI - Scope - Manifest URL - Start URL - Display mode - Orientation - Theme color - Background color BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Adds more metadata to the about:webapks page Adds the following fields to the about:webapks page fore each WebAPK: - URI - Scope - Manifest URL - Start URL - Display mode - Orientation - Theme color - Background color BUG=696562 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2017/02/24 22:33:52, Dan Beam wrote: > https://codereview.chromium.org/2714633003/diff/1/chrome/browser/ui/webui/web... > File chrome/browser/ui/webui/webapks_handler.cc (right): > > https://codereview.chromium.org/2714633003/diff/1/chrome/browser/ui/webui/web... > chrome/browser/ui/webui/webapks_handler.cc:45: } > On 2017/02/24 22:30:52, gonzalon wrote: > > On 2017/02/24 18:36:16, Dan Beam wrote: > > > can this share code with > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/ntp/ntp_resource... > > > > Yes, we probably can. > > > https://cs.chromium.org/chromium/src/chrome/browser/android/webapk/webapk_ins... > > also uses similar code. My question is where you think this code should live? > > There are so many directories in this codebase, so I have no idea what the > > correct place would be. Or should I just leave it in one of these classes? > > i think they both share chrome/browser/ui/webui as a common ancestor, so maybe > in some util that already exists there or a new one if they're all different? Created ColorUtils since I couldn't find other files that would fit.
https://codereview.chromium.org/2714633003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2714633003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:685: List<Long> themeColors = new ArrayList<>(); On 2017/02/27 02:27:02, dominickn wrote: > Not sure if the URI, theme color, and background color are useful pieces of > information? Is there a bug for this which discusses the need for those pieces > of data? Yaron asked me to add this offline. I created a bug as you asked and added it to the description. https://codereview.chromium.org/2714633003/diff/20001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_info.cc (right): https://codereview.chromium.org/2714633003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_info.cc:5: #include "chrome/browser/android/webapk/webapk_info.h" On 2017/02/27 02:27:02, dominickn wrote: > #include <utility> for std::move() Done. https://codereview.chromium.org/2714633003/diff/20001/chrome/browser/android/... chrome/browser/android/webapk/webapk_info.cc:20: : name(name), On 2017/02/27 02:27:02, dominickn wrote: > Every string in here should have std::move(), i.e. > > name(std::move(name)); Done.
Just nits! https://codereview.chromium.org/2714633003/diff/1/chrome/browser/android/shor... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2714633003/diff/1/chrome/browser/android/shor... chrome/browser/android/shortcut_helper.cc:381: DCHECK(short_names.size() == background_colors.size()); I don't think that the DCHECKs are useful. If the DCHECKs fail you will just crash in the for loop. My vote is for removing the DCHECKS https://codereview.chromium.org/2714633003/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk_info.h (right): https://codereview.chromium.org/2714633003/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk_info.h:11: #include "content/public/common/manifest.h" Why this include? Do you need to include third_party/WebKit/public/platform/WebDisplayMode.h https://codereview.chromium.org/2714633003/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk_info.h:53: Can you please add comments for each member variable? https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/android/... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/android/... chrome/browser/android/shortcut_helper.cc:402: static_cast<blink::WebDisplayMode>(std::move(display_modes[i])), The blink::WebDisplayMode does not need to be moved. Its an enum value https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/android/... chrome/browser/android/shortcut_helper.cc:404: std::move(orientations[i])), The blink::WebScreenOrientationLockType does not need to be moved. Its an enum value https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/android/... chrome/browser/android/shortcut_helper.cc:405: std::move(theme_colors[i]), std::move(background_colors[i]))); The colors don't need to be moved. The colors are just int64s https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/color_utils.h (right): https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/color_utils.h:12: Nit: ui/gfx/color_utils.h is probably a better home for these functions https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/color_utils.h:19: static std::string ColorToString(int64_t color); I think that this method should remain an implementation detail of webapks_handler.cc. However, it is good that ColorToString() uses SkColorToRGBAString() (and that we are sharing SkColorToRGBAString() across several places in Chrome)
https://codereview.chromium.org/2714633003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2714633003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:685: List<Long> themeColors = new ArrayList<>(); I talked to him about this. We agreed that this information is useful for debugging user bug reports (Where the user is likely going to sbirch@)
Thanks for reviewing! https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/android/... File chrome/browser/android/shortcut_helper.cc (right): https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/android/... chrome/browser/android/shortcut_helper.cc:402: static_cast<blink::WebDisplayMode>(std::move(display_modes[i])), On 2017/02/27 20:50:59, pkotwicz wrote: > The blink::WebDisplayMode does not need to be moved. Its an enum value Done. https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/android/... chrome/browser/android/shortcut_helper.cc:404: std::move(orientations[i])), On 2017/02/27 20:50:59, pkotwicz wrote: > The blink::WebScreenOrientationLockType does not need to be moved. Its an enum > value Done. https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/android/... chrome/browser/android/shortcut_helper.cc:405: std::move(theme_colors[i]), std::move(background_colors[i]))); On 2017/02/27 20:50:58, pkotwicz wrote: > The colors don't need to be moved. The colors are just int64s Done. https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/color_utils.h (right): https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/color_utils.h:12: On 2017/02/27 20:50:59, pkotwicz wrote: > Nit: ui/gfx/color_utils.h is probably a better home for these functions Done. https://codereview.chromium.org/2714633003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/color_utils.h:19: static std::string ColorToString(int64_t color); On 2017/02/27 20:50:59, pkotwicz wrote: > I think that this method should remain an implementation detail of > webapks_handler.cc. However, it is good that ColorToString() uses > SkColorToRGBAString() (and that we are sharing SkColorToRGBAString() across > several places in Chrome) It was mostly a convenience function to avoid having to call SkColorToRGBAString with a static_cast, but since I moved it to the gfx file I don't want to bloat it with many very similar functions, so I just added SkColorToRGBAString and added the cast in the places that it's called.
LGTM for the webapk/ files with nits. (I am not an OWNER though) https://codereview.chromium.org/2714633003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (left): https://codereview.chromium.org/2714633003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:82: SkColor sk_color = reinterpret_cast<uint32_t&>(color); Nit: Please keep this line and call color_utils::SkColorToRGBAString() on a separate line https://codereview.chromium.org/2714633003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2714633003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:34: std::string ColorToString(int64_t color) { This function should be in the anonymous namespace For the function comment how about: "Converts a color from the format documented in content::Manifest to a rgba() CSS string."
c/b/a/webapks lgtm, thanks.
https://codereview.chromium.org/2714633003/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (left): https://codereview.chromium.org/2714633003/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:82: SkColor sk_color = reinterpret_cast<uint32_t&>(color); On 2017/02/27 23:39:39, pkotwicz wrote: > Nit: Please keep this line and call color_utils::SkColorToRGBAString() on a > separate line Done, but it would be nice if reviews were consistent regarding this. I actually agree with putting the variable on a new line like you said, but I got reviews saying to inline it because it was 'cleaner'. Is there a guideline regarding this or is it a matter of preference according to the reviewer? I've seen many examples across the codebase using both styles. https://codereview.chromium.org/2714633003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2714633003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:34: std::string ColorToString(int64_t color) { On 2017/02/27 23:39:39, pkotwicz wrote: > This function should be in the anonymous namespace > > For the function comment how about: > "Converts a color from the format documented in content::Manifest to a rgba() > CSS string." Done.
gonzalon@chromium.org changed reviewers: + msw@chromium.org
msw@chromium.org: Would you mind taking a look at the changes in ui/gfx/color_utils for ownership? Thanks for your time!
The CQ bit was checked by dbeam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/resource... File chrome/browser/resources/webapks/about_webapks.js (right): https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/resource... chrome/browser/resources/webapks/about_webapks.js:28: * @param {string} text Text to be shown in the span. you need to add a @param for type https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/resource... chrome/browser/resources/webapks/about_webapks.js:60: label, 'div', 'app-property-label'); this indent is slightly off, maybe run `git cl format --js`? https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:323: "webui/color_utils.h", do these still exist? https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:21: if (color == content::Manifest::kInvalidOrMissingColor) { no curlies https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:22: return ""; return std::string();
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
Thanks for your time reviewing! https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/resource... File chrome/browser/resources/webapks/about_webapks.js (right): https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/resource... chrome/browser/resources/webapks/about_webapks.js:28: * @param {string} text Text to be shown in the span. On 2017/02/28 17:40:52, Dan Beam wrote: > you need to add a @param for type Done. https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/resource... chrome/browser/resources/webapks/about_webapks.js:60: label, 'div', 'app-property-label'); On 2017/02/28 17:40:52, Dan Beam wrote: > this indent is slightly off, maybe run `git cl format --js`? I didn't know the '--js' option existed, I was wondering why the js files weren't being formatted, thanks a lot! https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:323: "webui/color_utils.h", On 2017/02/28 17:40:52, Dan Beam wrote: > do these still exist? Nope. Done. https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/webapks_handler.cc (right): https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:21: if (color == content::Manifest::kInvalidOrMissingColor) { On 2017/02/28 17:40:52, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/2714633003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/webapks_handler.cc:22: return ""; On 2017/02/28 17:40:52, Dan Beam wrote: > return std::string(); Done.
I'm not certain that these color->string functions have utility beyond their current callers (especially since one appends 'rgba(...)' and the other doesn't... why is that useful?), so I'm not sure they belong in ui/gfx... but I guess it could be okay. https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h File ui/gfx/color_utils.h (right): https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:143: // Creates an rgba string for an SkColor. nit: describe the output format or give an example, like: "For example: 'rgba(255,0,255,0.5)'.". Ditto below. https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:144: GFX_EXPORT std::string SkColorToRGBAString(SkColor color); Please add unit tests for these functions. https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:147: // the css can fill it in. Remove the mention about css, it's not really relevant to this function. https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:148: GFX_EXPORT std::string SkColorToRGBComponents(SkColor color); nit: s/Components/String/ to match above.
Thanks for reviewing. Some of the strings are shown on the UI and others I'm not sure, I was just asked to extract the code in a central place so that it can be reused since there was repeated code across the codebase. Someone else suggested moving it to these color_utils files since it looks like they are dealing with SkColors. https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h File ui/gfx/color_utils.h (right): https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:143: // Creates an rgba string for an SkColor. On 2017/02/28 18:17:10, msw wrote: > nit: describe the output format or give an example, like: "For example: > 'rgba(255,0,255,0.5)'.". Ditto below. Done. https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:144: GFX_EXPORT std::string SkColorToRGBAString(SkColor color); On 2017/02/28 18:17:10, msw wrote: > Please add unit tests for these functions. Done. https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:147: // the css can fill it in. On 2017/02/28 18:17:10, msw wrote: > Remove the mention about css, it's not really relevant to this function. Done. https://codereview.chromium.org/2714633003/diff/100001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:148: GFX_EXPORT std::string SkColorToRGBComponents(SkColor color); On 2017/02/28 18:17:10, msw wrote: > nit: s/Components/String/ to match above. Done.
ui/gfx/* lgtm with minor comments. https://codereview.chromium.org/2714633003/diff/140001/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://codereview.chromium.org/2714633003/diff/140001/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:356: "rgba(%d,%d,%d,%s)", SkColorGetR(color), SkColorGetG(color), optional nit: maybe it makes sense to compose SkColorToRGBString here: return base::StringPrintf( "rgba(%s,%s)", SkColorToRGBString(color), base::DoubleToString(SkColorGetA(color) / 255.0).c_str()); https://codereview.chromium.org/2714633003/diff/140001/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:361: std::string SkColorToRGBComponents(SkColor color) { You'll need to update this (cq dry run or local compile would help)
Thanks for reviewing! https://codereview.chromium.org/2714633003/diff/140001/ui/gfx/color_utils.cc File ui/gfx/color_utils.cc (right): https://codereview.chromium.org/2714633003/diff/140001/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:356: "rgba(%d,%d,%d,%s)", SkColorGetR(color), SkColorGetG(color), On 2017/02/28 22:43:15, msw wrote: > optional nit: maybe it makes sense to compose SkColorToRGBString here: > return base::StringPrintf( > "rgba(%s,%s)", SkColorToRGBString(color), > base::DoubleToString(SkColorGetA(color) / 255.0).c_str()); Done. https://codereview.chromium.org/2714633003/diff/140001/ui/gfx/color_utils.cc#... ui/gfx/color_utils.cc:361: std::string SkColorToRGBComponents(SkColor color) { On 2017/02/28 22:43:15, msw wrote: > You'll need to update this (cq dry run or local compile would help) Done.
lgtm https://codereview.chromium.org/2714633003/diff/160001/ui/gfx/color_utils.h File ui/gfx/color_utils.h (right): https://codereview.chromium.org/2714633003/diff/160001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:144: GFX_EXPORT std::string SkColorToRGBAString(SkColor color); nit: SkColorToRgbaString() RGBA -> Rgba https://codereview.chromium.org/2714633003/diff/160001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:147: GFX_EXPORT std::string SkColorToRGBString(SkColor color); ToRgb
Thanks for all the reviews! https://codereview.chromium.org/2714633003/diff/160001/ui/gfx/color_utils.h File ui/gfx/color_utils.h (right): https://codereview.chromium.org/2714633003/diff/160001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:144: GFX_EXPORT std::string SkColorToRGBAString(SkColor color); On 2017/03/03 20:09:43, Dan Beam wrote: > nit: SkColorToRgbaString() > > RGBA -> Rgba Done. https://codereview.chromium.org/2714633003/diff/160001/ui/gfx/color_utils.h#n... ui/gfx/color_utils.h:147: GFX_EXPORT std::string SkColorToRGBString(SkColor color); On 2017/03/03 20:09:43, Dan Beam wrote: > ToRgb Done.
The CQ bit was checked by gonzalon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hartmanng@chromium.org, dominickn@chromium.org, pkotwicz@chromium.org, msw@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2714633003/#ps180001 (title: "Adds more metadata to the about:webapks page")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by gonzalon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, msw@chromium.org, pkotwicz@chromium.org, dbeam@chromium.org, hartmanng@chromium.org Link to the patchset: https://codereview.chromium.org/2714633003/#ps200001 (title: "Adds more metadata to the about:webapks page")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by gonzalon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, msw@chromium.org, pkotwicz@chromium.org, dbeam@chromium.org, hartmanng@chromium.org Link to the patchset: https://codereview.chromium.org/2714633003/#ps220001 (title: "Adds more metadata to the about:webapks page")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1488581585131460,
"parent_rev": "54b7508d67d5da9f907703daf998399a74a1d7ea", "commit_rev":
"85e97545954dc8828c9e76c1ba3ead10e9a6007e"}
Message was sent while issue was closed.
Description was changed from ========== Adds more metadata to the about:webapks page Adds the following fields to the about:webapks page fore each WebAPK: - URI - Scope - Manifest URL - Start URL - Display mode - Orientation - Theme color - Background color BUG=696562 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Adds more metadata to the about:webapks page Adds the following fields to the about:webapks page fore each WebAPK: - URI - Scope - Manifest URL - Start URL - Display mode - Orientation - Theme color - Background color BUG=696562 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2714633003 Cr-Commit-Position: refs/heads/master@{#454717} Committed: https://chromium.googlesource.com/chromium/src/+/85e97545954dc8828c9e76c1ba3e... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/85e97545954dc8828c9e76c1ba3e... |
