Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2544073002:
Resuming from suspend should stop backlights forced off if they are (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. |
DescriptionResuming from suspend should stop backlights forced off if they are
BUG=670402
TEST=manual device test, also add test coverage
Committed: https://crrev.com/30955abfe3214c8fb4258b05a0db86848f1c592f
Cr-Commit-Position: refs/heads/master@{#435854}
Patch Set 1 #Patch Set 2 : add test coverage #
Total comments: 6
Patch Set 3 : nits #
Messages
Total messages: 15 (7 generated)
warx@chromium.org changed reviewers: + derat@chromium.org
Hi Daniel, please take a look, thanks!
Description was changed from ========== Resuming from suspend should stop backlights forced off if they are BUG=670402 ========== to ========== Resuming from suspend should stop backlights forced off if they are BUG=670402 TEST=manual device test, also add test coverage ==========
generally lgtm with a few comments https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:97: // that woke the system. If backlights are forced off, stop forcing off nit: i'd move this "If ..." part down just before the "if (down && backlights_forced_off_)" line, since that's what it's referring to. https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:218: // |tick_clock| owned by |tablet_controller_|. nit: remove this comment; passing an std::unique_ptr makes it clear that ownership is being transferred. you might want to consider just making this a member and passing it in the base class's SetUp method, since it's needed by multiple tests now. https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:249: // |tick_clock| owned by |tablet_controller_|. same here https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:259: // Because of backlights forced off, resuming system will not resume nit: s/will not resume/will not restore/
https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:104: SetBacklightsForcedOff(false); Hi, just want to mention here, the long pressed shutdown white animation is missing here because the following StartShutdownTimer is ignored. I need to think of fixing it. It can be fixed within this cl or separately.
https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... ash/system/chromeos/power/tablet_power_button_controller.cc:104: SetBacklightsForcedOff(false); On 2016/12/02 01:06:38, Qiang (Joe) Xu wrote: > Hi, just want to mention here, the long pressed shutdown white animation is > missing here because the following StartShutdownTimer is ignored. I need to > think of fixing it. It can be fixed within this cl or separately. are you just saying power button down events that wake the system from suspend won't trigger shutdown? i think that that's the correct behavior, actually. the non-tablet power button controller does something similar: we don't lock or shut down when the power button is pressed while the screen is off.
On 2016/12/02 01:10:26, Daniel Erat wrote: > https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... > File ash/system/chromeos/power/tablet_power_button_controller.cc (right): > > https://codereview.chromium.org/2544073002/diff/20001/ash/system/chromeos/pow... > ash/system/chromeos/power/tablet_power_button_controller.cc:104: > SetBacklightsForcedOff(false); > On 2016/12/02 01:06:38, Qiang (Joe) Xu wrote: > > Hi, just want to mention here, the long pressed shutdown white animation is > > missing here because the following StartShutdownTimer is ignored. I need to > > think of fixing it. It can be fixed within this cl or separately. > > are you just saying power button down events that wake the system from suspend > won't trigger shutdown? i think that that's the correct behavior, actually. the > non-tablet power button controller does something similar: we don't lock or shut > down when the power button is pressed while the screen is off. No, holding power button down when screen is off should trigger shutdown. That is the new design and agrees with the android phone. Because pressing firstly will turn on the display, so user can notice system will do shutdown (because of white animation on UI). On screen off, press (trigger screen on) and hold, while hold > 1s, system starts white animation, and any release here will cancel the animation and screen stays turned on. That is right in this code when screen is off but not suspended. --> That is what I mean.
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org Link to the patchset: https://codereview.chromium.org/2544073002/#ps40001 (title: "nits")
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": 40001, "attempt_start_ts": 1480650654739280,
"parent_rev": "3e6ae323d71ab67f8cb16e2383ac9f055af56f86", "commit_rev":
"5a057a805c972d333eec8dfb3ee372a0e0845c09"}
Message was sent while issue was closed.
Description was changed from ========== Resuming from suspend should stop backlights forced off if they are BUG=670402 TEST=manual device test, also add test coverage ========== to ========== Resuming from suspend should stop backlights forced off if they are BUG=670402 TEST=manual device test, also add test coverage ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Resuming from suspend should stop backlights forced off if they are BUG=670402 TEST=manual device test, also add test coverage ========== to ========== Resuming from suspend should stop backlights forced off if they are BUG=670402 TEST=manual device test, also add test coverage Committed: https://crrev.com/30955abfe3214c8fb4258b05a0db86848f1c592f Cr-Commit-Position: refs/heads/master@{#435854} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/30955abfe3214c8fb4258b05a0db86848f1c592f Cr-Commit-Position: refs/heads/master@{#435854} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
