Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2342793005:
[ash-md] Adds Wi-Fi header row to system tray network detailed view (Closed)
Description[ash-md] Adds Wi-Fi header row to system tray network detailed view
This CL adds a Wi-Fi header row in the list of networks.
BUG=631831
Committed: https://crrev.com/0146bc4b2196fd50f780d0bdb5bfbe1645d1f784
Cr-Commit-Position: refs/heads/master@{#423330}
Patch Set 1 : [ash-md] Materializes system tray network detailed view #
Total comments: 27
Patch Set 2 : [ash-md] Materializes system tray network detailed view (comments) #
Total comments: 49
Patch Set 3 : [ash-md] Materializes system tray network detailed view (comments) #Patch Set 4 : [ash-md] Materializes system tray network detailed view (rebased) #
Total comments: 36
Patch Set 5 : [ash-md] Materializes system tray network detailed view (comments) #
Total comments: 20
Patch Set 6 : [ash-md] Materializes system tray network detailed view (prominent button color) #Patch Set 7 : [ash-md] Materializes system tray network detailed view (comments) #
Total comments: 10
Patch Set 8 : [ash-md] Materializes system tray network detailed view (simpler) #
Total comments: 8
Patch Set 9 : [ash-md] Materializes system tray network detailed view (comments) #Patch Set 10 : [ash-md] Materializes system tray network detailed view (color constant) #Patch Set 11 : [ash-md] Materializes system tray network detailed view (restored fixed row height) #
Total comments: 6
Patch Set 12 : [ash-md] Materializes system tray network detailed view (nits) #Messages
Total messages: 97 (69 generated)
The CQ bit was checked by varkha@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by varkha@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...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by varkha@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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Description was changed from ========== [ash-md] Materializes system tray network detailed view This CL adds a Wi-Fi header row in the list of networks. BUG=631831 ========== to ========== [ash-md] Materializes system tray network detailed view This CL adds a Wi-Fi header row in the list of networks. BUG=631831 ==========
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by varkha@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...
varkha@chromium.org changed reviewers: + estade@chromium.org
estade@, can you please comment on the first cut of this? One thing I could not easily figure out was why the badge did not extend all the way to the bottom (the bottom row of the badge was getting cut). I have hacked this by decrementing the position but it seems wrong. Should I be using fractional coordinates in .ico file or something? There is more work needed but I think this could be a useful intermediate step. Still want to clean up some constants before I send it for real review but should be commentable now. Thanks!
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Description was changed from ========== [ash-md] Materializes system tray network detailed view This CL adds a Wi-Fi header row in the list of networks. BUG=631831 ========== to ========== [ash-md] Adds Wi-Fi header row to system tray network detailed view This CL adds a Wi-Fi header row in the list of networks. BUG=631831 ==========
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
+tdanderson@ for early comments.
https://codereview.chromium.org/2342793005/diff/80001/ash/common/accelerators... File ash/common/accelerators/accelerator_table.cc (right): https://codereview.chromium.org/2342793005/diff/80001/ash/common/accelerators... ash/common/accelerators/accelerator_table.cc:241: {true, ui::VKEY_N, kDebugModifier, TOGGLE_WIFI}, [self review] Split this in a separate CL. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:276: height - badges_.bottom_left.height() - 1); Not sure why this is necessary but without it the bottom row of a badge is clipped. Interestingly the rightmost column of a badge is not clipped. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:319: color_(GetBaseColorForIconType(icon_type_)), [self review] Need to add a color or alpha for the badge to allow themeing both colors. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:480: if (color != SK_ColorTRANSPARENT) [self review] This seems hacky. Maybe provide a more explicit way to specify that the color is set by a client. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_list_md.cc:92: container->SetBorder(views::Border::CreateEmptyBorder(4, 18, 4, 10)); [self review] Need to move those constants above or make them common. https://codereview.chromium.org/2342793005/diff/80001/ui/gfx/vector_icons/net... File ui/gfx/vector_icons/network_badge_add_other.icon (right): https://codereview.chromium.org/2342793005/diff/80001/ui/gfx/vector_icons/net... ui/gfx/vector_icons/network_badge_add_other.icon:5: CANVAS_DIMENSIONS, 16, I thought that if I make this larger the badge will be allowed to overlap the main icon but it seems to be getting scaled into the same corner space. Is there a way to specify the absolute size of the badge here?
https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:276: height - badges_.bottom_left.height() - 1); On 2016/09/19 20:19:23, varkha wrote: > Not sure why this is necessary but without it the bottom row of a badge is > clipped. Interestingly the rightmost column of a badge is not clipped. I don't think this is correct. Note that the size of the icon you're getting back here is 24x24dp (see GetSizeForBaseIconSize). Are you sure you have enough space in your layout to display that? https://codereview.chromium.org/2342793005/diff/80001/ui/gfx/vector_icons/net... File ui/gfx/vector_icons/network_badge_add_other.icon (right): https://codereview.chromium.org/2342793005/diff/80001/ui/gfx/vector_icons/net... ui/gfx/vector_icons/network_badge_add_other.icon:5: CANVAS_DIMENSIONS, 16, On 2016/09/19 20:19:23, varkha wrote: > I thought that if I make this larger the badge will be allowed to overlap the > main icon but it seems to be getting scaled into the same corner space. Is there > a way to specify the absolute size of the badge here? I'm not sure what you're seeing. Can you post screenshots? This is the code that places the badge. It shouldn't be doing any scaling: https://cs.chromium.org/chromium/src/ui/chromeos/network/network_icon.cc?rcl=...
Informal pass lg. https://chromiumcodereview.appspot.com/2342793005/diff/80001/ash/common/syste... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (right): https://chromiumcodereview.appspot.com/2342793005/diff/80001/ash/common/syste... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:329: network_list_view_.reset(new ui::NetworkListViewMd(this)); perhaps include a comment somewhere that you've forked NetworkListViewMd from NetworkListView. https://chromiumcodereview.appspot.com/2342793005/diff/80001/ui/chromeos/netw... File ui/chromeos/network/network_icon.h (right): https://chromiumcodereview.appspot.com/2342793005/diff/80001/ui/chromeos/netw... ui/chromeos/network/network_icon.h:45: // Gets the disconnected image for a Wi-Fi network. Update comment. https://chromiumcodereview.appspot.com/2342793005/diff/80001/ui/chromeos/netw... File ui/chromeos/network/network_list_md.cc (right): https://chromiumcodereview.appspot.com/2342793005/diff/80001/ui/chromeos/netw... ui/chromeos/network/network_list_md.cc:89: SetBorder(views::Border::CreateSolidSidedBorder(1, 0, 0, 0, Can you add a TODO(tdanderson) to eventually unify this with the generic menu row class I'm working on? ui/chromeos can't include ash/common currently so regardless we'd have to leave this layout code in here for the time being. https://chromiumcodereview.appspot.com/2342793005/diff/80001/ui/chromeos/netw... ui/chromeos/network/network_list_md.cc:111: label_->SetFontList(rb.GetFontList(ui::ResourceBundle::BoldFont)); Try using TrayPopupItemStyle to set the font style/size/color properties of |label_|. https://chromiumcodereview.appspot.com/2342793005/diff/80001/ui/chromeos/netw... ui/chromeos/network/network_list_md.cc:471: WiFiHeaderRowView** view) { *& preferable to **? https://chromiumcodereview.appspot.com/2342793005/diff/80001/ui/chromeos/netw... ui/chromeos/network/network_list_md.cc:472: CHECK(view); Don't forget to remove / change to DCHECK https://chromiumcodereview.appspot.com/2342793005/diff/80001/ui/chromeos/netw... ui/chromeos/network/network_list_md.cc:503: return; nit: not needed.
The CQ bit was checked by varkha@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...
Patchset #2 (id:140001) has been deleted
Thanks. Would welcome another round of your comments. Got the badge placement, size and circular background thanks to your suggestions Thanks! https://codereview.chromium.org/2342793005/diff/80001/ash/common/accelerators... File ash/common/accelerators/accelerator_table.cc (right): https://codereview.chromium.org/2342793005/diff/80001/ash/common/accelerators... ash/common/accelerators/accelerator_table.cc:241: {true, ui::VKEY_N, kDebugModifier, TOGGLE_WIFI}, On 2016/09/19 20:19:23, varkha wrote: > [self review] Split this in a separate CL. Done. https://codereview.chromium.org/2342793005/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (right): https://codereview.chromium.org/2342793005/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:329: network_list_view_.reset(new ui::NetworkListViewMd(this)); On 2016/09/20 14:44:33, tdanderson wrote: > perhaps include a comment somewhere that you've forked NetworkListViewMd from > NetworkListView. Done. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:276: height - badges_.bottom_left.height() - 1); On 2016/09/19 20:40:47, Evan Stade wrote: > On 2016/09/19 20:19:23, varkha wrote: > > Not sure why this is necessary but without it the bottom row of a badge is > > clipped. Interestingly the rightmost column of a badge is not clipped. > > I don't think this is correct. > > Note that the size of the icon you're getting back here is 24x24dp (see > GetSizeForBaseIconSize). Are you sure you have enough space in your layout to > display that? Acknowledged. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_icon.cc:319: color_(GetBaseColorForIconType(icon_type_)), On 2016/09/19 20:19:23, varkha wrote: > [self review] Need to add a color or alpha for the badge to allow themeing both > colors. Done. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... File ui/chromeos/network/network_icon.h (right): https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_icon.h:45: // Gets the disconnected image for a Wi-Fi network. On 2016/09/20 14:44:33, tdanderson wrote: > Update comment. Done. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_list_md.cc:89: SetBorder(views::Border::CreateSolidSidedBorder(1, 0, 0, 0, On 2016/09/20 14:44:33, tdanderson wrote: > Can you add a TODO(tdanderson) to eventually unify this with the generic menu > row class I'm working on? ui/chromeos can't include ash/common currently so > regardless we'd have to leave this layout code in here for the time being. Done. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_list_md.cc:92: container->SetBorder(views::Border::CreateEmptyBorder(4, 18, 4, 10)); On 2016/09/19 20:19:23, varkha wrote: > [self review] Need to move those constants above or make them common. Done. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_list_md.cc:111: label_->SetFontList(rb.GetFontList(ui::ResourceBundle::BoldFont)); On 2016/09/20 14:44:33, tdanderson wrote: > Try using TrayPopupItemStyle to set the font style/size/color properties of > |label_|. I can't - it is in ash and I cannot use ash here. Refactoring this into UI in ash and a programmatic list in ui/chromeos would help. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_list_md.cc:471: WiFiHeaderRowView** view) { On 2016/09/20 14:44:33, tdanderson wrote: > *& preferable to **? I thought the style was to pass output parameters by pointer. Here the WiFiHeaderRowView* is returned by pointer so WiFiHeaderRowView**, no? https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_list_md.cc:472: CHECK(view); On 2016/09/20 14:44:33, tdanderson wrote: > Don't forget to remove / change to DCHECK |view| (unlike *view) should never be passed as nullptr, so the CHECK seems legit here and above in line 446. https://codereview.chromium.org/2342793005/diff/80001/ui/chromeos/network/net... ui/chromeos/network/network_list_md.cc:503: return; On 2016/09/20 14:44:33, tdanderson wrote: > nit: not needed. Done. https://codereview.chromium.org/2342793005/diff/80001/ui/gfx/vector_icons/net... File ui/gfx/vector_icons/network_badge_add_other.icon (right): https://codereview.chromium.org/2342793005/diff/80001/ui/gfx/vector_icons/net... ui/gfx/vector_icons/network_badge_add_other.icon:5: CANVAS_DIMENSIONS, 16, On 2016/09/19 20:40:47, Evan Stade wrote: > On 2016/09/19 20:19:23, varkha wrote: > > I thought that if I make this larger the badge will be allowed to overlap the > > main icon but it seems to be getting scaled into the same corner space. Is > there > > a way to specify the absolute size of the badge here? > > I'm not sure what you're seeing. Can you post screenshots? This is the code that > places the badge. It shouldn't be doing any scaling: > > https://cs.chromium.org/chromium/src/ui/chromeos/network/network_icon.cc?rcl=... Yes, your offline suggestion helped. The real size is set in a 1x icon.
The CQ bit was checked by varkha@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...
NetworkListMd is making my head spin. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_icon.cc:319: color_(GetBaseColorForIconType(icon_type_)), nit: maybe GetDefaultColorForIconType makes more sense now https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_icon.cc:922: gfx::ImageSkia GetImageForHeaderWifiNetwork(SkColor icon_color, nit: name suggestion - GetImageForNewWifiNetwork https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_icon.cc:927: GetImageForIndex(image_type, ICON_TYPE_LIST, icon_color, connected_index); I think we should just use SignalStrengthImageSource so we don't have to add a generally unused parameter to GetImageForIndex https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_info.h (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_info.h:33: bool wifi; can we call this is_wifi? https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:48: const int kWifiRowVerticalInset = 4; nit: WiFi or Wifi? I would err on the side of Wifi, because that aligns with |wifi| which is used in many places, as well as IDS_..._WIFI (which is not IDS_..._WI_FI). https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:52: const int kWifiRowHorizontalSpacing = 0; nit: remove https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:55: const SkColor kWifiRowSeparatorColor = SkColorSetA(SK_ColorBLACK, .12f * 0xFF); I've tended to calculate this out and just put the hex value here. I think we should generally stick to that just to make it easier to see matching alpha values (in case we can share a constant, for example). https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:330: void NetworkListViewMd::HandleRelayout() { inline this fn? https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:332: for (auto& iter : service_path_map_) { const? https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:349: // Insert child views I do not understand this comment https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:352: // High-priority networks (not Wi-Fi) nit: final punctuation on all comments (here and many places) https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:365: needs_relayout |= this use of |= seems kinda treacherous. It doesn't seem like you really want bitwise or, but if you switched to needs_relayout || [...] then you might skip [...] when needs_relayout is already true. really I think the much simpler approach would be to make use of View::InvalidateLayout() as appropriate and then check it at some relevant parent view (container_?) after this is all done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:376: ++index; why is this increment not inlined? https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:405: std::set<std::string>* new_service_paths, imo this function should return a set. If that set is empty, no relayout needed. The only difference is when PlaceViewAtIndex returns false, which doesn't seem overly worth worrying about. Or is it? How often do we call UpdateNetworkListEntries? https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:406: int* child_index, undocumented in/out param... oof I have no idea what this code is doing https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:410: for (auto& info : network_list_) { can this be const auto& https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:427: container_->AddChildViewAt(container, index); it is really confusing that you have both container and container_ and apparently they're different things
https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:90: Init(); a separate Init function that's called from the ctor --- not sure what that's buying us. (Also Init should not be public. Also most of this class should not be public.) https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:103: views::BoxLayout* layout1 = there's gotta be a better name for this variable https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:117: rb.GetLocalizedString(IDS_ASH_STATUS_TRAY_NETWORK_WIFI); inline? https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:141: void SetEnabled(bool enabled) { this is hiding SetEnabled on views::View (it's too bad the compiler is even configured to allow this) https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:162: toggle_->SizeToPreferredSize(); this should not be necessary. Why is it necessary? My crystal ball is telling me that your box layout is stretching the toggle too tall. You can fix this with BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:169: views::ToggleButton* toggle_; nice to see this being used https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.h (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.h:63: bool UpdateWiFiHeaderRow(bool enabled, int index, WiFiHeaderRowView** view); these are all in dire need of documentation
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. I didn't really want to rework the non-MD fork of the file (which is where some of the troubles got copied from) as it will be ultimately deleted. There was also a View leak that I fixed. See if you can spot it. Thanks! https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_icon.cc (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_icon.cc:319: color_(GetBaseColorForIconType(icon_type_)), On 2016/09/23 01:22:34, Evan Stade wrote: > nit: maybe GetDefaultColorForIconType makes more sense now Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_icon.cc:922: gfx::ImageSkia GetImageForHeaderWifiNetwork(SkColor icon_color, On 2016/09/23 01:22:34, Evan Stade wrote: > nit: name suggestion - GetImageForNewWifiNetwork Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_icon.cc:927: GetImageForIndex(image_type, ICON_TYPE_LIST, icon_color, connected_index); On 2016/09/23 01:22:34, Evan Stade wrote: > I think we should just use SignalStrengthImageSource so we don't have to add a > generally unused parameter to GetImageForIndex Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_info.h (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_info.h:33: bool wifi; On 2016/09/23 01:22:34, Evan Stade wrote: > can we call this is_wifi? Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:48: const int kWifiRowVerticalInset = 4; On 2016/09/23 01:22:35, Evan Stade wrote: > nit: WiFi or Wifi? > > I would err on the side of Wifi, because that aligns with |wifi| which is used > in many places, as well as IDS_..._WIFI (which is not IDS_..._WI_FI). Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:52: const int kWifiRowHorizontalSpacing = 0; On 2016/09/23 01:22:35, Evan Stade wrote: > nit: remove Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:55: const SkColor kWifiRowSeparatorColor = SkColorSetA(SK_ColorBLACK, .12f * 0xFF); On 2016/09/23 01:22:34, Evan Stade wrote: > I've tended to calculate this out and just put the hex value here. I think we > should generally stick to that just to make it easier to see matching alpha > values (in case we can share a constant, for example). Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:90: Init(); On 2016/09/23 01:37:59, Evan Stade wrote: > a separate Init function that's called from the ctor --- not sure what that's > buying us. (Also Init should not be public. Also most of this class should not > be public.) Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:103: views::BoxLayout* layout1 = On 2016/09/23 01:37:59, Evan Stade wrote: > there's gotta be a better name for this variable Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:117: rb.GetLocalizedString(IDS_ASH_STATUS_TRAY_NETWORK_WIFI); On 2016/09/23 01:37:59, Evan Stade wrote: > inline? Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:141: void SetEnabled(bool enabled) { On 2016/09/23 01:37:59, Evan Stade wrote: > this is hiding SetEnabled on views::View (it's too bad the compiler is even > configured to allow this) Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:162: toggle_->SizeToPreferredSize(); On 2016/09/23 01:37:59, Evan Stade wrote: > this should not be necessary. Why is it necessary? My crystal ball is telling me > that your box layout is stretching the toggle too tall. You can fix this with > BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER Yes, thank you! https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:169: views::ToggleButton* toggle_; On 2016/09/23 01:37:59, Evan Stade wrote: > nice to see this being used Acknowledged. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:330: void NetworkListViewMd::HandleRelayout() { On 2016/09/23 01:22:34, Evan Stade wrote: > inline this fn? Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:332: for (auto& iter : service_path_map_) { On 2016/09/23 01:22:35, Evan Stade wrote: > const? Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:349: // Insert child views On 2016/09/23 01:22:35, Evan Stade wrote: > I do not understand this comment See if you like this edit. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:352: // High-priority networks (not Wi-Fi) On 2016/09/23 01:22:35, Evan Stade wrote: > nit: final punctuation on all comments (here and many places) Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:365: needs_relayout |= On 2016/09/23 01:22:35, Evan Stade wrote: > this use of |= seems kinda treacherous. It doesn't seem like you really want > bitwise or, but if you switched to needs_relayout || [...] then you might skip > [...] when needs_relayout is already true. > > really I think the much simpler approach would be to make use of > View::InvalidateLayout() as appropriate and then check it at some relevant > parent view (container_?) after this is all done. See if you like this more. View::needs_layout() is protected and I didn't want to make a child class for container just to expose that. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:376: ++index; On 2016/09/23 01:22:35, Evan Stade wrote: > why is this increment not inlined? Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:405: std::set<std::string>* new_service_paths, On 2016/09/23 01:22:35, Evan Stade wrote: > imo this function should return a set. If that set is empty, no relayout needed. > The only difference is when PlaceViewAtIndex returns false, which doesn't seem > overly worth worrying about. Or is it? How often do we call > UpdateNetworkListEntries? I've left for the Update[...]View methods to worry about |needs_layout_|. My preference was to have methods that have any output parameters not favor one of them as a return value so I like it when they are all just returned by pointers with the method returning success or failure (or nothing as I've changed this). See if you are OK with this. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:406: int* child_index, On 2016/09/23 01:22:35, Evan Stade wrote: > undocumented in/out param... oof > > I have no idea what this code is doing Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:410: for (auto& info : network_list_) { On 2016/09/23 01:22:35, Evan Stade wrote: > can this be const auto& Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:427: container_->AddChildViewAt(container, index); On 2016/09/23 01:22:34, Evan Stade wrote: > it is really confusing that you have both container and container_ and > apparently they're different things Done. https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.h (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.h:63: bool UpdateWiFiHeaderRow(bool enabled, int index, WiFiHeaderRowView** view); On 2016/09/23 01:37:59, Evan Stade wrote: > these are all in dire need of documentation Done.
Patchset #3 (id:170001) has been deleted
Patchset #3 (id:190001) has been deleted
The CQ bit was checked by varkha@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 checked by varkha@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.
I agree it's not worth spending time on the non-md version. Thanks for the updates. A few more suggestions mostly targeted at improving readability (from the perspective of someone who's trying to read it). https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/160001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:365: needs_relayout |= On 2016/09/23 04:50:36, varkha wrote: > On 2016/09/23 01:22:35, Evan Stade wrote: > > this use of |= seems kinda treacherous. It doesn't seem like you really want > > bitwise or, but if you switched to needs_relayout || [...] then you might skip > > [...] when needs_relayout is already true. > > > > really I think the much simpler approach would be to make use of > > View::InvalidateLayout() as appropriate and then check it at some relevant > > parent view (container_?) after this is all done. > > See if you like this more. View::needs_layout() is protected and I didn't want > to make a child class for container just to expose that. container()->SetBoundsRect(container()->bounds()); https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:163: views::ImageButton* join_; comments please? https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:230: const bool above = nit, I think I've usually seen this written as: if (order1 != order2) return order1 < order2; if (network1->highlight != network2->highlight) return network1->highlight; return network1->service_path.compare(network2->service_path) < 0; https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:298: needs_relayout_ = false; is this necessary? https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:305: for (ServicePathMap::const_iterator it = service_path_map_.begin(); auto? https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:315: for (std::set<std::string>::const_iterator remove_it = auto? https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:338: std::set<std::string>* new_service_paths) { this one definitely seems like it should be a return value a) you always pass in an empty set b) there are no other params or return values https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:345: UpdateNetworkChildren(false /* not Wi-Fi */, new_service_paths, &index); maybe std::set<string> new_service_paths = UpdateNetworkChildren(false, 0); int last_child_index = new_service_paths.size(); https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:356: UpdateInfoLabel(message_id, &index, &no_cellular_networks_view_); I'd consider making this: UpdateInfoLabel(message_id, index, &no_cellular_networks_view_); if (no_cellular_networks_view_) /* or if (message_id) */ ++index; so that UpdateInfoLabel doesn't have to have an in-out param (which is always harder to keep track of). https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:374: UpdateNetworkChildren(true /* Wi-Fi */, new_service_paths, &index); and here: std::set<string> new_wifi_service_paths = UpdateNetworkChildren(true, last_child_index); last_child_index += new_wifi_service_paths.size(); new_service_paths.insert(new_wifi_service_paths.begin(), new_wifi_service_paths.end()); https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:381: } I think if you make all these param changes then all index incrementing is concentrated in this one function https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:430: void NetworkListViewMd::UpdateInfoLabel(int message_id, I wrote this out to see if it seemed any clearer. It's a little longer but I think the reduced nesting is a little easier to grok. wdyt? void NetworkListViewMd::UpdateInfoLabel(int message_id, int insertion_index, views::Label** label_ptr) { views::Label* label = *label_ptr; if (!message_id) { if (label) needs_relayout_ = true; delete label; *label_ptr = nullptr; return; } ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); base::string16 text = rb.GetLocalizedString(message_id); if (label) { label->SetText(text); if (PlaceViewAtIndex(label, insertion_index)) needs_relayout_ = true; return; } views::Label* new_label = delegate_->CreateInfoLabel(); new_label->SetText(text); container_->AddChildViewAt(new_label, insertion_index); needs_relayout_ = true; *label_ptr = new_label; } https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:457: int* child_index, doesn't seem necessary to make this an in/outparam, just an inparam will do + increment at callsite. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:459: CHECK(child_index); I don't think these checks are necessary; it's obvious you expect them to be non-null from the derefs below https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:486: } else if (sender == wifi_header_view_->join()) { no else after return I'd do: if (...) { ... // no return } else if (...) { ... // no return } else { NOTREACHED(); } https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.h (right): https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.h:80: // Reorders children of |container_| as necessary placing |view| at |index|. I see that this is a member of NetworkListViewBase. All data members should be private, not protected, with accessors as necessary. https://codereview.chromium.org/2342793005/diff/230001/ui/gfx/vector_icons/ne... File ui/gfx/vector_icons/network_badge_add_other.icon (right): https://codereview.chromium.org/2342793005/diff/230001/ui/gfx/vector_icons/ne... ui/gfx/vector_icons/network_badge_add_other.icon:6: PATH_COLOR_ARGB, 0xFF, 0xFF, 0xFF, 0xFF, normally we use PATH_MODE_CLEAR but I don't think that will work in this case given the way we're compositing images. Hmm...
ash/common/system lgtm https://chromiumcodereview.appspot.com/2342793005/diff/230001/ash/common/syst... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (right): https://chromiumcodereview.appspot.com/2342793005/diff/230001/ash/common/syst... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:328: // TODO(varkha): NetworkListViewMd is a temporary fork of NetworkListView. nit: please add "See crbug.com/614453" https://chromiumcodereview.appspot.com/2342793005/diff/230001/ui/chromeos/net... File ui/chromeos/network/network_list_md.h (right): https://chromiumcodereview.appspot.com/2342793005/diff/230001/ui/chromeos/net... ui/chromeos/network/network_list_md.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 here and in the .cc?
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Thanks, PTAL. https://codereview.chromium.org/2342793005/diff/230001/ash/common/system/chro... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (right): https://codereview.chromium.org/2342793005/diff/230001/ash/common/system/chro... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:328: // TODO(varkha): NetworkListViewMd is a temporary fork of NetworkListView. On 2016/09/23 22:14:10, tdanderson wrote: > nit: please add "See crbug.com/614453" Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:163: views::ImageButton* join_; On 2016/09/23 21:32:12, Evan Stade wrote: > comments please? Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:230: const bool above = On 2016/09/23 21:32:12, Evan Stade wrote: > nit, I think I've usually seen this written as: > > if (order1 != order2) > return order1 < order2; > if (network1->highlight != network2->highlight) > return network1->highlight; > return network1->service_path.compare(network2->service_path) < 0; Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:298: needs_relayout_ = false; On 2016/09/23 21:32:12, Evan Stade wrote: > is this necessary? I thought so. This update is called from time to time and nothing else ever resets |needs_relayout_| to false. Am I missing something? https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:305: for (ServicePathMap::const_iterator it = service_path_map_.begin(); On 2016/09/23 21:32:12, Evan Stade wrote: > auto? Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:315: for (std::set<std::string>::const_iterator remove_it = On 2016/09/23 21:32:12, Evan Stade wrote: > auto? Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:338: std::set<std::string>* new_service_paths) { On 2016/09/23 21:32:12, Evan Stade wrote: > this one definitely seems like it should be a return value > a) you always pass in an empty set > b) there are no other params or return values Sure. How about this? https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:345: UpdateNetworkChildren(false /* not Wi-Fi */, new_service_paths, &index); On 2016/09/23 21:32:12, Evan Stade wrote: > maybe > > std::set<string> new_service_paths = UpdateNetworkChildren(false, 0); > int last_child_index = new_service_paths.size(); Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:356: UpdateInfoLabel(message_id, &index, &no_cellular_networks_view_); On 2016/09/23 21:32:12, Evan Stade wrote: > I'd consider making this: > > UpdateInfoLabel(message_id, index, &no_cellular_networks_view_); > if (no_cellular_networks_view_) /* or if (message_id) */ > ++index; > > so that UpdateInfoLabel doesn't have to have an in-out param (which is always > harder to keep track of). Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:374: UpdateNetworkChildren(true /* Wi-Fi */, new_service_paths, &index); On 2016/09/23 21:32:13, Evan Stade wrote: > and here: > > std::set<string> new_wifi_service_paths = UpdateNetworkChildren(true, > last_child_index); > last_child_index += new_wifi_service_paths.size(); > new_service_paths.insert(new_wifi_service_paths.begin(), > new_wifi_service_paths.end()); Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:381: } On 2016/09/23 21:32:12, Evan Stade wrote: > I think if you make all these param changes then all index incrementing is > concentrated in this one function Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:430: void NetworkListViewMd::UpdateInfoLabel(int message_id, On 2016/09/23 21:32:13, Evan Stade wrote: > I wrote this out to see if it seemed any clearer. It's a little longer but I > think the reduced nesting is a little easier to grok. wdyt? > > void NetworkListViewMd::UpdateInfoLabel(int message_id, > int insertion_index, > views::Label** label_ptr) { > views::Label* label = *label_ptr; > if (!message_id) { > if (label) > needs_relayout_ = true; > delete label; > *label_ptr = nullptr; > return; > } > > ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); > base::string16 text = rb.GetLocalizedString(message_id); > if (label) { > label->SetText(text); > if (PlaceViewAtIndex(label, insertion_index)) > needs_relayout_ = true; > return; > } > > views::Label* new_label = delegate_->CreateInfoLabel(); > new_label->SetText(text); > container_->AddChildViewAt(new_label, insertion_index); > needs_relayout_ = true; > *label_ptr = new_label; > } PTAL. I realize that it's OK to delete nullptr but there is no need to if we are checking label anyway. Also reusing |label| in the last clause seemed innocent enough. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:457: int* child_index, On 2016/09/23 21:32:12, Evan Stade wrote: > doesn't seem necessary to make this an in/outparam, just an inparam will do + > increment at callsite. Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:459: CHECK(child_index); On 2016/09/23 21:32:12, Evan Stade wrote: > I don't think these checks are necessary; it's obvious you expect them to be > non-null from the derefs below Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:486: } else if (sender == wifi_header_view_->join()) { On 2016/09/23 21:32:12, Evan Stade wrote: > no else after return > > I'd do: > > if (...) { > ... > // no return > } else if (...) { > ... > // no return > } else { > NOTREACHED(); > } Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.h (right): https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/09/23 22:14:10, tdanderson wrote: > 2016 here and in the .cc? Done. https://codereview.chromium.org/2342793005/diff/230001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.h:80: // Reorders children of |container_| as necessary placing |view| at |index|. On 2016/09/23 21:32:13, Evan Stade wrote: > I see that this is a member of NetworkListViewBase. All data members should be > private, not protected, with accessors as necessary. Done. https://codereview.chromium.org/2342793005/diff/230001/ui/gfx/vector_icons/ne... File ui/gfx/vector_icons/network_badge_add_other.icon (right): https://codereview.chromium.org/2342793005/diff/230001/ui/gfx/vector_icons/ne... ui/gfx/vector_icons/network_badge_add_other.icon:6: PATH_COLOR_ARGB, 0xFF, 0xFF, 0xFF, 0xFF, On 2016/09/23 21:32:13, Evan Stade wrote: > normally we use PATH_MODE_CLEAR but I don't think that will work in this case > given the way we're compositing images. Hmm... Yes, I've tried using PATH_MODE_CLEAR first but that didn't work. Maybe having both background and foreground colors settable externally could be something to consider for vectorized icons?
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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Patchset #5 (id:250001) has been deleted
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 checked by varkha@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/2342793005/diff/270001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:51: const int kWifiRowSeparatorThickness = 1; orthogonal to this CL, can you confirm with sgabriel this is 1dp and not 1px (since most/all rules/outlines/separators in top chrome are 1px) https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:139: rb.GetFontList(ui::ResourceBundle::BoldFont)); spec seems to say medium, not bold ? https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:148: join_->SetImage(views::CustomButton::STATE_NORMAL, &join_image_); you only need to set normal, the other states will fall back to the normal image https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:152: views::ImageButton::ALIGN_MIDDLE); pretty sure this shouldn't be necessary with the cross axis centering https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:153: join_->SetMinimumImageSize(gfx::Size(kWifiIconSize, kWifiIconSize)); why is this necessary https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:220: if (!pattern.MatchesType(network->type())) nit: if (pattern.Matches...) network_list.push_back... https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:234: int order1 = GetOrder(handler_->GetNetworkState(network1->service_path)); nit: imo these temps are no longer necessary https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:425: if (PlaceViewAtIndex(network_view, index)) looks like you always handle the return value the same way. Maybe make PlaceViewAtIndex a void fn and have it set needs_relayout_? https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:453: ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); nit: inline rb imo
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:51: const int kWifiRowSeparatorThickness = 1; On 2016/09/29 21:07:58, Evan Stade wrote: > orthogonal to this CL, can you confirm with sgabriel this is 1dp and not 1px > (since most/all rules/outlines/separators in top chrome are 1px) The spec says 1dp. I think this separator would look odd if it is single native resolution 1 pixel thick. I will double check though. https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:139: rb.GetFontList(ui::ResourceBundle::BoldFont)); On 2016/09/29 21:07:58, Evan Stade wrote: > spec seems to say medium, not bold ? Done. https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:148: join_->SetImage(views::CustomButton::STATE_NORMAL, &join_image_); On 2016/09/29 21:07:58, Evan Stade wrote: > you only need to set normal, the other states will fall back to the normal image Done. https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:152: views::ImageButton::ALIGN_MIDDLE); On 2016/09/29 21:07:58, Evan Stade wrote: > pretty sure this shouldn't be necessary with the cross axis centering Done. https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:153: join_->SetMinimumImageSize(gfx::Size(kWifiIconSize, kWifiIconSize)); On 2016/09/29 21:07:58, Evan Stade wrote: > why is this necessary I think the current code didn't need it anymore. https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:220: if (!pattern.MatchesType(network->type())) On 2016/09/29 21:07:58, Evan Stade wrote: > nit: if (pattern.Matches...) > network_list.push_back... Done. https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:234: int order1 = GetOrder(handler_->GetNetworkState(network1->service_path)); On 2016/09/29 21:07:58, Evan Stade wrote: > nit: imo these temps are no longer necessary Isn't it better to eval those once though? The implementation of GetNetworkState() seems non-trivial. https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:425: if (PlaceViewAtIndex(network_view, index)) On 2016/09/29 21:07:59, Evan Stade wrote: > looks like you always handle the return value the same way. Maybe make > PlaceViewAtIndex a void fn and have it set needs_relayout_? Done. Simpler indeed. https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:453: ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); On 2016/09/29 21:07:58, Evan Stade wrote: > nit: inline rb imo Done.
The CQ bit was checked by varkha@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.
https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:234: int order1 = GetOrder(handler_->GetNetworkState(network1->service_path)); On 2016/09/29 23:31:52, varkha wrote: > On 2016/09/29 21:07:58, Evan Stade wrote: > > nit: imo these temps are no longer necessary > > Isn't it better to eval those once though? The implementation of > GetNetworkState() seems non-trivial. ah somehow i missed that they were used twice each. nevermind. https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:127: // views::BoxLayout::MAIN_AXIS_ALIGNMENT_CENTER); ? https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:157: views::Border::CreateEmptyBorder(vertical_padding, horizontal_padding, optional nit: can be views::Border::CreateEmptyBorder(gfx::Insets(vert, horiz)); https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:161: kFocusBorderColor, gfx::Insets(1, 1, 1, 1))); gfx::Insets(1) https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:167: views::View* toggle_container = new views::View; this doesn't seem right. The commented out code above doesn't work? https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:474: if (label) { if you make PlaceViewAtIndex handle the case where the view's not yet parented, this can be: if (!label) label = delegate_->CreateInfoLabel(); label->SetText(ui::ResourceBundle...); PlaceViewAtIndex(label, insertion_index); *label_ptr = label; other sites like UpdateNetworkChild can get shorter too.
Yes, those suggestions simplify code quite a bit. Thanks! https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/270001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:234: int order1 = GetOrder(handler_->GetNetworkState(network1->service_path)); On 2016/10/03 19:28:39, Evan Stade wrote: > On 2016/09/29 23:31:52, varkha wrote: > > On 2016/09/29 21:07:58, Evan Stade wrote: > > > nit: imo these temps are no longer necessary > > > > Isn't it better to eval those once though? The implementation of > > GetNetworkState() seems non-trivial. > > ah somehow i missed that they were used twice each. nevermind. Acknowledged. https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:127: // views::BoxLayout::MAIN_AXIS_ALIGNMENT_CENTER); On 2016/10/03 19:28:39, Evan Stade wrote: > ? Done. https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:157: views::Border::CreateEmptyBorder(vertical_padding, horizontal_padding, On 2016/10/03 19:28:39, Evan Stade wrote: > optional nit: can be > > views::Border::CreateEmptyBorder(gfx::Insets(vert, horiz)); Done. https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:161: kFocusBorderColor, gfx::Insets(1, 1, 1, 1))); On 2016/10/03 19:28:39, Evan Stade wrote: > gfx::Insets(1) Done. https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:167: views::View* toggle_container = new views::View; On 2016/10/03 19:28:39, Evan Stade wrote: > this doesn't seem right. The commented out code above doesn't work? Yes, needed to specify CROSS_AXIS_ALIGNMENT_CENTER for container_layout. https://codereview.chromium.org/2342793005/diff/300001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:474: if (label) { On 2016/10/03 19:28:39, Evan Stade wrote: > if you make PlaceViewAtIndex handle the case where the view's not yet parented, > this can be: > > if (!label) > label = delegate_->CreateInfoLabel(); > label->SetText(ui::ResourceBundle...); > PlaceViewAtIndex(label, insertion_index); > *label_ptr = label; > > other sites like UpdateNetworkChild can get shorter too. Done.
https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:56: const SkColor kFocusBorderColor = SkColorSetRGB(64, 128, 250); ick! could you at least use the color_palette constant instead? Also I am surprised we don't want to use the inkdrop highlight for focus as we do on other image buttons (browser actions, star icon, etc.) https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:113: int GetHeightForWidth(int width) const override { not sure you need this function since it parrots GetPreferredSize
https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:56: const SkColor kFocusBorderColor = SkColorSetRGB(64, 128, 250); On 2016/10/03 21:38:50, Evan Stade wrote: > ick! > > could you at least use the color_palette constant instead? Also I am surprised > we don't want to use the inkdrop highlight for focus as we do on other image > buttons (browser actions, star icon, etc.) Done. Yes, we would likely need ink drop for those. This is just in sync with the rest of the UI in the system menu. kGoogleBlue500 is a very close color. https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:113: int GetHeightForWidth(int width) const override { On 2016/10/03 21:38:50, Evan Stade wrote: > not sure you need this function since it parrots GetPreferredSize Done.
lgtm https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:54: ; ^H https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:56: const SkColor kFocusBorderColor = SkColorSetRGB(64, 128, 250); On 2016/10/03 22:28:23, varkha wrote: > On 2016/10/03 21:38:50, Evan Stade wrote: > > ick! > > > > could you at least use the color_palette constant instead? Also I am surprised > > we don't want to use the inkdrop highlight for focus as we do on other image > > buttons (browser actions, star icon, etc.) > > Done. Yes, we would likely need ink drop for those. This is just in sync with > the rest of the UI in the system menu. kGoogleBlue500 is a very close color. if you want to use an ink drop (highlight), it should be easy to add now. I'm ok with punting, but it should only be a few lines so it could easily be added to this CL.
https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:54: ; On 2016/10/03 23:59:10, Evan Stade wrote: > ^H Done. https://codereview.chromium.org/2342793005/diff/320001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.cc:56: const SkColor kFocusBorderColor = SkColorSetRGB(64, 128, 250); On 2016/10/03 23:59:10, Evan Stade wrote: > On 2016/10/03 22:28:23, varkha wrote: > > On 2016/10/03 21:38:50, Evan Stade wrote: > > > ick! > > > > > > could you at least use the color_palette constant instead? Also I am > surprised > > > we don't want to use the inkdrop highlight for focus as we do on other image > > > buttons (browser actions, star icon, etc.) > > > > Done. Yes, we would likely need ink drop for those. This is just in sync with > > the rest of the UI in the system menu. kGoogleBlue500 is a very close color. > > if you want to use an ink drop (highlight), it should be easy to add now. I'm ok > with punting, but it should only be a few lines so it could easily be added to > this CL. Acknowledged.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2342793005/#ps360001 (title: "[ash-md] Materializes system tray network detailed view (color constant)")
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 varkha@chromium.org
varkha@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb@ for OWNERS in ui/chromeos. Thanks!
The CQ bit was checked by varkha@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.
varkha@chromium.org changed reviewers: + oshima@chromium.org - stevenjb@chromium.org
oshima@, can you please take a look for OWNERS in ui/chromeos? Thanks!
lgtm with nits https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... File ui/chromeos/network/network_list.cc (right): https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... ui/chromeos/network/network_list.cc:267: views::View* network_view = NULL; would you mind changing NULL -> nullptr in this file? https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.h (right): https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.h:89: views::Label** label_ptr); I think _out is more common. https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.h:113: typedef std::map<views::View*, std::string> NetworkMap; nit: using NetworkMap = ...
https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... File ui/chromeos/network/network_list.cc (right): https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... ui/chromeos/network/network_list.cc:267: views::View* network_view = NULL; On 2016/10/05 20:20:11, oshima wrote: > would you mind changing NULL -> nullptr in this file? Done. https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... File ui/chromeos/network/network_list_md.h (right): https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.h:89: views::Label** label_ptr); On 2016/10/05 20:20:11, oshima wrote: > I think _out is more common. label_ptr is both in and out parameter so I just made this explicit in a comment. https://codereview.chromium.org/2342793005/diff/380001/ui/chromeos/network/ne... ui/chromeos/network/network_list_md.h:113: typedef std::map<views::View*, std::string> NetworkMap; On 2016/10/05 20:20:11, oshima wrote: > nit: using NetworkMap = ... Done.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, estade@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2342793005/#ps400001 (title: "[ash-md] Materializes system tray network detailed view (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.
Description was changed from ========== [ash-md] Adds Wi-Fi header row to system tray network detailed view This CL adds a Wi-Fi header row in the list of networks. BUG=631831 ========== to ========== [ash-md] Adds Wi-Fi header row to system tray network detailed view This CL adds a Wi-Fi header row in the list of networks. BUG=631831 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Adds Wi-Fi header row to system tray network detailed view This CL adds a Wi-Fi header row in the list of networks. BUG=631831 ========== to ========== [ash-md] Adds Wi-Fi header row to system tray network detailed view This CL adds a Wi-Fi header row in the list of networks. BUG=631831 Committed: https://crrev.com/0146bc4b2196fd50f780d0bdb5bfbe1645d1f784 Cr-Commit-Position: refs/heads/master@{#423330} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/0146bc4b2196fd50f780d0bdb5bfbe1645d1f784 Cr-Commit-Position: refs/heads/master@{#423330} |
