Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817423002:
ash: Move "WiFi is turned on." buble to the notification center. (Closed)
Descriptionash: Move "WiFi is turned on." buble to the notification center.
When WiFi is enabled with WiFi toggle key, "WiFi is turned on." bubble
pops up. However, the bubble was not using the notification center.
If you don't have the key on your Chromebook, enable
chrome://flags/#ash-debug-shortcuts then Ctrl-Shift-Alt-N.
BUG=686206
TEST=manually tested.
Review-Url: https://codereview.chromium.org/2817423002
Cr-Commit-Position: refs/heads/master@{#466576}
Committed: https://chromium.googlesource.com/chromium/src/+/081c5abe1f023603f1af0db522848988255b4a7c
Patch Set 1 #
Total comments: 11
Patch Set 2 : Address review comments. #
Total comments: 5
Patch Set 3 : Fix not to break NetworkStateListDetailedView. #
Total comments: 8
Patch Set 4 : Address review comments. #Patch Set 5 : Resolve merge conflict. #
Messages
Total messages: 22 (9 generated)
tetsui@google.com changed reviewers: + fukino@chromium.org
PTAL.
Thank you for working on this! https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... File ash/system/network/tray_network.cc (right): https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.cc:180: namespace { Please merge this block with previous unnamed namespace (line 47-53) https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.cc:182: const char kWifiToggleNotificationId[] = "wifi-toggle"; nit: const => constexpr https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.cc:184: std::unique_ptr<Notification> CreateNotification() { You should be able to use Notificatoin::CreateSystemNotification(). https://codesearch.chromium.org/chromium/src/ui/message_center/notification.h... https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.cc:248: if (message_center->FindVisibleNotificationById(kWifiToggleNotificationId)) { nit: We usually omit {} for single-line statements. https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... File ash/system/network/tray_network.h (right): https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.h:17: #include "ui/message_center/message_center.h" These look unnecessary.
Thank you for reviewing! https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... File ash/system/network/tray_network.cc (right): https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.cc:180: namespace { On 2017/04/17 08:43:40, fukino wrote: > Please merge this block with previous unnamed namespace (line 47-53) Done. https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.cc:182: const char kWifiToggleNotificationId[] = "wifi-toggle"; On 2017/04/17 08:43:40, fukino wrote: > nit: const => constexpr Done. https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.cc:184: std::unique_ptr<Notification> CreateNotification() { On 2017/04/17 08:43:40, fukino wrote: > You should be able to use Notificatoin::CreateSystemNotification(). > https://codesearch.chromium.org/chromium/src/ui/message_center/notification.h... It is not possible to use CreateSystemNotification() here because we don't want SetSystemPriority() here. https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.cc:248: if (message_center->FindVisibleNotificationById(kWifiToggleNotificationId)) { On 2017/04/17 08:43:40, fukino wrote: > nit: We usually omit {} for single-line statements. Done. https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... File ash/system/network/tray_network.h (right): https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.h:17: #include "ui/message_center/message_center.h" On 2017/04/17 08:43:40, fukino wrote: > These look unnecessary. Done.
fukino@chromium.org changed reviewers: + tdanderson@chromium.org
lgtm. Thanks! Deferring to Terry for the owner review. https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... File ash/system/network/tray_network.cc (right): https://codereview.chromium.org/2817423002/diff/1/ash/system/network/tray_net... ash/system/network/tray_network.cc:184: std::unique_ptr<Notification> CreateNotification() { On 2017/04/18 01:11:33, tetsui wrote: > On 2017/04/17 08:43:40, fukino wrote: > > You should be able to use Notificatoin::CreateSystemNotification(). > > > https://codesearch.chromium.org/chromium/src/ui/message_center/notification.h... > > It is not possible to use CreateSystemNotification() here because we don't want > SetSystemPriority() here. Acknowledged.
Hi, please see my comments below: https://codereview.chromium.org/2817423002/diff/20001/ash/system/network/tray... File ash/system/network/tray_network.cc (left): https://codereview.chromium.org/2817423002/diff/20001/ash/system/network/tray... ash/system/network/tray_network.cc:176: class NetworkWifiDetailedView : public NetworkDetailedView { From what I can tell, once this CL lands you should be able to remove the NetworkDetailedView interface and make NetworkStateListDetailedView directly inherit from TrayDetailsView. If possible, could you do that in a follow-on CL (or at the very least please file a bug and add a TODO to track that work and cc me)? https://codereview.chromium.org/2817423002/diff/20001/ash/system/network/tray... ash/system/network/tray_network.cc:274: detailed_ = new tray::NetworkStateListDetailedView( I don't think you want to kill off CreateDetailedView() entirely. If |request_wifi_view_| is false then we still want to instantiate NetworkStateListDetailedView as the detailed view type, which represents the list of available networks. If you build and test this CL on device, try opening the system menu and clicking on the network row; you should see a list of available WiFi networks, but with this CL I think you will see nothing. Please let me know if I am mistaken.
Sorry for that, I'll be more careful next time. https://codereview.chromium.org/2817423002/diff/20001/ash/system/network/tray... File ash/system/network/tray_network.cc (left): https://codereview.chromium.org/2817423002/diff/20001/ash/system/network/tray... ash/system/network/tray_network.cc:176: class NetworkWifiDetailedView : public NetworkDetailedView { On 2017/04/18 23:33:32, tdanderson wrote: > From what I can tell, once this CL lands you should be able to remove the > NetworkDetailedView interface and make NetworkStateListDetailedView directly > inherit from TrayDetailsView. If possible, could you do that in a follow-on CL > (or at the very least please file a bug and add a TODO to track that work and cc > me)? Sure, I would do the follow-on CL then. https://codereview.chromium.org/2817423002/diff/20001/ash/system/network/tray... ash/system/network/tray_network.cc:274: detailed_ = new tray::NetworkStateListDetailedView( On 2017/04/18 23:33:32, tdanderson wrote: > I don't think you want to kill off CreateDetailedView() entirely. If > |request_wifi_view_| is false then we still want to instantiate > NetworkStateListDetailedView as the detailed view type, which represents the > list of available networks. If you build and test this CL on device, try opening > the system menu and clicking on the network row; you should see a list of > available WiFi networks, but with this CL I think you will see nothing. Please > let me know if I am mistaken. Done.
tdanderson@: friendly ping
LGTM, and apologies for the delay. https://codereview.chromium.org/2817423002/diff/20001/ash/system/network/tray... File ash/system/network/tray_network.cc (left): https://codereview.chromium.org/2817423002/diff/20001/ash/system/network/tray... ash/system/network/tray_network.cc:176: class NetworkWifiDetailedView : public NetworkDetailedView { On 2017/04/20 01:54:54, tetsui wrote: > On 2017/04/18 23:33:32, tdanderson wrote: > > From what I can tell, once this CL lands you should be able to remove the > > NetworkDetailedView interface and make NetworkStateListDetailedView directly > > inherit from TrayDetailsView. If possible, could you do that in a follow-on CL > > (or at the very least please file a bug and add a TODO to track that work and > cc > > me)? > > Sure, I would do the follow-on CL then. Thanks! https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... File ash/system/network/tray_network.cc (left): https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... ash/system/network/tray_network.cc:263: nit: please restore this blank line https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... File ash/system/network/tray_network.cc (right): https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... ash/system/network/tray_network.cc:37: #include "ui/views/layout/box_layout.h" nit: I think you can remove this #include now, and ditto for image_view.h. https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... ash/system/network/tray_network.cc:60: const int string_id = wifi_enabled nit: can you run 'git cl format ash' to verify this is formatted correctly? https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... ash/system/network/tray_network.cc:268: tray::kWifiToggleNotificationId)) nit: since the 'if' condition spans more than a single line, you should use {} even though the body is only a single line.
Thank you! https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... File ash/system/network/tray_network.cc (left): https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... ash/system/network/tray_network.cc:263: On 2017/04/21 15:25:02, tdanderson wrote: > nit: please restore this blank line Done. https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... File ash/system/network/tray_network.cc (right): https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... ash/system/network/tray_network.cc:37: #include "ui/views/layout/box_layout.h" On 2017/04/21 15:25:01, tdanderson wrote: > nit: I think you can remove this #include now, and ditto for image_view.h. Done. I think image_view.h is still used. https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... ash/system/network/tray_network.cc:60: const int string_id = wifi_enabled On 2017/04/21 15:25:01, tdanderson wrote: > nit: can you run 'git cl format ash' to verify this is formatted correctly? Done. https://codereview.chromium.org/2817423002/diff/40001/ash/system/network/tray... ash/system/network/tray_network.cc:268: tray::kWifiToggleNotificationId)) On 2017/04/21 15:25:02, tdanderson wrote: > nit: since the 'if' condition spans more than a single line, you should use {} > even though the body is only a single line. Done.
The CQ bit was checked by tetsui@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org, tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2817423002/#ps60001 (title: "Address review comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tetsui@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, fukino@chromium.org Link to the patchset: https://codereview.chromium.org/2817423002/#ps80001 (title: "Resolve merge conflict.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492996727370670,
"parent_rev": "1945a87fa4e60d4749fc02db1a939e21c3470e63", "commit_rev":
"081c5abe1f023603f1af0db522848988255b4a7c"}
Message was sent while issue was closed.
Description was changed from ========== ash: Move "WiFi is turned on." buble to the notification center. When WiFi is enabled with WiFi toggle key, "WiFi is turned on." bubble pops up. However, the bubble was not using the notification center. If you don't have the key on your Chromebook, enable chrome://flags/#ash-debug-shortcuts then Ctrl-Shift-Alt-N. BUG=686206 TEST=manually tested. ========== to ========== ash: Move "WiFi is turned on." buble to the notification center. When WiFi is enabled with WiFi toggle key, "WiFi is turned on." bubble pops up. However, the bubble was not using the notification center. If you don't have the key on your Chromebook, enable chrome://flags/#ash-debug-shortcuts then Ctrl-Shift-Alt-N. BUG=686206 TEST=manually tested. Review-Url: https://codereview.chromium.org/2817423002 Cr-Commit-Position: refs/heads/master@{#466576} Committed: https://chromium.googlesource.com/chromium/src/+/081c5abe1f023603f1af0db52284... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/081c5abe1f023603f1af0db52284... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
