Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2571943002:
[ash-md] Adds VPN network status indicators in system menu (Closed)
|
Created:
4 years ago by varkha Modified:
4 years ago Reviewers:
tdanderson CC:
chromium-reviews, kalyank, stevenjb+watch_chromium.org, oshima+watch_chromium.org, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ash-md] Adds VPN network status indicators in system menu
BUG=674174
Test=Connect to a VPN network.
Verify that connected network has "Connected" text indicator.
Committed: https://crrev.com/e1c1bf58180cee3eae3a380f1f53ddb22bbedede
Cr-Commit-Position: refs/heads/master@{#438560}
Patch Set 1 #
Total comments: 8
Patch Set 2 : [ash-md] Adds VPN network status indicators in system menu (separated not closing menu) #Messages
Total messages: 22 (15 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...
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@, can you please take a look? This should be similar to https://codereview.chromium.org/2517953006/ . Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (left): https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:500: owner()->system_tray()->CloseSystemBubble(); If this CL is to be merged back into M-56 then removing this line concerns me slightly, given the number of possible things that assume the menu will close once something is selected. I have seen areas of other detailed views that only work because they rely on this menu closing behavior. However keeping the menu open is the right thing to do, so this lg but I would prefer to not merge this back to 56. https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (right): https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:3: // found in the LICENSE file. Assuming you want this merged back into M-56, please file a separate bug for this CL and use that instead in the BUG= line. https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/vpn_list_view.cc:303: AddIconAndLabel(image, label, IsConnectedOrConnecting(network)); nit: you can just pass in false for the third argument here since MD never highlights any rows. https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/vpn_list_view.cc:342: style.SetupLabel(sub_text_label()); Wouldn't you also need to set the "connecting..." label to have a CAPTION style, too? If so then we should probably be doing the same thing in the Bluetooth version of this file too (in a separate CL though).
On 2016/12/14 00:24:23, tdanderson wrote: > lgtm > > https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... > File ash/common/system/chromeos/network/network_state_list_detailed_view.cc > (left): > > https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... > ash/common/system/chromeos/network/network_state_list_detailed_view.cc:500: > owner()->system_tray()->CloseSystemBubble(); > If this CL is to be merged back into M-56 then removing this line concerns me > slightly, given the number of possible things that assume the menu will close > once something is selected. I have seen areas of other detailed views that only > work because they rely on this menu closing behavior. However keeping the menu > open is the right thing to do, so this lg but I would prefer to not merge this > back to 56. > > https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... > File ash/common/system/chromeos/network/network_state_list_detailed_view.cc > (right): > > https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... > ash/common/system/chromeos/network/network_state_list_detailed_view.cc:3: // > found in the LICENSE file. > Assuming you want this merged back into M-56, please file a separate bug for > this CL and use that instead in the BUG= line. > > https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... > File ash/common/system/chromeos/network/vpn_list_view.cc (right): > > https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... > ash/common/system/chromeos/network/vpn_list_view.cc:303: AddIconAndLabel(image, > label, IsConnectedOrConnecting(network)); > nit: you can just pass in false for the third argument here since MD never > highlights any rows. > > https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... > ash/common/system/chromeos/network/vpn_list_view.cc:342: > style.SetupLabel(sub_text_label()); > Wouldn't you also need to set the "connecting..." label to have a CAPTION style, > too? If so then we should probably be doing the same thing in the Bluetooth > version of this file too (in a separate CL though). Change "Assuming you want this merged back into M-56, please file..." to "If this is being merged back to M-56, ..."
https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (left): https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:500: owner()->system_tray()->CloseSystemBubble(); On 2016/12/14 00:24:23, tdanderson wrote: > If this CL is to be merged back into M-56 then removing this line concerns me > slightly, given the number of possible things that assume the menu will close > once something is selected. I have seen areas of other detailed views that only > work because they rely on this menu closing behavior. However keeping the menu > open is the right thing to do, so this lg but I would prefer to not merge this > back to 56. Acknowledged. https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (right): https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:3: // found in the LICENSE file. On 2016/12/14 00:24:23, tdanderson wrote: > Assuming you want this merged back into M-56, please file a separate bug for > this CL and use that instead in the BUG= line. Acknowledged. https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/vpn_list_view.cc (right): https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/vpn_list_view.cc:303: AddIconAndLabel(image, label, IsConnectedOrConnecting(network)); On 2016/12/14 00:24:23, tdanderson wrote: > nit: you can just pass in false for the third argument here since MD never > highlights any rows. Done. https://codereview.chromium.org/2571943002/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/vpn_list_view.cc:342: style.SetupLabel(sub_text_label()); On 2016/12/14 00:24:23, tdanderson wrote: > Wouldn't you also need to set the "connecting..." label to have a CAPTION style, > too? If so then we should probably be doing the same thing in the Bluetooth > version of this file too (in a separate CL though). I think code here does that, no? https://cs.chromium.org/chromium/src/ash/common/system/tray/hover_highlight_v...
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...
Description was changed from
==========
[ash-md] Adds VPN network status indicators in system menu
BUG=646698
Test=Connect to a VPN network.
Verify that connected network has "Connected" text indicator.
==========
to
==========
[ash-md] Adds VPN network status indicators in system menu
BUG=674174
Test=Connect to a VPN network.
Verify that connected network has "Connected" text indicator.
==========
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-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
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/2571943002/#ps20001 (title: "[ash-md] Adds VPN network status indicators in system menu (separated not closing menu)")
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": 20001, "attempt_start_ts": 1481736677224180,
"parent_rev": "99bd18fe8d2d28f2dd2620eead6f73d53c319bdd", "commit_rev":
"1cff0dc086e754e228750df2481b1a5e07b8f3e8"}
Message was sent while issue was closed.
Description was changed from
==========
[ash-md] Adds VPN network status indicators in system menu
BUG=674174
Test=Connect to a VPN network.
Verify that connected network has "Connected" text indicator.
==========
to
==========
[ash-md] Adds VPN network status indicators in system menu
BUG=674174
Test=Connect to a VPN network.
Verify that connected network has "Connected" text indicator.
Review-Url: https://codereview.chromium.org/2571943002
==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from
==========
[ash-md] Adds VPN network status indicators in system menu
BUG=674174
Test=Connect to a VPN network.
Verify that connected network has "Connected" text indicator.
Review-Url: https://codereview.chromium.org/2571943002
==========
to
==========
[ash-md] Adds VPN network status indicators in system menu
BUG=674174
Test=Connect to a VPN network.
Verify that connected network has "Connected" text indicator.
Committed: https://crrev.com/e1c1bf58180cee3eae3a380f1f53ddb22bbedede
Cr-Commit-Position: refs/heads/master@{#438560}
==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e1c1bf58180cee3eae3a380f1f53ddb22bbedede Cr-Commit-Position: refs/heads/master@{#438560} |
