Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2692943005:
[ash-md] Disables proxy settings button when there are no networks (Closed)
|
Created:
3 years, 10 months ago by varkha Modified:
3 years, 10 months 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] Disables proxy settings button when there are no networks
When converting to MD the code that updated the proxy settings
button became lost. This CL restores it.
BUG=677926
TEST=visual per bug description
Review-Url: https://codereview.chromium.org/2692943005
Cr-Commit-Position: refs/heads/master@{#450862}
Committed: https://chromium.googlesource.com/chromium/src/+/7009b6753eccab02d997e91702bccb9041442a5c
Patch Set 1 #
Total comments: 5
Patch Set 2 : [ash-md] Disables proxy settings button when there are no networks (comments) #
Messages
Total messages: 22 (14 generated)
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@, can you please take a look? 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.
https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (right): https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:536: nullptr); So I assume that just putting this in CreateExtraTitleRowButtons() would be insufficient because the button state may change during the lifetime of the system menu? Also is Update() guaranteed to be called after CreateExtraTitleRowButtons()? (Would it make sense to set the enabled state of this button in CreateExtraTitleRowButtons() as well?) https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/network_state_list_detailed_view.h (right): https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/network_state_list_detailed_view.h:77: // Update UI components when Update() is called. I don't think this comment is necessary.
PTAL. https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (right): https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:536: nullptr); On 2017/02/14 23:00:46, tdanderson wrote: > So I assume that just putting this in CreateExtraTitleRowButtons() would be > insufficient because the button state may change during the lifetime of the > system menu? > > Also is Update() guaranteed to be called after CreateExtraTitleRowButtons()? > (Would it make sense to set the enabled state of this button in > CreateExtraTitleRowButtons() as well?) I've restored the logic that existed before the MD changes. Looking at it some more it's a "yes" to both questions. The state can change because we have e.g. accelerators for disabling Wi-Fi (and the state of connection may change during the lifetime of the menu). Update is called after creating the controls - https://cs.chromium.org/chromium/src/ash/common/system/chromeos/network/netwo... To see old code path - https://chromiumcodereview.appspot.com/2429923002 https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/network_state_list_detailed_view.h (right): https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/network_state_list_detailed_view.h:77: // Update UI components when Update() is called. On 2017/02/14 23:00:46, tdanderson wrote: > I don't think this comment is necessary. 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.
LGTM https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/network/network_state_list_detailed_view.cc (right): https://codereview.chromium.org/2692943005/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/network/network_state_list_detailed_view.cc:536: nullptr); On 2017/02/15 19:27:10, varkha wrote: > On 2017/02/14 23:00:46, tdanderson wrote: > > So I assume that just putting this in CreateExtraTitleRowButtons() would be > > insufficient because the button state may change during the lifetime of the > > system menu? > > > > Also is Update() guaranteed to be called after CreateExtraTitleRowButtons()? > > (Would it make sense to set the enabled state of this button in > > CreateExtraTitleRowButtons() as well?) > > I've restored the logic that existed before the MD changes. Looking at it some > more it's a "yes" to both questions. The state can change because we have e.g. > accelerators for disabling Wi-Fi (and the state of connection may change during > the lifetime of the menu). Update is called after creating the controls - > https://cs.chromium.org/chromium/src/ash/common/system/chromeos/network/netwo... > > To see old code path - https://chromiumcodereview.appspot.com/2429923002 Ah I see, thanks. Sorry, looks like this was caused by an oversight on my part.
The CQ bit was checked by varkha@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by varkha@chromium.org
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": 1487205310132000,
"parent_rev": "9fad3677dfb44803f01ad43c92b9dc1a82b40794", "commit_rev":
"7009b6753eccab02d997e91702bccb9041442a5c"}
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Disables proxy settings button when there are no networks When converting to MD the code that updated the proxy settings button became lost. This CL restores it. BUG=677926 TEST=visual per bug description ========== to ========== [ash-md] Disables proxy settings button when there are no networks When converting to MD the code that updated the proxy settings button became lost. This CL restores it. BUG=677926 TEST=visual per bug description Review-Url: https://codereview.chromium.org/2692943005 Cr-Commit-Position: refs/heads/master@{#450862} Committed: https://chromium.googlesource.com/chromium/src/+/7009b6753eccab02d997e91702bc... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7009b6753eccab02d997e91702bc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
