Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2264383002:
More closely align palette to spec. (Closed)
DescriptionMore closely align palette to spec.
BUG=638975
Committed: https://crrev.com/31bd95138a51522e0c77d88269b1cd44728ff021
Cr-Commit-Position: refs/heads/master@{#414896}
Patch Set 1 : Initial upload #
Total comments: 8
Patch Set 2 : Address comments #Patch Set 3 : Use 48x48 icon as source #
Total comments: 2
Patch Set 4 : Add fs to icon numbers #Messages
Total messages: 39 (23 generated)
The CQ bit was checked by jdufault@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jdufault@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Description was changed from ========== More closely align palette to spec. BUG=638975 ========== to ========== More closely align palette to spec. BUG=638975 ==========
jdufault@chromium.org changed reviewers: + estade@chromium.org, stevenjb@chromium.org
stevenjb@, PTAL at ash/*. estade@, PTAL at ui/gfx/*. Thanks!
https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... File ui/gfx/vector_icons/help.icon (right): https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... ui/gfx/vector_icons/help.icon:1: // Copyright 2016 The Chromium Authors. All rights reserved. please only call this icon "help" if it matches https://icons.googleplex.com/#icon=ic_help (in which case I wonder why the dimensions are 40x40)
https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... File ui/gfx/vector_icons/help.icon (right): https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... ui/gfx/vector_icons/help.icon:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/08/24 01:30:40, Evan Stade (ooo wed-thurs) wrote: > please only call this icon "help" if it matches > https://icons.googleplex.com/#icon=ic_help (in which case I wonder why the > dimensions are 40x40) Those look like the same icon to me. I can look into importing the 48x48 variant. Do we already have ic_help imported? I'm not sure what you mean by only calling this icon "help" though, since it looks like the filename is already "help".
On 2016/08/24 01:38:41, jdufault wrote: > https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... > File ui/gfx/vector_icons/help.icon (right): > > https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... > ui/gfx/vector_icons/help.icon:1: // Copyright 2016 The Chromium Authors. All > rights reserved. > On 2016/08/24 01:30:40, Evan Stade (ooo wed-thurs) wrote: > > please only call this icon "help" if it matches > > https://icons.googleplex.com/#icon=ic_help (in which case I wonder why the > > dimensions are 40x40) > > Those look like the same icon to me. I can look into importing the 48x48 > variant. Do we already have ic_help imported? > > I'm not sure what you mean by only calling this icon "help" though, since it > looks like the filename is already "help". let me try to clarify that: call it "help" only if it matches ic_help
On 2016/08/24 01:51:35, Evan Stade (ooo wed-thurs) wrote: > On 2016/08/24 01:38:41, jdufault wrote: > > > https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... > > File ui/gfx/vector_icons/help.icon (right): > > > > > https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... > > ui/gfx/vector_icons/help.icon:1: // Copyright 2016 The Chromium Authors. All > > rights reserved. > > On 2016/08/24 01:30:40, Evan Stade (ooo wed-thurs) wrote: > > > please only call this icon "help" if it matches > > > https://icons.googleplex.com/#icon=ic_help (in which case I wonder why the > > > dimensions are 40x40) > > > > Those look like the same icon to me. I can look into importing the 48x48 > > variant. Do we already have ic_help imported? > > > > I'm not sure what you mean by only calling this icon "help" though, since it > > looks like the filename is already "help". > > let me try to clarify that: call it "help" only if it matches ic_help Yes, it does. Are we fine importing at the current 40x40 dimensions or do you prefer 48x48?
On 2016/08/24 01:53:32, jdufault wrote: > On 2016/08/24 01:51:35, Evan Stade (ooo wed-thurs) wrote: > > On 2016/08/24 01:38:41, jdufault wrote: > > > > > > https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... > > > File ui/gfx/vector_icons/help.icon (right): > > > > > > > > > https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... > > > ui/gfx/vector_icons/help.icon:1: // Copyright 2016 The Chromium Authors. All > > > rights reserved. > > > On 2016/08/24 01:30:40, Evan Stade (ooo wed-thurs) wrote: > > > > please only call this icon "help" if it matches > > > > https://icons.googleplex.com/#icon=ic_help (in which case I wonder why the > > > > dimensions are 40x40) > > > > > > Those look like the same icon to me. I can look into importing the 48x48 > > > variant. Do we already have ic_help imported? > > > > > > I'm not sure what you mean by only calling this icon "help" though, since it > > > looks like the filename is already "help". > > > > let me try to clarify that: call it "help" only if it matches ic_help > > Yes, it does. Are we fine importing at the current 40x40 dimensions or do you > prefer 48x48? 48 please
On 2016/08/24 02:05:57, Evan Stade (ooo wed-thurs) wrote: > On 2016/08/24 01:53:32, jdufault wrote: > > On 2016/08/24 01:51:35, Evan Stade (ooo wed-thurs) wrote: > > > On 2016/08/24 01:38:41, jdufault wrote: > > > > > > > > > > https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... > > > > File ui/gfx/vector_icons/help.icon (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2264383002/diff/20001/ui/gfx/vector_icons/hel... > > > > ui/gfx/vector_icons/help.icon:1: // Copyright 2016 The Chromium Authors. > All > > > > rights reserved. > > > > On 2016/08/24 01:30:40, Evan Stade (ooo wed-thurs) wrote: > > > > > please only call this icon "help" if it matches > > > > > https://icons.googleplex.com/#icon=ic_help (in which case I wonder why > the > > > > > dimensions are 40x40) > > > > > > > > Those look like the same icon to me. I can look into importing the 48x48 > > > > variant. Do we already have ic_help imported? > > > > > > > > I'm not sure what you mean by only calling this icon "help" though, since > it > > > > looks like the filename is already "help". > > > > > > let me try to clarify that: call it "help" only if it matches ic_help > > > > Yes, it does. Are we fine importing at the current 40x40 dimensions or do you > > prefer 48x48? > > 48 please also, we already have help_outline.icon. Is there a good reason we don't want to reuse it or switch its users to help.icon?
https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:85: // TODO: Add HELP icon TODO(jdufault) https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:190: auto* separator = CreateSeparator(views::Separator::HORIZONTAL); We generally avoid using auto* like this. It's allowed in line 184 since the type is already there (but doesn't save much and is a little inconsistent with other places, e.g. line 177), but here it is less clear what exactly |separator| is. i.e. auto is great for unwieldy iterators, but less ideal for a simple pointer to a class. There are more guidelines in the Google Style Guide: https://google.github.io/styleguide/cppguide.html#auto https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_header_button.cc (right): https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_popup_header_button.cc:51: accessible_name_id) { This is awkward and may be tricky to debug if GetImageForResourceId were to return null. Instead, can we move the code in the constructor above into a private method and call that from each constructor instead?
https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/chrom... File ash/common/system/chromeos/palette/palette_tray.cc (right): https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:85: // TODO: Add HELP icon On 2016/08/24 15:58:57, stevenjb wrote: > TODO(jdufault) Oops - I forgot to delete that before submitting :/ https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/chrom... ash/common/system/chromeos/palette/palette_tray.cc:190: auto* separator = CreateSeparator(views::Separator::HORIZONTAL); On 2016/08/24 15:58:57, stevenjb wrote: > We generally avoid using auto* like this. It's allowed in line 184 since the > type is already there (but doesn't save much and is a little inconsistent with > other places, e.g. line 177), but here it is less clear what exactly |separator| > is. i.e. auto is great for unwieldy iterators, but less ideal for a simple > pointer to a class. > > There are more guidelines in the Google Style Guide: > https://google.github.io/styleguide/cppguide.html#auto Done. https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_header_button.cc (right): https://codereview.chromium.org/2264383002/diff/20001/ash/common/system/tray/... ash/common/system/tray/tray_popup_header_button.cc:51: accessible_name_id) { On 2016/08/24 15:58:57, stevenjb wrote: > This is awkward and may be tricky to debug if GetImageForResourceId were to > return null. Instead, can we move the code in the constructor above into a > private method and call that from each constructor instead? Done.
The CQ bit was checked by jdufault@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by jdufault@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...
estade@, PTAL. I've converted the icon to defined in terms of a 48x48 format.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
lgtm https://codereview.chromium.org/2264383002/diff/60001/ui/gfx/vector_icons/hel... File ui/gfx/vector_icons/help.icon (right): https://codereview.chromium.org/2264383002/diff/60001/ui/gfx/vector_icons/hel... ui/gfx/vector_icons/help.icon:31: R_CUBIC_TO, 0, 1.76, -0.71, 3.35, -1.87, 4.51, don't you need fs on all of these?
https://codereview.chromium.org/2264383002/diff/60001/ui/gfx/vector_icons/hel... File ui/gfx/vector_icons/help.icon (right): https://codereview.chromium.org/2264383002/diff/60001/ui/gfx/vector_icons/hel... ui/gfx/vector_icons/help.icon:31: R_CUBIC_TO, 0, 1.76, -0.71, 3.35, -1.87, 4.51, On 2016/08/27 00:24:33, Evan Stade (ooo wed-thurs) wrote: > don't you need fs on all of these? Yep, looks like the windows compile failed.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2264383002/#ps80001 (title: "Add fs to icon numbers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== More closely align palette to spec. BUG=638975 ========== to ========== More closely align palette to spec. BUG=638975 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== More closely align palette to spec. BUG=638975 ========== to ========== More closely align palette to spec. BUG=638975 Committed: https://crrev.com/31bd95138a51522e0c77d88269b1cd44728ff021 Cr-Commit-Position: refs/heads/master@{#414896} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/31bd95138a51522e0c77d88269b1cd44728ff021 Cr-Commit-Position: refs/heads/master@{#414896} |
