|
|
Created:
4 years, 1 month ago by Qiang(Joe) Xu Modified:
4 years, 1 month ago Reviewers:
Daniel Erat CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTablet-like power button behavior on Convertible/Tablet ChromeOS devices
Changes:
device consistency: convertible/tablet chromeos devices follow the behavior described in go/touchview-power convertible behavior; other devices follow clamshell behavior.
BUG=633304
TEST=manually test; add unitttest
Committed: https://crrev.com/72135ade1aacb9f9fcfe61f8a704567a98cc3178
Cr-Commit-Position: refs/heads/master@{#431931}
Patch Set 1 #
Total comments: 26
Patch Set 2 : based on Daniel's comments #
Total comments: 40
Patch Set 3 : cr based on comments #
Total comments: 4
Patch Set 4 : extract TabletPowerButtonController #
Total comments: 4
Patch Set 5 : nits on tests #
Total comments: 36
Patch Set 6 : rebase #Patch Set 7 : cr and additional changes #
Total comments: 18
Patch Set 8 : based on comments #
Total comments: 20
Patch Set 9 : rebase #Patch Set 10 : cr based on comments #
Total comments: 23
Patch Set 11 : comments from ps10 #
Total comments: 1
Patch Set 12 : nits #Messages
Total messages: 48 (28 generated)
Description was changed from ========== Tablet-like power button behavior on Convertible/Tablet ChromeOS devices BUG=633304 TEST=manually test; add unitttest ========== to ========== Tablet-like power button behavior on Convertible/Tablet ChromeOS devices Changes: device consistency: convertible/tablet chromeos devices follow the behavior described in go/touchview-power convertible behavior; other devices follow clamshell behavior. BUG=633304 TEST=manually test; add unitttest ==========
warx@chromium.org changed reviewers: + derat@chromium.org
Hi Daniel, I would like to send this out and please see if it is good enough for starting the review. On veyron_minnie, touchscreen doesn't wake device from S3, so this CL works fine on veyron_minnie. I would also try to add more test coverage. Thanks!
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
some initial comments. i haven't looked at the tests https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:73: base::MakeUnique<TabletPowerStateDelegateChromeos>(); i don't understand the reason for this delegate. delegates are usually used to do dependency injection -- for example, if ash/ weren't able to depend on chromeos/, then ash would define an abstract DelegateInterface, and then chrome/ (which can depend on both ash/ and chromeos/) could define an implementation of DelegateInterface that calls into chromeos/ and then instantiate it and pass it into ash::PowerButtonController.[1] you don't have that dependency problem here, though, and the delegate implementation actually lives inside of ash/ and is directly instantiated by this class. i think that you would have less code, and simpler code, if you got rid of the delegate and just made PowerButtonController call the PowerManagerClient methods directly. 1. note that we're moving away from this delegate-implemented-by-chrome pattern due to mus+ash, though. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:76: base::ThreadTaskRunnerHandle::Get()->PostTask( what's the reason for posting this instead of just calling GetBacklightsForcedOff directly within the c'tor? note that the method usually makes an asynchronous method call to powerd. if the stub implementation runs the callback synchronously instead, please feel free to update it to be async so it matches the real implementation. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:106: if (!has_legacy_power_button_ && !MaybeTakeScreenshot(down) && instead of calling this helper method twice, why not just remove it and inline it above this? bool should_take_screenshot = down && volume_down_pressed_ && WmShell::Get() ->maximize_mode_controller() ->IsMaximizeModeWindowManagerEnabled(); https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:258: pressed_on_screen_off_ = true; it seems a bit safer to set this unconditionally here instead of only setting it to true: power_button_down_while_screen_off_ = brightness_is_zero_; if (backlights_forced_off_) SetBacklightsForcedOff(false); it would also be better to set this from OnPowerButtonEvent instead of OnTabletPowerButtonEvent so it'll also have the correct state when we aren't in tablet mode. (if backlights_forced_off_ is true, won't brightness_is_zero_ also be true?) https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:302: SetBacklightsForcedOff(false); i don't understand why this call is needed. the backlights are already forced off in this case. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:306: tablet_pre_start_shutdown_animation_timer_.Stop(); you don't need this call to Stop(); Start() already restarts the timer if it's already running. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:328: void PowerButtonController::WakeOnLaptopMode() { using the word "wake" here seems a bit inaccurate; it usually refers to returning from the S3 (suspended) power state. if this method becomes shorter (see below), i'd just inline it within the places that call it. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:333: !maximize_mode_controller->IsMaximizeModeWindowManagerEnabled() && i find all of these references to MaximizeModeController a bit confusing. how about extracting them to two private helper methods? bool IsTabletModeActive() const; bool IsTabletModeSupported() const; i think that'll make the rest of the code much clearer. this would turn into IsTabletModeSupported() && !IsTabletModeActive(), for example. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:334: (backlights_forced_off_ || brightness_is_zero_)) { i don't understand the brightness_is_zero_ check here. if the backlights aren't forced off but the brightness is zero (e.g. because the user reduced it to 0%), why are you calling this? calling SetBacklightsForcedOff(false) won't do anything if they already aren't forced off. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.h:117: // A helper method to be called on convertible/tablet device. nit: // Called by OnPowerButtonEvent on convertible/tablet devices. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.h:151: bool MaybeTakeScreenshot(bool power_button_pressed); "MaybeTakeScreenshot" suggests that this method might take a screenshot when called, but it looks like it's actually just checking whether a screenshot should be taken (without any side effects). but i think you should just remove this and inline the code -- see the comment in the .cc file. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.h:188: bool pressed_on_screen_off_; this name is a bit vague. how about "power_button_down_while_screen_off_"?
Hi Daniel, patch uploaded, please take a look, thanks. New changes are based on comments. I didn't add more tests on this patch. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:73: base::MakeUnique<TabletPowerStateDelegateChromeos>(); On 2016/11/03 21:28:08, Daniel Erat wrote: > i don't understand the reason for this delegate. > > delegates are usually used to do dependency injection -- for example, if ash/ > weren't able to depend on chromeos/, then ash would define an abstract > DelegateInterface, and then chrome/ (which can depend on both ash/ and > chromeos/) could define an implementation of DelegateInterface that calls into > chromeos/ and then instantiate it and pass it into > ash::PowerButtonController.[1] > > you don't have that dependency problem here, though, and the delegate > implementation actually lives inside of ash/ and is directly instantiated by > this class. > > i think that you would have less code, and simpler code, if you got rid of the > delegate and just made PowerButtonController call the PowerManagerClient methods > directly. > > 1. note that we're moving away from this delegate-implemented-by-chrome pattern > due to mus+ash, though. done, thanks. I was thinking using |delegate_| initialized or not to check OS_CHROMEOS env. But it looks like I am not in the right way. I agree with removing it. Thanks for explaining! https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:76: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/11/03 21:28:08, Daniel Erat wrote: > what's the reason for posting this instead of just calling > GetBacklightsForcedOff directly within the c'tor? note that the method usually > makes an asynchronous method call to powerd. if the stub implementation runs the > callback synchronously instead, please feel free to update it to be async so it > matches the real implementation. I think just GetInitialBacklightsForcedOff should be fine. It is called from PowerButtonController and dbus manager should be ready. Thanks! https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:106: if (!has_legacy_power_button_ && !MaybeTakeScreenshot(down) && On 2016/11/03 21:28:07, Daniel Erat wrote: > instead of calling this helper method twice, why not just remove it and inline > it above this? > > bool should_take_screenshot = > down && volume_down_pressed_ && > WmShell::Get() > ->maximize_mode_controller() > ->IsMaximizeModeWindowManagerEnabled(); Done. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:258: pressed_on_screen_off_ = true; On 2016/11/03 21:28:07, Daniel Erat wrote: > it seems a bit safer to set this unconditionally here instead of only setting it > to true: > > power_button_down_while_screen_off_ = brightness_is_zero_; > if (backlights_forced_off_) > SetBacklightsForcedOff(false); > > it would also be better to set this from OnPowerButtonEvent instead of > OnTabletPowerButtonEvent so it'll also have the correct state when we aren't in > tablet mode. > > (if backlights_forced_off_ is true, won't brightness_is_zero_ also be true?) done. If backlights_forced_off_is true, brightness_is_zero_ should be true. However, in test, that is not true. So there are two options: (1) leave backlights_forced_off_ || brightness_is_zero_ here (2) leave brightness_is_zero_ here, and do some work in tests so that when SetBacklightsForcedOff is called, BrightnessChanged can be called between zero and non-zero. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:302: SetBacklightsForcedOff(false); On 2016/11/03 21:28:07, Daniel Erat wrote: > i don't understand why this call is needed. the backlights are already forced > off in this case. I thought when constructed, user should face non-forced-off state. But I agree they are not needed. We should follow the actual initialization state. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:306: tablet_pre_start_shutdown_animation_timer_.Stop(); On 2016/11/03 21:28:08, Daniel Erat wrote: > you don't need this call to Stop(); Start() already restarts the timer if it's > already running. Done. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:328: void PowerButtonController::WakeOnLaptopMode() { On 2016/11/03 21:28:07, Daniel Erat wrote: > using the word "wake" here seems a bit inaccurate; it usually refers to > returning from the S3 (suspended) power state. if this method becomes shorter > (see below), i'd just inline it within the places that call it. Done. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:333: !maximize_mode_controller->IsMaximizeModeWindowManagerEnabled() && On 2016/11/03 21:28:07, Daniel Erat wrote: > i find all of these references to MaximizeModeController a bit confusing. how > about extracting them to two private helper methods? > > bool IsTabletModeActive() const; > bool IsTabletModeSupported() const; > > i think that'll make the rest of the code much clearer. this would turn into > IsTabletModeSupported() && !IsTabletModeActive(), for example. Done. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:334: (backlights_forced_off_ || brightness_is_zero_)) { On 2016/11/03 21:28:07, Daniel Erat wrote: > i don't understand the brightness_is_zero_ check here. if the backlights aren't > forced off but the brightness is zero (e.g. because the user reduced it to 0%), > why are you calling this? calling SetBacklightsForcedOff(false) won't do > anything if they already aren't forced off. Yes, brightness_is_zero_ is not needed. Thanks! https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.h:117: // A helper method to be called on convertible/tablet device. On 2016/11/03 21:28:08, Daniel Erat wrote: > nit: // Called by OnPowerButtonEvent on convertible/tablet devices. Done. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.h:151: bool MaybeTakeScreenshot(bool power_button_pressed); On 2016/11/03 21:28:08, Daniel Erat wrote: > "MaybeTakeScreenshot" suggests that this method might take a screenshot when > called, but it looks like it's actually just checking whether a screenshot > should be taken (without any side effects). > > but i think you should just remove this and inline the code -- see the comment > in the .cc file. Done. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.h:188: bool pressed_on_screen_off_; On 2016/11/03 21:28:08, Daniel Erat wrote: > this name is a bit vague. how about "power_button_down_while_screen_off_"? Done.
https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:258: pressed_on_screen_off_ = true; On 2016/11/04 01:14:03, Qiang (Joe) Xu wrote: > On 2016/11/03 21:28:07, Daniel Erat wrote: > > it seems a bit safer to set this unconditionally here instead of only setting > it > > to true: > > > > power_button_down_while_screen_off_ = brightness_is_zero_; > > if (backlights_forced_off_) > > SetBacklightsForcedOff(false); > > > > it would also be better to set this from OnPowerButtonEvent instead of > > OnTabletPowerButtonEvent so it'll also have the correct state when we aren't > in > > tablet mode. > > > > (if backlights_forced_off_ is true, won't brightness_is_zero_ also be true?) > > done. If backlights_forced_off_is true, brightness_is_zero_ should be true. > However, in test, that is not true. > > So there are two options: > (1) leave backlights_forced_off_ || brightness_is_zero_ here > (2) leave brightness_is_zero_ here, and do some work in tests so that when > SetBacklightsForcedOff is called, BrightnessChanged can be called between zero > and non-zero. i think that 2 is much better, since it ensures that the tests are simulating the real behavior instead of testing something that should never happen. :-) https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:35: const int kTabletPreStartShutdownAnimationTimeoutMs = 1000; // Amount of time the power button must be held to start the // pre-shutdown animation when in tablet mode. constexpr int kTabletShutdownTimeoutMs = 1000; https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:36: either remove blank line here or add a blank line after "namespace {" to match https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:101: backlights_forced_off_ || brightness_is_zero_; after updating tests, you can change this to just be: power_button_down_while_screen_off_ = down ? brightness_is_zero_ : false; , right? (so you also clear the old state on button-up) https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:158: if (IsTabletModeActive() && enable_quick_lock_) enable_quick_lock_ looks like it's true when --ash-enable-touch-view is set. is that still relevant? i'm not sure that this code will ever be executed now -- won't we call OnTabletPowerButtonEvent() earlier when we see that IsTabletModeSupported() return true? https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:198: if (IsTabletModeSupported() && !IsTabletModeActive() && this can just be: if (!IsTabletModeActive() && backlights_forced_off_) { , right? tablet mode can't be active if it isn't supported. :-) https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:218: if (IsTabletModeSupported() && !IsTabletModeActive() && same comment here https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:256: if (down) { i think that this could be changed to: if (down) { SetBacklightsForcedOff(false); StartTabletPowerButtonTimer(); } else { , since SetBacklightsForcedOff is a no-op if the state didn't change. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:261: if (power_button_down_while_screen_off_) { seems simpler to move the timer check to the outer level: if (tablet_power_button_timer_.IsRunning()) { tablet_power_button_timer_.Stop(); if (!power_button_down_while_screen_off_) { SetBacklightsForcedOff(true); LockScreenIfRequired(); } } https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:264: power_button_down_while_screen_off_ = false; i'd remove this line; it's better to only set the member from a single place (i.e. OnPowerButtonEvent) https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:52: bool tablet_pre_start_shutdown_animation_timer_is_running() const { neither of these are trivial getters or setters, so they should use TheRegularMethodNamingStyle and be defined in the .cc file instead of inlined here https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:107: // Sends a request to get the backlights forced off state so that nit: "... a request to powerd to get ..." https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:112: void HandleInitialBacklightsForcedOff(bool is_forced_off); nit: OnGotInitialBacklightsForcedOff for consistency with other methods https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:115: // white animation managed by |controller_|. // Starts |tablet_power_button_timer_| when the power button // is pressed while in tablet mode. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:119: // by |controller_|. // Called by |tablet_power_button_timer_| to start the // pre-shutdown animation. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:123: // setting backlights forced off should be accompanied with locking screen. // Locks the screen if the "require password to wake from sleep" // pref is set and locking is possible. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:130: // Returns true if device works in tablet/maximize mode, otherwise false. s/if device works in/if device is currently in/ https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:133: // The one sec timer to wait for tablet power button released. If it is nit: mind moving this down to the bottom instead of putting it above the more-important-seeming power_button_down_ and lock_button_down_ members? i also wouldn't mention the delay in the comment, since it's likely to become stale if we change this. maybe something like this instead: // Started when the tablet power button is pressed and stopped // when it's released. Runs OnTabletPowerButtonTimeout() to start // shutdown. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:135: base::OneShotTimer tablet_pre_start_shutdown_animation_timer_; maybe just call this tablet_power_button_timer_? i know that's a bit less descriptive but pre_start_shutdown_animation is pretty confusing. :-) if we need additional timers later, it's easy to give this a longer name then. if so, i'd also rename the methods to StartTabletPowerButtonTimer and OnTabletPowerButtonTimeout. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:168: // or not. // True if the screen was off when the power button was pressed. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:172: // calling GetBacklightsForcedOff method. // Current forced-off state of backlights.
Hi Daniel, thanks for reviewing. Here is the new patch and the reply. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_control... ash/wm/power_button_controller.cc:258: pressed_on_screen_off_ = true; On 2016/11/04 16:12:51, Daniel Erat wrote: > On 2016/11/04 01:14:03, Qiang (Joe) Xu wrote: > > On 2016/11/03 21:28:07, Daniel Erat wrote: > > > it seems a bit safer to set this unconditionally here instead of only > setting > > it > > > to true: > > > > > > power_button_down_while_screen_off_ = brightness_is_zero_; > > > if (backlights_forced_off_) > > > SetBacklightsForcedOff(false); > > > > > > it would also be better to set this from OnPowerButtonEvent instead of > > > OnTabletPowerButtonEvent so it'll also have the correct state when we aren't > > in > > > tablet mode. > > > > > > (if backlights_forced_off_ is true, won't brightness_is_zero_ also be true?) > > > > done. If backlights_forced_off_is true, brightness_is_zero_ should be true. > > However, in test, that is not true. > > > > So there are two options: > > (1) leave backlights_forced_off_ || brightness_is_zero_ here > > (2) leave brightness_is_zero_ here, and do some work in tests so that when > > SetBacklightsForcedOff is called, BrightnessChanged can be called between zero > > and non-zero. > > i think that 2 is much better, since it ensures that the tests are simulating > the real behavior instead of testing something that should never happen. :-) Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:35: const int kTabletPreStartShutdownAnimationTimeoutMs = 1000; On 2016/11/04 16:12:52, Daniel Erat wrote: > // Amount of time the power button must be held to start the > // pre-shutdown animation when in tablet mode. > constexpr int kTabletShutdownTimeoutMs = 1000; Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:36: On 2016/11/04 16:12:52, Daniel Erat wrote: > either remove blank line here or add a blank line after "namespace {" to match Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:101: backlights_forced_off_ || brightness_is_zero_; On 2016/11/04 16:12:52, Daniel Erat wrote: > after updating tests, you can change this to just be: > > power_button_down_while_screen_off_ = > down ? brightness_is_zero_ : false; > > , right? (so you also clear the old state on button-up) We cannot clear the state on button-up here because it is needed in OnTabletPowerButtonEvent. It will cause clearing the state before using. if (down) power_button_down_while_screen_off_ = brightness_is_zero_; And later clear it after it is used in if-else. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:158: if (IsTabletModeActive() && enable_quick_lock_) On 2016/11/04 16:12:52, Daniel Erat wrote: > enable_quick_lock_ looks like it's true when --ash-enable-touch-view is set. is > that still relevant? i'm not sure that this code will ever be executed now -- > won't we call OnTabletPowerButtonEvent() earlier when we see that > IsTabletModeSupported() return true? mm.. yes, do we still need quick lock here? I think power button pressed will directly cause the screen off on tablet mode. The "quick lock" probably should be deleted. I don't have much context with "quick lock", just thinking from the name that it may be no longer needed. Tom is OOO today. I may check with him later. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:198: if (IsTabletModeSupported() && !IsTabletModeActive() && On 2016/11/04 16:12:52, Daniel Erat wrote: > this can just be: > > if (!IsTabletModeActive() && backlights_forced_off_) { > > , right? tablet mode can't be active if it isn't supported. :-) yes, done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:218: if (IsTabletModeSupported() && !IsTabletModeActive() && On 2016/11/04 16:12:52, Daniel Erat wrote: > same comment here Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:256: if (down) { On 2016/11/04 16:12:52, Daniel Erat wrote: > i think that this could be changed to: > > if (down) { > SetBacklightsForcedOff(false); > StartTabletPowerButtonTimer(); > } else { > > , since SetBacklightsForcedOff is a no-op if the state didn't change. Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:261: if (power_button_down_while_screen_off_) { On 2016/11/04 16:12:52, Daniel Erat wrote: > seems simpler to move the timer check to the outer level: > > if (tablet_power_button_timer_.IsRunning()) { > tablet_power_button_timer_.Stop(); > if (!power_button_down_while_screen_off_) { > SetBacklightsForcedOff(true); > LockScreenIfRequired(); > } > } done, yes, it is clear. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:264: power_button_down_while_screen_off_ = false; On 2016/11/04 16:12:52, Daniel Erat wrote: > i'd remove this line; it's better to only set the member from a single place > (i.e. OnPowerButtonEvent) See the comment OnPowerButtonEvent. If you don't like reset the status here, we probably can create a Scoped Object that gets created on power_button_down_while_screen_off_ and reset here. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:52: bool tablet_pre_start_shutdown_animation_timer_is_running() const { On 2016/11/04 16:12:52, Daniel Erat wrote: > neither of these are trivial getters or setters, so they should use > TheRegularMethodNamingStyle and be defined in the .cc file instead of inlined > here Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:107: // Sends a request to get the backlights forced off state so that On 2016/11/04 16:12:52, Daniel Erat wrote: > nit: "... a request to powerd to get ..." Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:112: void HandleInitialBacklightsForcedOff(bool is_forced_off); On 2016/11/04 16:12:52, Daniel Erat wrote: > nit: OnGotInitialBacklightsForcedOff for consistency with other methods Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:115: // white animation managed by |controller_|. On 2016/11/04 16:12:52, Daniel Erat wrote: > // Starts |tablet_power_button_timer_| when the power button > // is pressed while in tablet mode. Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:119: // by |controller_|. On 2016/11/04 16:12:52, Daniel Erat wrote: > // Called by |tablet_power_button_timer_| to start the > // pre-shutdown animation. Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:123: // setting backlights forced off should be accompanied with locking screen. On 2016/11/04 16:12:52, Daniel Erat wrote: > // Locks the screen if the "require password to wake from sleep" > // pref is set and locking is possible. Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:130: // Returns true if device works in tablet/maximize mode, otherwise false. On 2016/11/04 16:12:52, Daniel Erat wrote: > s/if device works in/if device is currently in/ Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:133: // The one sec timer to wait for tablet power button released. If it is On 2016/11/04 16:12:52, Daniel Erat wrote: > nit: mind moving this down to the bottom instead of putting it above the > more-important-seeming power_button_down_ and lock_button_down_ members? i also > wouldn't mention the delay in the comment, since it's likely to become stale if > we change this. maybe something like this instead: > > // Started when the tablet power button is pressed and stopped > // when it's released. Runs OnTabletPowerButtonTimeout() to start > // shutdown. Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:135: base::OneShotTimer tablet_pre_start_shutdown_animation_timer_; On 2016/11/04 16:12:52, Daniel Erat wrote: > maybe just call this tablet_power_button_timer_? i know that's a bit less > descriptive but pre_start_shutdown_animation is pretty confusing. :-) if we need > additional timers later, it's easy to give this a longer name then. > > if so, i'd also rename the methods to StartTabletPowerButtonTimer and > OnTabletPowerButtonTimeout. OK. I agree with this change. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:168: // or not. On 2016/11/04 16:12:52, Daniel Erat wrote: > // True if the screen was off when the power button was pressed. Done. https://codereview.chromium.org/2474913004/diff/20001/ash/wm/power_button_con... ash/wm/power_button_controller.h:172: // calling GetBacklightsForcedOff method. On 2016/11/04 16:12:52, Daniel Erat wrote: > // Current forced-off state of backlights. Done.
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...
https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:253: void PowerButtonController::OnTabletPowerButtonEvent( hmm... so i've noticed that the tablet power button code seems to be completely disjoint from the regular power button code now. would it make sense to move it into its own class in a different file after all, and have it be chrome-os-only? PowerButtonController would instantiate TabletPowerButtonController only when OS_CHROMEOS is defined. i feel like that might be cleaner, since it feels completely separate from the regular code. class TabletPowerButtonController { public: // Returns true if power button events should be handled by // this class instead of PowerButtonController. bool ShouldHandlePowerButtonEvents() const; // Handles a power button event. void OnPowerButtonEvent(bool down, base::TimeTicks timestamp); private: // all of your new methods go here // also members like power_button_down_while_screen_off_ } then in PowerButtonController::OnPowerButtonEvent: #if defined(OS_CHROMEOS) if (!has_legacy_power_button_ && !should_take_screenshot && tablet_controller_->ShouldHandlePowerButtonEvents()) { tablet_controller_->OnPowerButtonEvent(down, timestamp); return; } https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_con... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.h:52: bool TabletPowerButtonTimerIsRunning() const; nit: add comments to both of these documenting what they do
Hi Daniel, attached the CL based on the comments. Thanks! https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:253: void PowerButtonController::OnTabletPowerButtonEvent( On 2016/11/04 20:18:34, Daniel Erat wrote: > hmm... so i've noticed that the tablet power button code seems to be completely > disjoint from the regular power button code now. would it make sense to move it > into its own class in a different file after all, and have it be chrome-os-only? > PowerButtonController would instantiate TabletPowerButtonController only when > OS_CHROMEOS is defined. > > i feel like that might be cleaner, since it feels completely separate from the > regular code. > > class TabletPowerButtonController { > public: > // Returns true if power button events should be handled by > // this class instead of PowerButtonController. > bool ShouldHandlePowerButtonEvents() const; > > // Handles a power button event. > void OnPowerButtonEvent(bool down, base::TimeTicks timestamp); > > private: > // all of your new methods go here > // also members like power_button_down_while_screen_off_ > } > > then in PowerButtonController::OnPowerButtonEvent: > > #if defined(OS_CHROMEOS) > if (!has_legacy_power_button_ && !should_take_screenshot && > tablet_controller_->ShouldHandlePowerButtonEvents()) { > tablet_controller_->OnPowerButtonEvent(down, timestamp); > return; > } Actually there are some overlaps. Like TabletPowerButtonController needs the access to brightness_is_zero_ (but we can pass it as parameter). In this way, TabletPowerButtonController only needs to subclass ui::EventHandler. https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_con... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_con... ash/wm/power_button_controller.h:52: bool TabletPowerButtonTimerIsRunning() const; On 2016/11/04 20:18:34, Daniel Erat wrote: > nit: add comments to both of these documenting what they do Done. https://codereview.chromium.org/2474913004/diff/60001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (left): https://codereview.chromium.org/2474913004/diff/60001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:21: #include "ui/events/event_handler.h" it is included in head file. https://codereview.chromium.org/2474913004/diff/60001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/60001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:84: ->IsMaximizeModeWindowManagerEnabled(); Shall we make IsTabletModeActive and IsTabletModeSupported methods belong to PowerButtonController? They can be static method so that TabletPowerButtonController can also call them. https://codereview.chromium.org/2474913004/diff/60001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:145: ->IsMaximizeModeWindowManagerEnabled() && This should not be reached any more.. we probably need to delete the related code. I am surprised to see no tests are broken here. Deletion of code can be the separate cl I think. https://codereview.chromium.org/2474913004/diff/60001/ash/wm/tablet_power_but... File ash/wm/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/60001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:45: bool ShouldHandlePowerButtonEvents() const; It is the same as "IsTabletModeSupported". Using ShouldHandlePowerButtonEvents is to make it more clear?
thanks for moving the code into a new class! it's much easier to understand now. :-) https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:53: tablet_controller_.reset(new TabletPowerButtonController(controller_)); do you mind renaming the "controller_" member to "lock_state_controller_"? there are too many controllers in this file. :-) https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... ash/wm/power_button_controller.h:35: class TabletPowerButtonControllerTest; this should also be declared for OS_CHROMEOS https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... ash/wm/power_button_controller.h:84: friend class test::TabletPowerButtonControllerTest; this seems strange. why do the tests for the TabletPowerButtonController class need to mess with the internals of PowerButtonController? https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... ash/wm/power_button_controller.h:117: // When device is convertible/tablet, pass the OnPowerButtonEvent to nit: "// Handles events for convertible/tablet devices." https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... File ash/wm/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:21: constexpr int kTabletShutdownTimeoutMs = 1000; nit: remove "Tablet" from name https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:37: controller_->OnTabletPowerButtonTimeout(); i'd make sure that the timer is actually running here first https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:43: controller_ = controller; nit: use an initialization list for controller_ https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:45: GetInitialBacklightsForcedOff(); i think you also need to handle the case where powerd restarts. you could do this by using PowerManagerClient::Observer::PowerManagerRestarted to either a) query powerd's new state, or b) set powerd's state to backlights_forced_off_. b) seems like it might be slightly better... https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:53: return IsTabletModeSupported(); would it be cleaner to make IsTabletModeSupported public? maybe it could even be static, and then you could avoid instantiating TabletPowerButtonController if it's unneeded (assuming that the "supported" state can't change at runtime) https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:63: StartTabletPowerButtonTimer(); should you only start this if we aren't already shutting down? otherwise, it looks like OnTabletPowerButtonTimeout could call StartShutdownAnimation a second time, which seems like it could be problematic. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:74: // When power button is released, Cancel Shutdown animation whenever it is nit: s/Shutdown/shutdown/ https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:86: void TabletPowerButtonController::OnMouseEvent(ui::MouseEvent* event) { i'm assuming that we intentionally don't want to stop forcing the backlights off for touch events, but what about stylus events (assuming that we can distinguish those)? https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... File ash/wm/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:28: // Returns true when |tablet_power_button_timer_| is not timeout. s/is not timeout/is running/ https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:68: void StartTabletPowerButtonTimer(); nit: rename this to match the member's name (see below) https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:71: void OnTabletPowerButtonTimeout(); nit: rename https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:89: bool power_button_down_while_screen_off_ = false; i know i suggested this name earlier, but how about "screen_off_when_power_button_down_"? that seems even clearer. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:93: base::OneShotTimer tablet_power_button_timer_; how about renaming this to "shutdown_timer_" now? that seems shorter and more specific :-) https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... File ash/wm/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller_unittest.cc:63: power_button_controller_->OnPowerButtonEvent(true, base::TimeTicks::Now()); i'd avoid using PowerButtonController at all in this test and just call TabletPowerButtonController's methods directly. otherwise, you're testing PowerButtonController in addition to TabletPowerButtonController.
Patchset #6 (id:100001) has been deleted
Patchset #7 (id:140001) has been deleted
Hi Daniel, new patch change contains the below ones in addition to your comments. (1) move ash/wm/tablet_power_button_controller.h(cc) to ash/system/chromeos/power/tablet_pwer_button_controller.h(cc) as ash/wm cannot directly depend on chromeos/dbus. It has to be #if defined(OS_CHROMEOS). (2) delete quick lock related code. (3) add support for removing stylus to wake device (4) fix the bug when pressing power button to wake from suspended state Thanks! https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... ash/wm/power_button_controller.cc:53: tablet_controller_.reset(new TabletPowerButtonController(controller_)); On 2016/11/07 20:54:41, Daniel Erat wrote: > do you mind renaming the "controller_" member to "lock_state_controller_"? there > are too many controllers in this file. :-) Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... ash/wm/power_button_controller.h:35: class TabletPowerButtonControllerTest; On 2016/11/07 20:54:41, Daniel Erat wrote: > this should also be declared for OS_CHROMEOS done by removing this. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... ash/wm/power_button_controller.h:84: friend class test::TabletPowerButtonControllerTest; On 2016/11/07 20:54:41, Daniel Erat wrote: > this seems strange. why do the tests for the TabletPowerButtonController class > need to mess with the internals of PowerButtonController? It is to get the access to |tablet_controller_|. I am removing this because now I make the TabletPowerButtonController created in PowerButtonController and static GetInstance in test. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/power_button_con... ash/wm/power_button_controller.h:117: // When device is convertible/tablet, pass the OnPowerButtonEvent to On 2016/11/07 20:54:41, Daniel Erat wrote: > nit: "// Handles events for convertible/tablet devices." Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... File ash/wm/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:21: constexpr int kTabletShutdownTimeoutMs = 1000; On 2016/11/07 20:54:41, Daniel Erat wrote: > nit: remove "Tablet" from name Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:37: controller_->OnTabletPowerButtonTimeout(); On 2016/11/07 20:54:41, Daniel Erat wrote: > i'd make sure that the timer is actually running here first Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:43: controller_ = controller; On 2016/11/07 20:54:41, Daniel Erat wrote: > nit: use an initialization list for controller_ Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:45: GetInitialBacklightsForcedOff(); On 2016/11/07 20:54:41, Daniel Erat wrote: > i think you also need to handle the case where powerd restarts. you could do > this by using PowerManagerClient::Observer::PowerManagerRestarted to either a) > query powerd's new state, or b) set powerd's state to backlights_forced_off_. b) > seems like it might be slightly better... Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:53: return IsTabletModeSupported(); On 2016/11/07 20:54:41, Daniel Erat wrote: > would it be cleaner to make IsTabletModeSupported public? maybe it could even be > static, and then you could avoid instantiating TabletPowerButtonController if > it's unneeded (assuming that the "supported" state can't change at runtime) Avoid instantiating makes sense to me. However, I find CanEnterMaximizeMode is not updated in PBC's ctor. That means OnAccelerometerUpdated is being notified at a later time. I don't know whether we have a way to process this.. I am doing the change by moving IsTabletModeSupported and IsTabletModeActive to anonymous space. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:63: StartTabletPowerButtonTimer(); On 2016/11/07 20:54:41, Daniel Erat wrote: > should you only start this if we aren't already shutting down? otherwise, it > looks like OnTabletPowerButtonTimeout could call StartShutdownAnimation a second > time, which seems like it could be problematic. This case will be handled by PBC::OnPowerButtonEvent. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:74: // When power button is released, Cancel Shutdown animation whenever it is On 2016/11/07 20:54:41, Daniel Erat wrote: > nit: s/Shutdown/shutdown/ Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.cc:86: void TabletPowerButtonController::OnMouseEvent(ui::MouseEvent* event) { On 2016/11/07 20:54:41, Daniel Erat wrote: > i'm assuming that we intentionally don't want to stop forcing the backlights off > for touch events, but what about stylus events (assuming that we can distinguish > those)? I learned from Jacob that stylus event is a mouse event. I updated the code. I discussed with Tom. Stylus touchscreen events should keep forcing the backlights off. However, taking stylus from device should stop forcing the backlights off. I add changes to the new patch. However, Stylus touchscreen events should keep forcing the backlights off doesn't work on kevin due to this: crbug.com/664002. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... File ash/wm/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:28: // Returns true when |tablet_power_button_timer_| is not timeout. On 2016/11/07 20:54:41, Daniel Erat wrote: > s/is not timeout/is running/ Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:68: void StartTabletPowerButtonTimer(); On 2016/11/07 20:54:41, Daniel Erat wrote: > nit: rename this to match the member's name (see below) Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:71: void OnTabletPowerButtonTimeout(); On 2016/11/07 20:54:41, Daniel Erat wrote: > nit: rename Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:89: bool power_button_down_while_screen_off_ = false; On 2016/11/07 20:54:41, Daniel Erat wrote: > i know i suggested this name earlier, but how about > "screen_off_when_power_button_down_"? that seems even clearer. Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller.h:93: base::OneShotTimer tablet_power_button_timer_; On 2016/11/07 20:54:41, Daniel Erat wrote: > how about renaming this to "shutdown_timer_" now? that seems shorter and more > specific :-) Done. https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... File ash/wm/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2474913004/diff/80001/ash/wm/tablet_power_but... ash/wm/tablet_power_button_controller_unittest.cc:63: power_button_controller_->OnPowerButtonEvent(true, base::TimeTicks::Now()); On 2016/11/07 20:54:42, Daniel Erat wrote: > i'd avoid using PowerButtonController at all in this test and just call > TabletPowerButtonController's methods directly. otherwise, you're testing > PowerButtonController in addition to TabletPowerButtonController. Done.
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...
https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.cc:27: constexpr int kWaitSuspendDoneDurationMs = 2000; nit: kIgnorePowerButtonAfterResumeMs? https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.cc:61: DCHECK(ShutdownTimerIsRunning()); how about making this method return false if the timer isn't running and adding WARN_UNUSED_RESULT to the signature? then you can get rid of ShutdownTimerIsRunning() and tests can just EXPECT_FALSE(TriggerShutdownTimeout) instead. https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.cc:102: // When pressing power button causing SuspendDone, chrome side will first how about this? // When the system resumes in response to the power button being pressed, // Chrome receives powerd's SuspendDone signal and notification that the // backlight has been turned back on before seeing the power button events // that woke the system. Ignore events just after resuming to ensure that // we don't turn the screen off in response to the events. https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.cc:146: last_resume_time_ = base::TimeTicks::Now(); i'd suggest using base::TickClock instead so you can inject fake time to test this code: https://chromium.googlesource.com/chromium/src/+/master/base/time/tick_clock.h https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:31: virtual ~TestApi(); this doesn't need to be virtual https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:48: static TabletPowerButtonController* GetInstance(); if at all possible, please don't introduce new singletons. they cause all sorts of problems around initialization and destruction order. it's much clearer and safer to have clear ownership of objects. https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:70: // Set backlights to |forced_off| if needed. nit: s/if needed/if they aren't already/ https://codereview.chromium.org/2474913004/diff/160001/ash/wm/power_button_co... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/160001/ash/wm/power_button_co... ash/wm/power_button_controller.h:105: std::unique_ptr<TabletPowerButtonController> tablet_controller_; you need to #include <memory> for this https://codereview.chromium.org/2474913004/diff/160001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/2474913004/diff/160001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:143: forced_off ? SendBrightnessChanged(0, true) please don't call SendBrightnessChanged from here; it makes it impossible to test other things happening between sending the request and receiving the notification. tests should be able to call SendBrightnessChanged themselves when they're ready for the notification to be sent.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Daniel, new patch uploaded. Changes mostly focused on comments. Add test coverage for pressing power button when suspended. Thanks! https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.cc:27: constexpr int kWaitSuspendDoneDurationMs = 2000; On 2016/11/10 22:48:55, Daniel Erat wrote: > nit: kIgnorePowerButtonAfterResumeMs? Done. https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.cc:61: DCHECK(ShutdownTimerIsRunning()); On 2016/11/10 22:48:54, Daniel Erat wrote: > how about making this method return false if the timer isn't running and adding > WARN_UNUSED_RESULT to the signature? then you can get rid of > ShutdownTimerIsRunning() and tests can just EXPECT_FALSE(TriggerShutdownTimeout) > instead. EXPECT_FALSE(TriggerShutdownTimeout) can represent EXPECT_TRUE(ShutdownTimerIsRunning), but then we lose the ability to check EXPECT_FALSE(ShutdownTimerIsRunning), which I use in one of the unittests, if we get rid of ShutdownTimerIsRunning. I didn't update here right now. https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.cc:102: // When pressing power button causing SuspendDone, chrome side will first On 2016/11/10 22:48:55, Daniel Erat wrote: > how about this? > > // When the system resumes in response to the power button being pressed, > // Chrome receives powerd's SuspendDone signal and notification that the > // backlight has been turned back on before seeing the power button events > // that woke the system. Ignore events just after resuming to ensure that > // we don't turn the screen off in response to the events. It is much better. Thanks! https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.cc:146: last_resume_time_ = base::TimeTicks::Now(); On 2016/11/10 22:48:54, Daniel Erat wrote: > i'd suggest using base::TickClock instead so you can inject fake time to test > this code: > https://chromium.googlesource.com/chromium/src/+/master/base/time/tick_clock.h Done. https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:31: virtual ~TestApi(); On 2016/11/10 22:48:55, Daniel Erat wrote: > this doesn't need to be virtual Done. https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:48: static TabletPowerButtonController* GetInstance(); On 2016/11/10 22:48:55, Daniel Erat wrote: > if at all possible, please don't introduce new singletons. they cause all sorts > of problems around initialization and destruction order. it's much clearer and > safer to have clear ownership of objects. I see. I make TabletPowerButtonController belongs to ash::Shell now. https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:70: // Set backlights to |forced_off| if needed. On 2016/11/10 22:48:55, Daniel Erat wrote: > nit: s/if needed/if they aren't already/ Done. https://codereview.chromium.org/2474913004/diff/160001/ash/wm/power_button_co... File ash/wm/power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/160001/ash/wm/power_button_co... ash/wm/power_button_controller.h:105: std::unique_ptr<TabletPowerButtonController> tablet_controller_; On 2016/11/10 22:48:55, Daniel Erat wrote: > you need to #include <memory> for this Done. https://codereview.chromium.org/2474913004/diff/160001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/2474913004/diff/160001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:143: forced_off ? SendBrightnessChanged(0, true) On 2016/11/10 22:48:55, Daniel Erat wrote: > please don't call SendBrightnessChanged from here; it makes it impossible to > test other things happening between sending the request and receiving the > notification. tests should be able to call SendBrightnessChanged themselves when > they're ready for the notification to be sent. done by moving to tests.
thanks. i'll be ooo tomorrow https://codereview.chromium.org/2474913004/diff/180001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/2474913004/diff/180001/ash/shell.h#newcode120 ash/shell.h:120: class TabletPowerButtonController; nit: this should probably be in an ifdef too https://codereview.chromium.org/2474913004/diff/180001/ash/shell.h#newcode536 ash/shell.h:536: std::unique_ptr<TabletPowerButtonController> tablet_power_button_controller_; you can probably avoid moving this to ash::Shell by just adding a tablet_power_button_controller_for_test() accessor to PowerButtonController, right? https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:24: // instantiated and used in PowerButtonController. (i'm still hoping you can move this out of ash::Shell, but if you can't, this comment needs to be updated) https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:29: constexpr int kNonZeroBrightnessForTest = 10; nit: drop the "ForTest" here? https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:100: base::Bind(&CheckBoolEquals, expected_forced_off)); does this callback ever run? i don't see the message loop getting run anywhere, so i suspect it might not. it'd probably be better to just do something like: void CopyResult(bool* dest, bool src) { *dest = src; } bool GetBacklightsForcedOff() WARN_UNUSED_RESULT { bool forced_off = false; power_manager_client_->GetBacklightsForcedOff( base::Bind(&CopyResult, base::Unretained(&forced_off))); base::RunLoop().RunUntilIdle(); return forced_off; } EXPECT_TRUE(GetBacklightsForcedOff()); https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:104: chromeos::FakePowerManagerClient* power_manager_client_; nit: add a blank line after this line so it's clearer that the previous comment only applies to this member https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:142: // is timeout. nit: "Tests that shutdown is cancelled if the power button is released quickly." https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:144: ReleasePowerButtonBeforeStartingShutdownTimer) { this should be "BeforeStartingShutdownAnimation", right? the timer is already started. (maybe it's referring to the animation timer, but please make the name of this test consistent with the name of the next one.) https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:176: PressPowerButton(); i don't understand what this part of the test is doing. isn't the system already shutting down at this point due to the earlier TriggerShutdownTimeout call? https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:197: ReleasePowerButton(); you should also check that the backlights are still not forced off now, right?
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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_...)
Hi Daniel, Uploaded new patch and my comments. Thanks for reviewing! https://codereview.chromium.org/2474913004/diff/180001/ash/shell.h File ash/shell.h (right): https://codereview.chromium.org/2474913004/diff/180001/ash/shell.h#newcode120 ash/shell.h:120: class TabletPowerButtonController; On 2016/11/11 05:40:50, Daniel Erat wrote: > nit: this should probably be in an ifdef too done by removing https://codereview.chromium.org/2474913004/diff/180001/ash/shell.h#newcode536 ash/shell.h:536: std::unique_ptr<TabletPowerButtonController> tablet_power_button_controller_; On 2016/11/11 05:40:50, Daniel Erat wrote: > you can probably avoid moving this to ash::Shell by just adding a > tablet_power_button_controller_for_test() accessor to PowerButtonController, > right? Done. https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:24: // instantiated and used in PowerButtonController. On 2016/11/11 05:40:50, Daniel Erat wrote: > (i'm still hoping you can move this out of ash::Shell, but if you can't, this > comment needs to be updated) Done. https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:29: constexpr int kNonZeroBrightnessForTest = 10; On 2016/11/11 05:40:50, Daniel Erat wrote: > nit: drop the "ForTest" here? Done. https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:100: base::Bind(&CheckBoolEquals, expected_forced_off)); On 2016/11/11 05:40:51, Daniel Erat wrote: > does this callback ever run? i don't see the message loop getting run anywhere, > so i suspect it might not. > > it'd probably be better to just do something like: > > void CopyResult(bool* dest, bool src) { > *dest = src; > } > > bool GetBacklightsForcedOff() WARN_UNUSED_RESULT { > bool forced_off = false; > power_manager_client_->GetBacklightsForcedOff( > base::Bind(&CopyResult, base::Unretained(&forced_off))); > base::RunLoop().RunUntilIdle(); > return forced_off; > } > > EXPECT_TRUE(GetBacklightsForcedOff()); Done. https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:104: chromeos::FakePowerManagerClient* power_manager_client_; On 2016/11/11 05:40:50, Daniel Erat wrote: > nit: add a blank line after this line so it's clearer that the previous comment > only applies to this member Done. https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:142: // is timeout. On 2016/11/11 05:40:50, Daniel Erat wrote: > nit: "Tests that shutdown is cancelled if the power button is released quickly." I changed to "Tests that shutdown animation is not started if the power button is released quickly". https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:144: ReleasePowerButtonBeforeStartingShutdownTimer) { On 2016/11/11 05:40:51, Daniel Erat wrote: > this should be "BeforeStartingShutdownAnimation", right? the timer is already > started. (maybe it's referring to the animation timer, but please make the name > of this test consistent with the name of the next one.) yes, it should be "BeforeStartingShutdownAnimation" https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:176: PressPowerButton(); On 2016/11/11 05:40:50, Daniel Erat wrote: > i don't understand what this part of the test is doing. isn't the system already > shutting down at this point due to the earlier TriggerShutdownTimeout call? I missed two lines in the earlier tests. I think "TriggerShutdownTimeout()" is misleading. The earlier test is to test cancelling/reverting shutdown animation when backlights is not forced off. The later test is to test cancelling/reverting shutdown animation when backlights is forced off. It is actually triggering the one second shutdown_timer_ timeout and start shutdown animation timer. But the one second timer is called |shutdown_timer_| in TabletPowerButtonController for short. Do we really want |shutdown_timer_| for short description? It sounds like causing misleading here. https://codereview.chromium.org/2474913004/diff/180001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:197: ReleasePowerButton(); On 2016/11/11 05:40:50, Daniel Erat wrote: > you should also check that the backlights are still not forced off now, right? Done.
Patchset #9 (id:200001) has been deleted
Patchset #10 (id:240001) has been deleted
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
just a few comments left https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:25: class ASH_EXPORT TabletPowerButtonController nit: does this actually need ASH_EXPORT? https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:58: // Overriden from chromeos::PowerManagerClient::Observer: nit: "Overridden" here and below https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:81: void SystemUnlocks() { nit: UnlockScreen? https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:97: void CheckLockedState(bool expected_locked) { how about: bool GetLockedState() so that you can use EXPECT_TRUE/FALSE in tests and get correct line numbers on failures? https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:127: // should lock screen automatically. nit: "... if automatic screen-locking was requested." https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:139: CheckLockedState(false); if you change to GetLockedState as requested above, i'd use ASSERT_FALSE here https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:142: // screen automatically. nit: "... if automatic screen-locking wasn't requested." https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:170: // Tests the power button released is done after shutdown timer is timeout, // Tests that the shutdown animation is started when the power button // is released after the timer fires. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:206: EXPECT_FALSE(GetBacklightsForcedOff()); nit: check this before sending the non-zero brightness? https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:224: // Test PowerButtonEvent is sent with a delay. nit: // Send the power button event after a short delay and check that it is ignored. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:228: EXPECT_FALSE(GetBacklightsForcedOff()); please send another event after another 500-600 ms and check that it's honored
Hi Daniel, new patch is uploaded, ptal, thanks https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:25: class ASH_EXPORT TabletPowerButtonController On 2016/11/14 16:15:30, Daniel Erat wrote: > nit: does this actually need ASH_EXPORT? Yes, it is needed, otherwise there will be linker error for tablet_power_button_controller_unittest. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller.h:58: // Overriden from chromeos::PowerManagerClient::Observer: On 2016/11/14 16:15:30, Daniel Erat wrote: > nit: "Overridden" here and below Done. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:81: void SystemUnlocks() { On 2016/11/14 16:15:31, Daniel Erat wrote: > nit: UnlockScreen? Done. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:97: void CheckLockedState(bool expected_locked) { On 2016/11/14 16:15:31, Daniel Erat wrote: > how about: > > bool GetLockedState() > > so that you can use EXPECT_TRUE/FALSE in tests and get correct line numbers on > failures? Done. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:127: // should lock screen automatically. On 2016/11/14 16:15:31, Daniel Erat wrote: > nit: "... if automatic screen-locking was requested." Done. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:139: CheckLockedState(false); On 2016/11/14 16:15:31, Daniel Erat wrote: > if you change to GetLockedState as requested above, i'd use ASSERT_FALSE here Done. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:142: // screen automatically. On 2016/11/14 16:15:31, Daniel Erat wrote: > nit: "... if automatic screen-locking wasn't requested." Done. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:170: // Tests the power button released is done after shutdown timer is timeout, On 2016/11/14 16:15:31, Daniel Erat wrote: > // Tests that the shutdown animation is started when the power button > // is released after the timer fires. Done. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:206: EXPECT_FALSE(GetBacklightsForcedOff()); On 2016/11/14 16:15:31, Daniel Erat wrote: > nit: check this before sending the non-zero brightness? Done. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:224: // Test PowerButtonEvent is sent with a delay. On 2016/11/14 16:15:31, Daniel Erat wrote: > nit: // Send the power button event after a short delay and check that it is > ignored. Done. https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:228: EXPECT_FALSE(GetBacklightsForcedOff()); On 2016/11/14 16:15:31, Daniel Erat wrote: > please send another event after another 500-600 ms and check that it's honored Since the time duration to wait is 2s, I think the suggestion here is 1500-1600 ms?
lgtm https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:228: EXPECT_FALSE(GetBacklightsForcedOff()); On 2016/11/14 18:47:57, Qiang (Joe) Xu wrote: > On 2016/11/14 16:15:31, Daniel Erat wrote: > > please send another event after another 500-600 ms and check that it's honored > > Since the time duration to wait is 2s, I think the suggestion here is 1500-1600 > ms? whoops. yes, sorry -- got the delays confused https://codereview.chromium.org/2474913004/diff/280001/ash/system/chromeos/po... File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2474913004/diff/280001/ash/system/chromeos/po... ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:138: EXPECT_FALSE(GetLockedState()); nit: ASSERT_FALSE since something is seriously wrong outside of the TabletPowerButtonController code if this is still true
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/2474913004/#ps300001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Tablet-like power button behavior on Convertible/Tablet ChromeOS devices Changes: device consistency: convertible/tablet chromeos devices follow the behavior described in go/touchview-power convertible behavior; other devices follow clamshell behavior. BUG=633304 TEST=manually test; add unitttest ========== to ========== Tablet-like power button behavior on Convertible/Tablet ChromeOS devices Changes: device consistency: convertible/tablet chromeos devices follow the behavior described in go/touchview-power convertible behavior; other devices follow clamshell behavior. BUG=633304 TEST=manually test; add unitttest ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Tablet-like power button behavior on Convertible/Tablet ChromeOS devices Changes: device consistency: convertible/tablet chromeos devices follow the behavior described in go/touchview-power convertible behavior; other devices follow clamshell behavior. BUG=633304 TEST=manually test; add unitttest ========== to ========== Tablet-like power button behavior on Convertible/Tablet ChromeOS devices Changes: device consistency: convertible/tablet chromeos devices follow the behavior described in go/touchview-power convertible behavior; other devices follow clamshell behavior. BUG=633304 TEST=manually test; add unitttest Committed: https://crrev.com/72135ade1aacb9f9fcfe61f8a704567a98cc3178 Cr-Commit-Position: refs/heads/master@{#431931} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/72135ade1aacb9f9fcfe61f8a704567a98cc3178 Cr-Commit-Position: refs/heads/master@{#431931}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:300001) has been created in https://codereview.chromium.org/2496043004/ by horo@chromium.org. The reason for reverting is: Caused failures of AshTestHelperTest.AshTestHelper in ash_unittests. See http://crbug.com/665278 BUG=665278 . |