Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2546303002:
ash: Start shutdown timer on post-resume power button press (Closed)
|
Created:
4 years ago by Qiang(Joe) Xu Modified:
4 years 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: Start shutdown timer on post-resume power button press
Changes:
Start shutdown timer on convertible Chromebooks when the power button is pressed just after resume. Continue avoiding forcing the display off in response to the button being released in this case, though.
BUG=633304
TEST=device test, and test coverage
Committed: https://crrev.com/d3dd2cafa37a9985dc4907ee364d9229b6c0006a
Cr-Commit-Position: refs/heads/master@{#436447}
Patch Set 1 #
Total comments: 12
Patch Set 2 : based on comments #
Total comments: 6
Patch Set 3 : nits #
Messages
Total messages: 19 (10 generated)
Description was changed from ========== Should ignore forcing off after resume instead of power button event BUG=633304 ========== to ========== Should ignore forcing off after resume instead of power button event Changes include: (1) ignore forcing off after resume instead of power button event (2) fix "idle-screen-off, suspend" and then press power button, alert_screen_on is not sent. BUG=633304 TEST=device test, and existing automated tests ==========
warx@chromium.org changed reviewers: + derat@chromium.org
Hi Daniel, please take a look, thanks!
https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.cc:30: constexpr int kDurationPowerButtonAsWakeSourceMs = 2000; this name and comment don't make that much sense to me now. "wake source" has a specific meaning in the kernel and this doesn't seem to align with it. the old name and comment made more sense to me. why do they need to be changed? https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.cc:51: alert_screen_off ? A11Y_ALERT_SCREEN_OFF : A11Y_ALERT_SCREEN_ON); is there any way to test that alerts are sent at the correct time (e.g. a stub delegate that logs the alerts it would emit)? https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.cc:114: SendA11yScreenAlert(false); why do you need to call this here? won't this make us announce that the screen is on even though we didn't announce that it was off? https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.cc:127: ignore_forcing_off_ = false; how about just setting this to true/false in the down handler? it feels a bit cleaner to only set it in one place. https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.h:115: bool ignore_forcing_off_; this variable name is pretty vague. how about inverting its logic and naming it |force_off_on_button_up_|, and then setting it to true in the c'tor? https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:270: // are forced off. it looks like you're only updating comments. can you add a test for the updated behavior?
Description was changed from ========== Should ignore forcing off after resume instead of power button event Changes include: (1) ignore forcing off after resume instead of power button event (2) fix "idle-screen-off, suspend" and then press power button, alert_screen_on is not sent. BUG=633304 TEST=device test, and existing automated tests ========== to ========== Should ignore forcing off after resume instead of power button event BUG=633304 TEST=device test, and existing automated tests ==========
Description was changed from ========== Should ignore forcing off after resume instead of power button event BUG=633304 TEST=device test, and existing automated tests ========== to ========== Should ignore forcing off after resume instead of power button event BUG=633304 TEST=device test, and test coverage ==========
Patchset #2 (id:20001) has been deleted
Hi Daniel, here is the reply and new patch, thanks! https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.cc:30: constexpr int kDurationPowerButtonAsWakeSourceMs = 2000; On 2016/12/05 17:25:59, Daniel Erat wrote: > this name and comment don't make that much sense to me now. "wake source" has a > specific meaning in the kernel and this doesn't seem to align with it. > > the old name and comment made more sense to me. why do they need to be changed? Because I think the code now is not ignoring the entire OnPowerButtonEvent, it needs special processing for resuming case. However, "ignore power button event" seems fine to me because we ignore the forcing off on power button up and we have documents on the code. So I restore the naming here. https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.cc:51: alert_screen_off ? A11Y_ALERT_SCREEN_OFF : A11Y_ALERT_SCREEN_ON); On 2016/12/05 17:26:00, Daniel Erat wrote: > is there any way to test that alerts are sent at the correct time (e.g. a stub > delegate that logs the alerts it would emit)? mm.. I don't find it. From the CL I found, there is also no test there. https://codereview.chromium.org/2488373002/. https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.cc:114: SendA11yScreenAlert(false); On 2016/12/05 17:26:00, Daniel Erat wrote: > why do you need to call this here? won't this make us announce that the screen > is on even though we didn't announce that it was off? If we want "tablet power button event" always trigger the alert if there is a screen on-off state change, this line is needed. But you are right, if the a11y alert is bonded to SetBacklightsForcedOff, then this line is not needed otherwise the alert is not paired. I start to believe we don't need it now since the a11y alert is introduced with the rule that it is bound to "forcing off". It is better to keep this rule right now. https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.cc:127: ignore_forcing_off_ = false; On 2016/12/05 17:26:00, Daniel Erat wrote: > how about just setting this to true/false in the down handler? it feels a bit > cleaner to only set it in one place. Done. https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller.h:115: bool ignore_forcing_off_; On 2016/12/05 17:26:00, Daniel Erat wrote: > this variable name is pretty vague. how about inverting its logic and naming it > |force_off_on_button_up_|, and then setting it to true in the c'tor? Done. https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2546303002/diff/1/ash/system/chromeos/power/t... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:270: // are forced off. On 2016/12/05 17:26:00, Daniel Erat wrote: > it looks like you're only updating comments. can you add a test for the updated > behavior? Done. https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (left): https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:124: screen_off_when_power_button_down_ = false; feel this is not needed, as it is always updated in power button down
looks okay with a few comments, but please make sure that it's obvious from the first line of the CL description what's being changed and that the rest of the description provides more detail. this really helps when people are looking through commit logs to find the sources of regressions. i'd recommend something like: ---- ash: Start shutdown timer on post-resume power button press. Start the shutdown timer on convertible Chromebooks when the power button is pressed just after resume. Continue avoiding forcing the display off in response to the button being released in this case, though. https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (left): https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:124: screen_off_when_power_button_down_ = false; On 2016/12/05 19:33:59, Qiang (Joe) Xu wrote: > feel this is not needed, as it is always updated in power button down yep, i agree https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:100: // baclight has been turned back on before seeing the power button events nit: s/baclight/backlight/ https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:101: // that woke the system. Ignore forcing off display just after resuming to nit: s/Ignore/Avoid/
Description was changed from ========== Should ignore forcing off after resume instead of power button event BUG=633304 TEST=device test, and test coverage ========== to ========== ash: Start shutdown timer on post-resume power button press Changes: Start shutdown timer on convertible Chromebooks when the power button is pressed just after resume. Continue avoiding forcing the display off in response to the button being released in this case, though. BUG=633304 TEST=device test, and test coverage ==========
Hi Daniel, thanks for the suggestions, new patch updated. https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:100: // baclight has been turned back on before seeing the power button events On 2016/12/05 22:08:09, Daniel Erat wrote: > nit: s/baclight/backlight/ Done. https://codereview.chromium.org/2546303002/diff/40001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:101: // that woke the system. Ignore forcing off display just after resuming to On 2016/12/05 22:08:09, Daniel Erat wrote: > nit: s/Ignore/Avoid/ Done.
lgtm thanks!
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": 60001, "attempt_start_ts": 1480977698456220,
"parent_rev": "76bb2336e0fad3c83f3f38dd9d829c916d792f1c", "commit_rev":
"9d6250ffc97cf9f9f345358053a34fb63fe110ff"}
Message was sent while issue was closed.
Description was changed from ========== ash: Start shutdown timer on post-resume power button press Changes: Start shutdown timer on convertible Chromebooks when the power button is pressed just after resume. Continue avoiding forcing the display off in response to the button being released in this case, though. BUG=633304 TEST=device test, and test coverage ========== to ========== ash: Start shutdown timer on post-resume power button press Changes: Start shutdown timer on convertible Chromebooks when the power button is pressed just after resume. Continue avoiding forcing the display off in response to the button being released in this case, though. BUG=633304 TEST=device test, and test coverage ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== ash: Start shutdown timer on post-resume power button press Changes: Start shutdown timer on convertible Chromebooks when the power button is pressed just after resume. Continue avoiding forcing the display off in response to the button being released in this case, though. BUG=633304 TEST=device test, and test coverage ========== to ========== ash: Start shutdown timer on post-resume power button press Changes: Start shutdown timer on convertible Chromebooks when the power button is pressed just after resume. Continue avoiding forcing the display off in response to the button being released in this case, though. BUG=633304 TEST=device test, and test coverage Committed: https://crrev.com/d3dd2cafa37a9985dc4907ee364d9229b6c0006a Cr-Commit-Position: refs/heads/master@{#436447} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d3dd2cafa37a9985dc4907ee364d9229b6c0006a Cr-Commit-Position: refs/heads/master@{#436447} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
