Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2702493002:
[ash-md] Disables the Wi-Fi and Cellular toggles until the state changes (Closed)
|
Created:
3 years, 10 months ago by varkha Modified:
3 years, 9 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 the Wi-Fi and Cellular toggles until the state changes
BUG=687922
TEST=Visual
On a mobile-equipped Chromebook (e.g. link with 3G)
Try to toggle Mobile toggle off and immediately on
Expected: The toggle cannot be re-enabled immediately.
When it can finally be re-enabled, it stays enabled.
Review-Url: https://codereview.chromium.org/2702493002
Cr-Commit-Position: refs/heads/master@{#455357}
Committed: https://chromium.googlesource.com/chromium/src/+/6a8e668da879ad1fc3b442a8c9a2be69b1a9a692
Patch Set 1 : [ash-md] Disables the Wi-Fi and Cellular toggles until the state changes #Patch Set 2 : [ash-md] Disables the Wi-Fi and Cellular toggles until the state changes (Bluetooth) #Patch Set 3 : [ash-md] Disables the Wi-Fi and Cellular toggles until the state changes (rebase) #
Total comments: 4
Patch Set 4 : [ash-md] Disables the Wi-Fi and Cellular toggles until the state changes (comments) #
Messages
Total messages: 37 (30 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 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: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@, can you please take a look? Thanks!
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a couple of comments, and my apologies for the delay. https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:456: // Disable the toggle to prevent bouncing. It will get re-enabled when nit: Perhaps be a bit more specific with what is meant by 'bouncing' - maybe say something like "In the event of multiple clicks within a short period of time, helps to prevent a toggle button appearance becoming inconsistent with the async operation of enabling/disabling Bluetooth" or similar but more condensed? Ditto for the comment in network_list_md https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.cc:99: toggle_->SetIsOn(enabled, true); For Bluetooth you pass in false for the animation parameter here just after re-enabling the toggle - is that inconsistency intentional?
On 2017/02/28 16:30:03, tdanderson (slow until Mar 8) wrote: > LGTM with a couple of comments, and my apologies for the delay. > > https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... > File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right): > > https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... > ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:456: // Disable the > toggle to prevent bouncing. It will get re-enabled when > nit: Perhaps be a bit more specific with what is meant by 'bouncing' - maybe say > something like "In the event of multiple clicks within a short period of time, > helps to prevent a toggle button appearance becoming inconsistent with the async > operation of enabling/disabling Bluetooth" or similar but more condensed? > > Ditto for the comment in network_list_md > > https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... > File ash/common/system/chromeos/network/network_list_md.cc (right): > > https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... > ash/common/system/chromeos/network/network_list_md.cc:99: > toggle_->SetIsOn(enabled, true); > For Bluetooth you pass in false for the animation parameter here just after > re-enabling the toggle - is that inconsistency intentional? Friendly ping on this. We should land and target this for an m-58 merge.
On 2017/03/07 15:55:17, tdanderson (slow until Mar 8) wrote: > On 2017/02/28 16:30:03, tdanderson (slow until Mar 8) wrote: > > LGTM with a couple of comments, and my apologies for the delay. > > > > > https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... > > File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right): > > > > > https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... > > ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:456: // Disable the > > toggle to prevent bouncing. It will get re-enabled when > > nit: Perhaps be a bit more specific with what is meant by 'bouncing' - maybe > say > > something like "In the event of multiple clicks within a short period of time, > > helps to prevent a toggle button appearance becoming inconsistent with the > async > > operation of enabling/disabling Bluetooth" or similar but more condensed? > > > > Ditto for the comment in network_list_md > > > > > https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... > > File ash/common/system/chromeos/network/network_list_md.cc (right): > > > > > https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... > > ash/common/system/chromeos/network/network_list_md.cc:99: > > toggle_->SetIsOn(enabled, true); > > For Bluetooth you pass in false for the animation parameter here just after > > re-enabling the toggle - is that inconsistency intentional? > > Friendly ping on this. We should land and target this for an m-58 merge. Yes, need to see if this is really effective on device. I had some doubts about the approach.
I have tested this a bit more on device. I have rolled back the changes to Bluetooth page - they seem to be ineffective although it is harder to reproduce than with the mobile switch requiring a couple connected BT devices. I have also confirmed that for the mobile switch the change is in fact effective. PTAL. https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:456: // Disable the toggle to prevent bouncing. It will get re-enabled when On 2017/02/28 16:30:03, tdanderson wrote: > nit: Perhaps be a bit more specific with what is meant by 'bouncing' - maybe say > something like "In the event of multiple clicks within a short period of time, > helps to prevent a toggle button appearance becoming inconsistent with the async > operation of enabling/disabling Bluetooth" or similar but more condensed? > > Ditto for the comment in network_list_md Done (in network_list_md.cc). https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... File ash/common/system/chromeos/network/network_list_md.cc (right): https://codereview.chromium.org/2702493002/diff/80001/ash/common/system/chrom... ash/common/system/chromeos/network/network_list_md.cc:99: toggle_->SetIsOn(enabled, true); On 2017/02/28 16:30:03, tdanderson wrote: > For Bluetooth you pass in false for the animation parameter here just after > re-enabling the toggle - is that inconsistency intentional? Done (no it was not intentional although I don't know how you can execute enabling BT without also hiding the detailed system menu view at the same time).
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] Disables the Wi-Fi and Cellular toggles until the state changes
BUG=687922
==========
to
==========
[ash-md] Disables the Wi-Fi and Cellular toggles until the state changes
BUG=687922
TEST=Visual
On a mobile-equipped Chromebook (e.g. link with 3G)
Try to toggle Mobile toggle off and immediately on
Expected: The toggle cannot be re-enabled immediately.
When it can finally be re-enabled, it stays enabled.
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by 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/2702493002/#ps100001 (title: "[ash-md] Disables the Wi-Fi and Cellular toggles until the state changes (comments)")
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": 100001, "attempt_start_ts": 1488940427496140,
"parent_rev": "2aeb58041a23a412147fedb6c8fea08a8eff5320", "commit_rev":
"6a8e668da879ad1fc3b442a8c9a2be69b1a9a692"}
Message was sent while issue was closed.
Description was changed from
==========
[ash-md] Disables the Wi-Fi and Cellular toggles until the state changes
BUG=687922
TEST=Visual
On a mobile-equipped Chromebook (e.g. link with 3G)
Try to toggle Mobile toggle off and immediately on
Expected: The toggle cannot be re-enabled immediately.
When it can finally be re-enabled, it stays enabled.
==========
to
==========
[ash-md] Disables the Wi-Fi and Cellular toggles until the state changes
BUG=687922
TEST=Visual
On a mobile-equipped Chromebook (e.g. link with 3G)
Try to toggle Mobile toggle off and immediately on
Expected: The toggle cannot be re-enabled immediately.
When it can finally be re-enabled, it stays enabled.
Review-Url: https://codereview.chromium.org/2702493002
Cr-Commit-Position: refs/heads/master@{#455357}
Committed:
https://chromium.googlesource.com/chromium/src/+/6a8e668da879ad1fc3b442a8c9a2...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/6a8e668da879ad1fc3b442a8c9a2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
