Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2773013003:
Remember User locked rotation (Closed)
DescriptionRemember User locked rotation
* Save when the toggle state changes, as that's
only time the value changes.
* implement reverse lock.
BUG=695281, 683456
TEST=manual, covered by unit test
Review-Url: https://codereview.chromium.org/2773013003
Cr-Commit-Position: refs/heads/master@{#459502}
Committed: https://chromium.googlesource.com/chromium/src/+/28bd8173b80ae1fe5e22875580051664cd97363d
Patch Set 1 : Remember User locked rotation #
Total comments: 3
Patch Set 2 : . #
Total comments: 7
Patch Set 3 : . #
Messages
Total messages: 31 (23 generated)
The CQ bit was checked by oshima@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Remember User locked rotation Resolve OrientationLock BUG= patch from issue 2768083003 at patchset 1 (http://crrev.com/2768083003#ps1) ========== to ========== Remember User locked rotation * save when the toggle state changes * load BUG=695281, 683456 TEST=manual, covered by unit test ==========
Description was changed from ========== Remember User locked rotation * save when the toggle state changes * load BUG=695281, 683456 TEST=manual, covered by unit test ========== to ========== Remember User locked rotation * Save when the toggle state changes, as that's only time the value changes. * implement reverse lock. BUG=695281, 683456 TEST=manual, covered by unit test ==========
oshima@chromium.org changed reviewers: + jonross@chromium.org
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) 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...
https://codereview.chromium.org/2773013003/diff/60001/ash/display/screen_orie... File ash/display/screen_orientation_controller_chromeos.cc (right): https://codereview.chromium.org/2773013003/diff/60001/ash/display/screen_orie... ash/display/screen_orientation_controller_chromeos.cc:323: observer.OnUserRotationLockChanged(); Do we want to send this if there was no rotation lock state change? Such as no lock at all, and entering maximize mode leads to no lock?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2773013003/diff/60001/ash/display/screen_orie... File ash/display/screen_orientation_controller_chromeos.cc (right): https://codereview.chromium.org/2773013003/diff/60001/ash/display/screen_orie... ash/display/screen_orientation_controller_chromeos.cc:323: observer.OnUserRotationLockChanged(); On 2017/03/24 15:21:04, jonross wrote: > Do we want to send this if there was no rotation lock state change? > > Such as no lock at all, and entering maximize mode leads to no lock? no lock is still one of states, so I consider this is still a state change. In clamshell, it's disabled, then it becomes one of any, portrait, secondary landscape etc upon entering the touch view mode. Which means I we should do the same when exiting, so thanks for pointing this out.
https://codereview.chromium.org/2773013003/diff/60001/ash/display/screen_orie... File ash/display/screen_orientation_controller_chromeos.cc (right): https://codereview.chromium.org/2773013003/diff/60001/ash/display/screen_orie... ash/display/screen_orientation_controller_chromeos.cc:323: observer.OnUserRotationLockChanged(); On 2017/03/24 16:33:43, oshima wrote: > On 2017/03/24 15:21:04, jonross wrote: > > Do we want to send this if there was no rotation lock state change? > > > > Such as no lock at all, and entering maximize mode leads to no lock? > > no lock is still one of states, so I consider this is still a state change. > In clamshell, it's disabled, then it becomes one of any, portrait, secondary > landscape etc > upon entering the touch view mode. > > Which means I we should do the same when exiting, so thanks for pointing this > out. Okay that sgtm https://codereview.chromium.org/2773013003/diff/80001/ash/display/screen_orie... File ash/display/screen_orientation_controller_chromeos.cc (right): https://codereview.chromium.org/2773013003/diff/80001/ash/display/screen_orie... ash/display/screen_orientation_controller_chromeos.cc:321: if (display::Display::HasInternalDisplay()) { Nit: early exit vs blocking off end of method https://codereview.chromium.org/2773013003/diff/80001/ash/display/screen_orie... ash/display/screen_orientation_controller_chromeos.cc:331: if (display::Display::HasInternalDisplay()) { Nit: early exit vs blocking off end of method https://codereview.chromium.org/2773013003/diff/80001/ash/system/chromeos/rot... File ash/system/chromeos/rotation/tray_rotation_lock.cc (right): https://codereview.chromium.org/2773013003/diff/80001/ash/system/chromeos/rot... ash/system/chromeos/rotation/tray_rotation_lock.cc:167: StopObservingRotation(); Are we sending state change notifications during shutdown? That's slightly concerning.
The CQ bit was checked by oshima@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/2773013003/diff/80001/ash/display/screen_orie... File ash/display/screen_orientation_controller_chromeos.cc (right): https://codereview.chromium.org/2773013003/diff/80001/ash/display/screen_orie... ash/display/screen_orientation_controller_chromeos.cc:321: if (display::Display::HasInternalDisplay()) { On 2017/03/24 16:46:49, jonross wrote: > Nit: early exit vs blocking off end of method Done. https://codereview.chromium.org/2773013003/diff/80001/ash/display/screen_orie... ash/display/screen_orientation_controller_chromeos.cc:331: if (display::Display::HasInternalDisplay()) { On 2017/03/24 16:46:49, jonross wrote: > Nit: early exit vs blocking off end of method Done. https://codereview.chromium.org/2773013003/diff/80001/ash/system/chromeos/rot... File ash/system/chromeos/rotation/tray_rotation_lock.cc (right): https://codereview.chromium.org/2773013003/diff/80001/ash/system/chromeos/rot... ash/system/chromeos/rotation/tray_rotation_lock.cc:167: StopObservingRotation(); On 2017/03/24 16:46:49, jonross wrote: > Are we sending state change notifications during shutdown? That's slightly > concerning. This was for the test which wasn't following the normal destroction flow. I changed the test instead.
The CQ bit was checked by oshima@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...
LGTM https://codereview.chromium.org/2773013003/diff/80001/ash/system/chromeos/rot... File ash/system/chromeos/rotation/tray_rotation_lock.cc (right): https://codereview.chromium.org/2773013003/diff/80001/ash/system/chromeos/rot... ash/system/chromeos/rotation/tray_rotation_lock.cc:167: StopObservingRotation(); On 2017/03/24 17:19:35, oshima wrote: > On 2017/03/24 16:46:49, jonross wrote: > > Are we sending state change notifications during shutdown? That's slightly > > concerning. > > This was for the test which wasn't following the normal destroction flow. I > changed the test instead. Acknowledged.
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 oshima@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": 90001, "attempt_start_ts": 1490381522465050,
"parent_rev": "49f6220a80ff68d06c6c1c5179d0e494e0b8ad00", "commit_rev":
"28bd8173b80ae1fe5e22875580051664cd97363d"}
Message was sent while issue was closed.
Description was changed from ========== Remember User locked rotation * Save when the toggle state changes, as that's only time the value changes. * implement reverse lock. BUG=695281, 683456 TEST=manual, covered by unit test ========== to ========== Remember User locked rotation * Save when the toggle state changes, as that's only time the value changes. * implement reverse lock. BUG=695281, 683456 TEST=manual, covered by unit test Review-Url: https://codereview.chromium.org/2773013003 Cr-Commit-Position: refs/heads/master@{#459502} Committed: https://chromium.googlesource.com/chromium/src/+/28bd8173b80ae1fe5e2287558005... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:90001) as https://chromium.googlesource.com/chromium/src/+/28bd8173b80ae1fe5e2287558005... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
