Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(63)

Issue 2474913004: Tablet-like power button behavior on Convertible/Tablet ChromeOS devices (Closed)

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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+702 lines, -87 lines) Patch
M ash/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -0 lines 0 comments Download
A ash/system/chromeos/power/tablet_power_button_controller.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +122 lines, -0 lines 0 comments Download
A ash/system/chromeos/power/tablet_power_button_controller.cc View 1 2 3 4 5 6 7 1 chunk +212 lines, -0 lines 0 comments Download
A ash/system/chromeos/power/tablet_power_button_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +306 lines, -0 lines 0 comments Download
M ash/wm/lock_state_controller.h View 2 chunks +0 lines, -2 lines 0 comments Download
M ash/wm/lock_state_controller_unittest.cc View 1 2 3 4 5 6 1 chunk +0 lines, -39 lines 0 comments Download
M ash/wm/power_button_controller.h View 1 2 3 4 5 6 7 8 9 5 chunks +14 lines, -13 lines 0 comments Download
M ash/wm/power_button_controller.cc View 1 2 3 4 5 6 7 8 9 8 chunks +34 lines, -32 lines 0 comments Download
M chromeos/dbus/fake_power_manager_client.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M chromeos/dbus/fake_power_manager_client.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -1 line 0 comments Download

Messages

Total messages: 48 (28 generated)
Qiang(Joe) Xu
Hi Daniel, I would like to send this out and please see if it is ...
4 years, 1 month ago (2016-11-03 18:43:45 UTC) #3
Daniel Erat
some initial comments. i haven't looked at the tests https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_controller.cc File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_controller.cc#newcode73 ash/wm/power_button_controller.cc:73: ...
4 years, 1 month ago (2016-11-03 21:28:08 UTC) #8
Qiang(Joe) Xu
Hi Daniel, patch uploaded, please take a look, thanks. New changes are based on comments. ...
4 years, 1 month ago (2016-11-04 01:14:04 UTC) #9
Daniel Erat
https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_controller.cc File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_controller.cc#newcode258 ash/wm/power_button_controller.cc:258: pressed_on_screen_off_ = true; On 2016/11/04 01:14:03, Qiang (Joe) Xu ...
4 years, 1 month ago (2016-11-04 16:12:52 UTC) #10
Qiang(Joe) Xu
Hi Daniel, thanks for reviewing. Here is the new patch and the reply. https://codereview.chromium.org/2474913004/diff/1/ash/wm/power_button_controller.cc File ...
4 years, 1 month ago (2016-11-04 19:29:52 UTC) #11
Daniel Erat
https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_controller.cc File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_controller.cc#newcode253 ash/wm/power_button_controller.cc:253: void PowerButtonController::OnTabletPowerButtonEvent( hmm... so i've noticed that the tablet ...
4 years, 1 month ago (2016-11-04 20:18:34 UTC) #14
Qiang(Joe) Xu
Hi Daniel, attached the CL based on the comments. Thanks! https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_controller.cc File ash/wm/power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/40001/ash/wm/power_button_controller.cc#newcode253 ...
4 years, 1 month ago (2016-11-04 23:18:21 UTC) #15
Daniel Erat
thanks for moving the code into a new class! it's much easier to understand now. ...
4 years, 1 month ago (2016-11-07 20:54:42 UTC) #16
Qiang(Joe) Xu
Hi Daniel, new patch change contains the below ones in addition to your comments. (1) ...
4 years, 1 month ago (2016-11-10 22:18:45 UTC) #19
Daniel Erat
https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/power/tablet_power_button_controller.cc File ash/system/chromeos/power/tablet_power_button_controller.cc (right): https://codereview.chromium.org/2474913004/diff/160001/ash/system/chromeos/power/tablet_power_button_controller.cc#newcode27 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/power/tablet_power_button_controller.cc#newcode61 ash/system/chromeos/power/tablet_power_button_controller.cc:61: ...
4 years, 1 month ago (2016-11-10 22:48:55 UTC) #22
Qiang(Joe) Xu
Hi Daniel, new patch uploaded. Changes mostly focused on comments. Add test coverage for pressing ...
4 years, 1 month ago (2016-11-11 04:39:23 UTC) #25
Daniel Erat
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 ...
4 years, 1 month ago (2016-11-11 05:40:51 UTC) #26
Qiang(Joe) Xu
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): ...
4 years, 1 month ago (2016-11-11 18:19:31 UTC) #31
Daniel Erat
just a few comments left https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/power/tablet_power_button_controller.h File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/power/tablet_power_button_controller.h#newcode25 ash/system/chromeos/power/tablet_power_button_controller.h:25: class ASH_EXPORT TabletPowerButtonController nit: ...
4 years, 1 month ago (2016-11-14 16:15:31 UTC) #38
Qiang(Joe) Xu
Hi Daniel, new patch is uploaded, ptal, thanks https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/power/tablet_power_button_controller.h File ash/system/chromeos/power/tablet_power_button_controller.h (right): https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/power/tablet_power_button_controller.h#newcode25 ash/system/chromeos/power/tablet_power_button_controller.h:25: class ...
4 years, 1 month ago (2016-11-14 18:47:57 UTC) #39
Daniel Erat
lgtm https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/power/tablet_power_button_controller_unittest.cc File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc (right): https://codereview.chromium.org/2474913004/diff/260001/ash/system/chromeos/power/tablet_power_button_controller_unittest.cc#newcode228 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: ...
4 years, 1 month ago (2016-11-14 19:22:50 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2474913004/300001
4 years, 1 month ago (2016-11-14 20:00:59 UTC) #43
commit-bot: I haz the power
Committed patchset #12 (id:300001)
4 years, 1 month ago (2016-11-14 22:48:12 UTC) #45
commit-bot: I haz the power
Patchset 12 (id:??) landed as https://crrev.com/72135ade1aacb9f9fcfe61f8a704567a98cc3178 Cr-Commit-Position: refs/heads/master@{#431931}
4 years, 1 month ago (2016-11-14 22:59:02 UTC) #47
horo
4 years, 1 month ago (2016-11-15 03:56:41 UTC) #48
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
.

Powered by Google App Engine
This is Rietveld 408576698