Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2664683002:
ash: Drop the immediately following power button released induced forcing off request (Closed)
|
Created:
3 years, 10 months ago by Qiang(Joe) Xu Modified:
3 years, 10 months ago Reviewers:
Daniel Erat CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org, derat+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionash: Drop the immediately following power button released induced forcing off request
Changes:
Record last power button released time and drop the immediately following (within 500ms) power button released induced forcing off request.
BUG=675291
TEST=device test: when screen is on, pressing/releasing side power
button quickly, will see screen is off for only the first time. Behavior
doesn't change for other cases.
Also, test coverage
TabletPowerButtonControllerTest.IgnoreRepeatedPowerButtonReleases is added.
Review-Url: https://codereview.chromium.org/2664683002
Cr-Commit-Position: refs/heads/master@{#447081}
Committed: https://chromium.googlesource.com/chromium/src/+/e735c591f218ba5c5a315fb5973bacf3d41e25b8
Patch Set 1 #Patch Set 2 : better comments #
Total comments: 15
Patch Set 3 : comments from ps2 #
Messages
Total messages: 19 (13 generated)
Description was changed from ========== ash: Drop the very adjacent power button released induced forcing off request BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ========== to ========== ash: Drop the very adjacent power button released induced forcing off request BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ==========
warx@chromium.org changed reviewers: + derat@chromium.org
The CQ bit was checked by warx@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: Drop the very adjacent power button released induced forcing off request BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ========== to ========== ash: Drop the very adjacent power button released induced forcing off request Changes: Recording last power button released time and dropping the very adjacent power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ==========
Description was changed from ========== ash: Drop the very adjacent power button released induced forcing off request Changes: Recording last power button released time and dropping the very adjacent power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ========== to ========== ash: Drop the very adjacent power button released induced forcing off request Changes: Record last power button released time and drop the very adjacent power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ==========
Description was changed from ========== ash: Drop the very adjacent power button released induced forcing off request Changes: Record last power button released time and drop the very adjacent power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ========== to ========== ash: Drop the very adjacent power button released induced forcing off request Changes: Record last power button released time and drop the very adjacent (within 500ms) power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ==========
Hi Daniel, ptal, thanks!
thanks for the tests! i mostly just have naming and comment suggestions. https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:40: constexpr int kIgnoreReleasedAfterLastOneMs = 500; nit: kIgnoreRepeatedButtonUpMs or something similar? i'd also change the comment to something like: // Ignore button-up events occurring within this many milliseconds // of the previous button-up event. This prevents us from falling // behind if the power button is pressed repeatedly. https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:80: last_released_time_(base::TimeTicks()), i don't think you need to initialize this member and last_resume_time_ explicitly; they should automatically be constructed with the zero value. https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:131: might be a little cleaner to do something like this: const base::TimeTicks previous_up_time = last_button_up_time_; last_button_up_time_ = tick_clock_->NowTicks(); if (timestamp - previous_up_time <= ...) { shutdown_timer_.Stop(); return; } then the member is only updated by a single statement, which feels a bit simpler. https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:132: // Drop the very adjacent power button released event for forcing off. maybe: // Ignore the event if it comes too soon after the last one. ? https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.h:121: base::TimeTicks last_released_time_; last_button_up_time_ or last_power_button_up_time_ to match the terminology already used in the class? https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.h:121: base::TimeTicks last_released_time_; last_button_up_time_ or last_power_button_up_time_ to match the terminology already used in the class? https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:442: // Tests that the very adjacent power button released for forcing off needs to // Tests that repeated power button releases are ignored (crbug...). ? https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:464: // since this very adjacent forcing off request needs to be dropped. hmm. "adjacent" usually refers more to physical distance than time distance, i think. maybe something like "soon after" or "quickly following" or "immediately following"?
Description was changed from ========== ash: Drop the very adjacent power button released induced forcing off request Changes: Record last power button released time and drop the very adjacent (within 500ms) power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ========== to ========== ash: Drop the immediately following power button released induced forcing off request Changes: Record last power button released time and drop the immediately following (within 500ms) power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ==========
Patchset #3 (id:2) has been deleted
Description was changed from ========== ash: Drop the immediately following power button released induced forcing off request Changes: Record last power button released time and drop the immediately following (within 500ms) power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.DropVeryAdjacentForcingOff is added. ========== to ========== ash: Drop the immediately following power button released induced forcing off request Changes: Record last power button released time and drop the immediately following (within 500ms) power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.IgnoreRepeatedPowerButtonReleases is added. ==========
Hi Daniel, new patch uploaded, ptal, thanks! https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:40: constexpr int kIgnoreReleasedAfterLastOneMs = 500; On 2017/01/30 17:55:00, Daniel Erat wrote: > nit: kIgnoreRepeatedButtonUpMs or something similar? > > i'd also change the comment to something like: > > // Ignore button-up events occurring within this many milliseconds > // of the previous button-up event. This prevents us from falling > // behind if the power button is pressed repeatedly. done, thanks! https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:80: last_released_time_(base::TimeTicks()), On 2017/01/30 17:55:00, Daniel Erat wrote: > i don't think you need to initialize this member and last_resume_time_ > explicitly; they should automatically be constructed with the zero value. Done. https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:131: On 2017/01/30 17:55:00, Daniel Erat wrote: > might be a little cleaner to do something like this: > > const base::TimeTicks previous_up_time = last_button_up_time_; > last_button_up_time_ = tick_clock_->NowTicks(); > > if (timestamp - previous_up_time <= ...) { > shutdown_timer_.Stop(); > return; > } > > then the member is only updated by a single statement, which feels a bit > simpler. Done. https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:132: // Drop the very adjacent power button released event for forcing off. On 2017/01/30 17:55:00, Daniel Erat wrote: > maybe: > > // Ignore the event if it comes too soon after the last one. > > ? Done. https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.h:121: base::TimeTicks last_released_time_; On 2017/01/30 17:55:00, Daniel Erat wrote: > last_button_up_time_ or last_power_button_up_time_ to match the terminology > already used in the class? Changed to last_button_up_time_ to match force_off_on_button_up_, thanks! https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:442: // Tests that the very adjacent power button released for forcing off needs to On 2017/01/30 17:55:00, Daniel Erat wrote: > // Tests that repeated power button releases are ignored (crbug...). > > ? Done. https://codereview.chromium.org/2664683002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:464: // since this very adjacent forcing off request needs to be dropped. On 2017/01/30 17:55:00, Daniel Erat wrote: > hmm. "adjacent" usually refers more to physical distance than time distance, i > think. maybe something like "soon after" or "quickly following" or "immediately > following"? done by changed to "immediately following"
lgtm
The CQ bit was checked by warx@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": 50001, "attempt_start_ts": 1485811115069380,
"parent_rev": "202539e475b6398e7181706d44b43a22f0711227", "commit_rev":
"e735c591f218ba5c5a315fb5973bacf3d41e25b8"}
Message was sent while issue was closed.
Description was changed from ========== ash: Drop the immediately following power button released induced forcing off request Changes: Record last power button released time and drop the immediately following (within 500ms) power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.IgnoreRepeatedPowerButtonReleases is added. ========== to ========== ash: Drop the immediately following power button released induced forcing off request Changes: Record last power button released time and drop the immediately following (within 500ms) power button released induced forcing off request. BUG=675291 TEST=device test: when screen is on, pressing/releasing side power button quickly, will see screen is off for only the first time. Behavior doesn't change for other cases. Also, test coverage TabletPowerButtonControllerTest.IgnoreRepeatedPowerButtonReleases is added. Review-Url: https://codereview.chromium.org/2664683002 Cr-Commit-Position: refs/heads/master@{#447081} Committed: https://chromium.googlesource.com/chromium/src/+/e735c591f218ba5c5a315fb5973b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:50001) as https://chromium.googlesource.com/chromium/src/+/e735c591f218ba5c5a315fb5973b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
