Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2321153002:
CrOS MD Network icon badging: layout (Closed)
|
Created:
4 years, 3 months ago by Evan Stade Modified:
4 years, 1 month ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCrOS MD Network icon badging: layout
Adjusts layout for the badges (they extend past the edge of the signal
icon). The old raster badges are still used rather than vectorized
assets.
BUG=617301
Committed: https://crrev.com/8551bd506a320063e1870e3b87415b8e1df1edac
Cr-Commit-Position: refs/heads/master@{#417956}
Patch Set 1 #
Total comments: 11
Patch Set 2 : tdanderson review #
Total comments: 4
Patch Set 3 : nits #Messages
Total messages: 19 (12 generated)
The CQ bit was checked by estade@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@chromium.org changed reviewers: + derat@chromium.org, tdanderson@chromium.org
+tdanderson for primary review +derat for OWNERS review (feel free to wait till after Terry's review)
lgtm with nits https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:65: : top_left(NULL), nit: NULL -> nullptr while you're here https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:214: class NetworkIconImageSource : public gfx::CanvasImageSource { optional: add a TODO to remove this class and reference crbug.com/614453 https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:248: // This defines how we assemble a network icon. nit: "...in material design" https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:259: int width = size().width(); int: const for width and height https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:297: int badge_offset = base_icon_size.width() == 16 ? kTrayIconBadgeOffset nit: const https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:342: // TODO(estade): use kTrayIconSize and kMenuIconSize. nit: can you define 16 and 20 as constants in this file, change lines 297 and 343, and move the TODO to above the constants? Also reference crbug.com/623987 in your TODO.
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 estade@chromium.org to run a CQ dry run
https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:65: : top_left(NULL), On 2016/09/08 19:11:57, tdanderson wrote: > nit: NULL -> nullptr while you're here Done. https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:214: class NetworkIconImageSource : public gfx::CanvasImageSource { On 2016/09/08 19:11:58, tdanderson wrote: > optional: add a TODO to remove this class and reference crbug.com/614453 I don't think it's necessary because it's guarded by UseMd(), so when we remove UseMd() it should be no longer used and obvious that we can remove it. https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:248: // This defines how we assemble a network icon. On 2016/09/08 19:11:58, tdanderson wrote: > nit: "...in material design" Do you think the class name makes that apparent? https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:297: int badge_offset = base_icon_size.width() == 16 ? kTrayIconBadgeOffset On 2016/09/08 19:11:58, tdanderson wrote: > nit: const why? there are lots of local variables where we could apply const but don't. I think it adds clutter most of the time. I just went up and looked up the c++ style guide. My reading is that constexpr/const should be used for compile-time constants (these are not that) and const should be used for function parameters, data members, and methods. Local variables are none of these. https://codereview.chromium.org/2321153002/diff/1/ui/chromeos/network/network... ui/chromeos/network/network_icon.cc:342: // TODO(estade): use kTrayIconSize and kMenuIconSize. On 2016/09/08 19:11:58, tdanderson wrote: > nit: can you define 16 and 20 as constants in this file, change lines 297 and > 343, and move the TODO to above the constants? Also reference crbug.com/623987 > in your TODO. done (more or less)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
owner lgtm with nits, most of which weren't caused by your change https://codereview.chromium.org/2321153002/diff/20001/ui/chromeos/network/net... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2321153002/diff/20001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:261: int width = size().width(); nit: make this and other unchanging locals const? https://codereview.chromium.org/2321153002/diff/20001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:542: gfx::ImageSkia* ConnectingVpnImage(double animation) { nit: mind renaming this to something closer to GetConnectingVpnImage (sigh, which is taken by another function) and documenting what |animation| is? it looks like a class name right now. https://codereview.chromium.org/2321153002/diff/20001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:670: icon->size()); nit: worthwhile refactoring this? if i'm not misreading, you could probably do something like: gfx::ImageSkia icon; Badges badges; if (connected_network) { icon = GetImageForNetwork(connected_network, icon_type); badges.bottom_left = ConnectingVpnBadge(animation); } else { icon = *ConnectingVpnImage(animation); } return UseMd() ? NetworkIconImageSourceMd::CreateImage(icon, badges) : gfx::ImageSkia(new NetworkIconImageSource(icon, badges), icon.size()); i think it's only two lines shorter but it makes the differences clearer. https://codereview.chromium.org/2321153002/diff/20001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:780: if (vpn && vpn_badge_ == nullptr) { uber-nit: it feels a bit confusing to coerce one pointer to bool and explicitly compare another pointer against nullptr in the same condition; mind just switching to !vpn_badge_? ditto in the else-if
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 estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2321153002/#ps40001 (title: "nits")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== CrOS MD Network icon badging: layout Adjusts layout for the badges (they extend past the edge of the signal icon). The old raster badges are still used rather than vectorized assets. BUG=617301 ========== to ========== CrOS MD Network icon badging: layout Adjusts layout for the badges (they extend past the edge of the signal icon). The old raster badges are still used rather than vectorized assets. BUG=617301 Committed: https://crrev.com/8551bd506a320063e1870e3b87415b8e1df1edac Cr-Commit-Position: refs/heads/master@{#417956} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8551bd506a320063e1870e3b87415b8e1df1edac Cr-Commit-Position: refs/heads/master@{#417956} |
