Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2186353002:
MD/vectorize wifi icons. (Closed)
|
Created:
4 years, 4 months ago by Evan Stade Modified:
4 years, 4 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD/vectorize wifi icons.
Testable with: chrome --ash-md=experimental --shill-stub=ethernet=off
BUG=617304
Committed: https://crrev.com/6ac85def240ddd290e4edcd608992c0198e4ec25
Cr-Commit-Position: refs/heads/master@{#408705}
Patch Set 1 #
Total comments: 17
Patch Set 2 : incorporate reviewer feedback #Patch Set 3 : sidestep mysterious compile failure #Messages
Total messages: 26 (12 generated)
estade@chromium.org changed reviewers: + oshima@chromium.org, tdanderson@chromium.org
oshima@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb@ would be better reviewer. here are just nits. https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:283: explicit ArcsImageSource(IconType icon_type, float signal_strength) nit: no explicit https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:291: nit: // gfx::CanvasImageSource: https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:302: const SkScalar angle_above_horizontal = 51.f; nit: kAngleAbove.. same for below. https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:370: if (image_type == ARCS) consolidate into single if?
Description was changed from ========== MD/vectorize wifi icons. Testable with: chrome --ash-md=experimental --shill-stub=ethernet=off BUG=617304 ========== to ========== MD/vectorize wifi icons. Testable with: chrome --ash-md=experimental --shill-stub=ethernet=off BUG=617304 ==========
oshima@chromium.org changed reviewers: - oshima@chromium.org
I'm not super familiar with Canvas, so I will defer to oshima / tanderson for that part, but generally this lgtm.
Awesome! lgtm with a few comments below (the most important being the 20dp size needed for the menu) https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:47: const int kNetworkIconSize = 16; Note the icon should be 16dp when displayed in the system tray but 20dp when displayed in the system menu. https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:293: gfx::RectF oval_bounds((gfx::Rect(size()))); nit: one layer of () not needed. https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:313: paint.setColor(SkColorSetA(base_color, 0x4D)); Can we move the 0x4D somewhere common so it can be used by the battery, wifi, and cellular network icons? https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:325: float size_reduction = (oval_bounds.height() / 2) * (1.f - wedge_percent); nit: const
https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:283: explicit ArcsImageSource(IconType icon_type, float signal_strength) On 2016/07/28 21:02:16, oshima wrote: > nit: no explicit Done. https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:291: On 2016/07/28 21:02:15, oshima wrote: > nit: // gfx::CanvasImageSource: Done. https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:293: gfx::RectF oval_bounds((gfx::Rect(size()))); On 2016/07/28 22:00:49, tdanderson wrote: > nit: one layer of () not needed. no, it's needed. Something about disambiguating function call bla bla https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:302: const SkScalar angle_above_horizontal = 51.f; On 2016/07/28 21:02:16, oshima wrote: > nit: > > kAngleAbove.. > same for below. Done. https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:313: paint.setColor(SkColorSetA(base_color, 0x4D)); On 2016/07/28 22:00:49, tdanderson wrote: > Can we move the 0x4D somewhere common so it can be used by the battery, wifi, > and cellular network icons? yes, and I see that you rounded 0.3*255 down instead of up and got 0x4C :) We should share that (and the base colors) but it seems like that belongs in ash/. I'm not sure why this file is not also in ash/ but currently it doesn't depend on ash/. We could create a new file for cros constants but I'd rather use one of the ones that's already in ash. Can we move this file (and the whole directory) to ash/common/system/chromeos/network/? Perhaps a task for a future CL? the base color and size constants should also be shared --- I've sprinkled some TODOs around. https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:325: float size_reduction = (oval_bounds.height() / 2) * (1.f - wedge_percent); On 2016/07/28 22:00:49, tdanderson wrote: > nit: const inlined this https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:370: if (image_type == ARCS) On 2016/07/28 21:02:16, oshima wrote: > consolidate into single if? well I anticipate very soon adding vectorized versions of the other image types, but I'll combine for now.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2186353002/#ps20001 (title: "incorporate reviewer feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:313: paint.setColor(SkColorSetA(base_color, 0x4D)); On 2016/07/29 16:59:13, Evan Stade wrote: > On 2016/07/28 22:00:49, tdanderson wrote: > > Can we move the 0x4D somewhere common so it can be used by the battery, wifi, > > and cellular network icons? > > yes, and I see that you rounded 0.3*255 down instead of up and got 0x4C :) We > should share that (and the base colors) but it seems like that belongs in ash/. > I'm not sure why this file is not also in ash/ but currently it doesn't depend > on ash/. We could create a new file for cros constants but I'd rather use one of > the ones that's already in ash. Can we move this file (and the whole directory) > to ash/common/system/chromeos/network/? Perhaps a task for a future CL? > > the base color and size constants should also be shared --- I've sprinkled some > TODOs around. This code is used in src/chrome by network_menu.cc and network_dropdown.cc. (src/chrome does not depend on src/ash). At some point those will be replaced with HTML, but that day is still a ways off.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/07/29 17:09:19, stevenjb wrote: > https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... > File ui/chromeos/network/network_icon.cc (right): > > https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... > ui/chromeos/network/network_icon.cc:313: paint.setColor(SkColorSetA(base_color, > 0x4D)); > On 2016/07/29 16:59:13, Evan Stade wrote: > > On 2016/07/28 22:00:49, tdanderson wrote: > > > Can we move the 0x4D somewhere common so it can be used by the battery, > wifi, > > > and cellular network icons? > > > > yes, and I see that you rounded 0.3*255 down instead of up and got 0x4C :) We > > should share that (and the base colors) but it seems like that belongs in > ash/. > > I'm not sure why this file is not also in ash/ but currently it doesn't depend > > on ash/. We could create a new file for cros constants but I'd rather use one > of > > the ones that's already in ash. Can we move this file (and the whole > directory) > > to ash/common/system/chromeos/network/? Perhaps a task for a future CL? > > > > the base color and size constants should also be shared --- I've sprinkled > some > > TODOs around. > > This code is used in src/chrome by network_menu.cc and network_dropdown.cc. > (src/chrome does not depend on src/ash). At some point those will be replaced > with HTML, but that day is still a ways off. Those files are both in src/chrome/browser which does depend on src/ash.
On 2016/07/29 17:17:30, Evan Stade wrote: > On 2016/07/29 17:09:19, stevenjb wrote: > > > https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... > > File ui/chromeos/network/network_icon.cc (right): > > > > > https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... > > ui/chromeos/network/network_icon.cc:313: > paint.setColor(SkColorSetA(base_color, > > 0x4D)); > > On 2016/07/29 16:59:13, Evan Stade wrote: > > > On 2016/07/28 22:00:49, tdanderson wrote: > > > > Can we move the 0x4D somewhere common so it can be used by the battery, > > wifi, > > > > and cellular network icons? > > > > > > yes, and I see that you rounded 0.3*255 down instead of up and got 0x4C :) > We > > > should share that (and the base colors) but it seems like that belongs in > > ash/. > > > I'm not sure why this file is not also in ash/ but currently it doesn't > depend > > > on ash/. We could create a new file for cros constants but I'd rather use > one > > of > > > the ones that's already in ash. Can we move this file (and the whole > > directory) > > > to ash/common/system/chromeos/network/? Perhaps a task for a future CL? > > > > > > the base color and size constants should also be shared --- I've sprinkled > > some > > > TODOs around. > > > > This code is used in src/chrome by network_menu.cc and network_dropdown.cc. > > (src/chrome does not depend on src/ash). At some point those will be replaced > > with HTML, but that day is still a ways off. > > Those files are both in src/chrome/browser which does depend on src/ash. Oh, right, sorry, I forgot that is still the case. At one point there was an effort to make Ash, or at least parts of Ash, independent of Chrome, which is why a bunch of status area UI code got moved to src/ui. I believe that a similar effort ins underway again, so please talk to jamescook@ before moving anything back to Ash.
oshima@chromium.org changed reviewers: + oshima@chromium.org
my bits lgtm https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2186353002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:370: if (image_type == ARCS) On 2016/07/29 16:59:13, Evan Stade wrote: > On 2016/07/28 21:02:16, oshima wrote: > > consolidate into single if? > > well I anticipate very soon adding vectorized versions of the other image types, > but I'll combine for now. Ah, i see. You may keep it as is then. sorry for noise.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, stevenjb@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2186353002/#ps40001 (title: "sidestep mysterious compile failure")
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 ========== MD/vectorize wifi icons. Testable with: chrome --ash-md=experimental --shill-stub=ethernet=off BUG=617304 ========== to ========== MD/vectorize wifi icons. Testable with: chrome --ash-md=experimental --shill-stub=ethernet=off BUG=617304 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MD/vectorize wifi icons. Testable with: chrome --ash-md=experimental --shill-stub=ethernet=off BUG=617304 ========== to ========== MD/vectorize wifi icons. Testable with: chrome --ash-md=experimental --shill-stub=ethernet=off BUG=617304 Committed: https://crrev.com/6ac85def240ddd290e4edcd608992c0198e4ec25 Cr-Commit-Position: refs/heads/master@{#408705} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6ac85def240ddd290e4edcd608992c0198e4ec25 Cr-Commit-Position: refs/heads/master@{#408705} |
