cros: Stop forcing off display when lid events are received

cros: Stop forcing off display when lid events are received

Changes:
Stop forcing off display when lid events are received

BUG=711033
TEST=added test coverage, also manually test on device

Review-Url: https://codereview.chromium.org/2815453007
Cr-Commit-Position: refs/heads/master@{#464469}
Committed: https://chromium.googlesource.com/chromium/src/+/1360898e06a4c3505237ccf3aa9425056e9e4af6

[Qiang(Joe) Xu, 2017-04-13 04:08:24]

Description was changed from

==========
cros: Stop forcing off display when lib events are received

BUG=711033
==========

to

==========
cros: Stop forcing off display when lib events are received

Changes:
Stop forcing off display when lib events are received

BUG=711033
TEST=added test coverage
==========

[Qiang(Joe) Xu, 2017-04-13 04:08:24]

warx@chromium.org changed reviewers:
+ derat@chromium.org

[Qiang(Joe) Xu, 2017-04-13 04:08:51]

Description was changed from

==========
cros: Stop forcing off display when lib events are received

Changes:
Stop forcing off display when lib events are received

BUG=711033
TEST=added test coverage
==========

to

==========
cros: Stop forcing off display when lid events are received

Changes:
Stop forcing off display when lid events are received

BUG=711033
TEST=added test coverage
==========

[Qiang(Joe) Xu, 2017-04-13 04:14:19]

Hi Dan, PTAL thanks

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
File ash/system/power/tablet_power_button_controller_unittest.cc (right):

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:500: // backlights.
For testing lid open event, generally speaking if backlights are forced off on
lid closed state. It will keep this state.

On Chrome side, only power button could do forcing off. But it should not
happen.

For testing this, we probably can expose SetDisplayForcedOff method through test
api. It should be served as testing purpose only. Shall we do that?

https://codereview.chromium.org/2815453007/diff/20001/chromeos/dbus/fake_powe...
File chromeos/dbus/fake_power_manager_client.h (right):

https://codereview.chromium.org/2815453007/diff/20001/chromeos/dbus/fake_powe...
chromeos/dbus/fake_power_manager_client.h:46: void set_lid_state(LidState state)
{ lid_state_ = state; }
set_lid_state is not used anywhere. Do we want to keep it?

[Qiang(Joe) Xu, 2017-04-13 04:14:44]

Description was changed from

==========
cros: Stop forcing off display when lid events are received

Changes:
Stop forcing off display when lid events are received

BUG=711033
TEST=added test coverage
==========

to

==========
cros: Stop forcing off display when lid events are received

Changes:
Stop forcing off display when lid events are received

BUG=711033
TEST=added test coverage, also manually test on device
==========

[Daniel Erat, 2017-04-13 04:31:17]

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
File ash/system/power/tablet_power_button_controller.cc (right):

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller.cc:169: state ==
chromeos::PowerManagerClient::LidState::CLOSED) {
you don't really need to test this; these are the only values that will ever be
passed.

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
File ash/system/power/tablet_power_button_controller_unittest.cc (right):

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:487: // Tests that
lid closed/open events can properly stop forcing off backlights.
nit: remove "can properly"

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:492:
EXPECT_TRUE(GetBacklightsForcedOff());
nit: ASSERT_TRUE since this isn't the thing that the test is trying to test, and
since the rest of the test will be useless if this doesn't work

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:500: // backlights.
On 2017/04/13 04:14:18, Qiang(Joe) Xu wrote:
> For testing lid open event, generally speaking if backlights are forced off on
> lid closed state. It will keep this state.
> 
> On Chrome side, only power button could do forcing off. But it should not
> happen.
> 
> For testing this, we probably can expose SetDisplayForcedOff method through
test
> api. It should be served as testing purpose only. Shall we do that?

hmm. can you just press and release the power button again to force the
backlights off here? i agree that it's something that shouldn't happen in real
life.

https://codereview.chromium.org/2815453007/diff/20001/chromeos/dbus/fake_powe...
File chromeos/dbus/fake_power_manager_client.h (right):

https://codereview.chromium.org/2815453007/diff/20001/chromeos/dbus/fake_powe...
chromeos/dbus/fake_power_manager_client.h:46: void set_lid_state(LidState state)
{ lid_state_ = state; }
On 2017/04/13 04:14:18, Qiang(Joe) Xu wrote:
> set_lid_state is not used anywhere. Do we want to keep it?

i think it's there so it can be used by future code that calls GetSwitchStates.
feel free to remove it since your new method can also be used to change the
state.

https://codereview.chromium.org/2815453007/diff/20001/chromeos/dbus/fake_powe...
chromeos/dbus/fake_power_manager_client.h:97: void SendLidEvent(LidState state,
const base::TimeTicks& timestamp);
how about renaming this to SetLidState? that makes it clearer that it's updating
this class's internal state as well.

[Qiang(Joe) Xu, 2017-04-13 04:47:54]

Patchset #3 (id:40001) has been deleted

[Qiang(Joe) Xu, 2017-04-13 04:49:40]

Updated, PTAL, thanks

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
File ash/system/power/tablet_power_button_controller.cc (right):

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller.cc:169: state ==
chromeos::PowerManagerClient::LidState::CLOSED) {
On 2017/04/13 04:31:16, Daniel Erat wrote:
> you don't really need to test this; these are the only values that will ever
be
> passed.

Done.

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
File ash/system/power/tablet_power_button_controller_unittest.cc (right):

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:487: // Tests that
lid closed/open events can properly stop forcing off backlights.
On 2017/04/13 04:31:16, Daniel Erat wrote:
> nit: remove "can properly"

Done.

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:492:
EXPECT_TRUE(GetBacklightsForcedOff());
On 2017/04/13 04:31:16, Daniel Erat wrote:
> nit: ASSERT_TRUE since this isn't the thing that the test is trying to test,
and
> since the rest of the test will be useless if this doesn't work

Done.

https://codereview.chromium.org/2815453007/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:500: // backlights.
On 2017/04/13 04:31:16, Daniel Erat wrote:
> On 2017/04/13 04:14:18, Qiang(Joe) Xu wrote:
> > For testing lid open event, generally speaking if backlights are forced off
on
> > lid closed state. It will keep this state.
> > 
> > On Chrome side, only power button could do forcing off. But it should not
> > happen.
> > 
> > For testing this, we probably can expose SetDisplayForcedOff method through
> test
> > api. It should be served as testing purpose only. Shall we do that?
> 
> hmm. can you just press and release the power button again to force the
> backlights off here? i agree that it's something that shouldn't happen in real
> life.

Done.

https://codereview.chromium.org/2815453007/diff/20001/chromeos/dbus/fake_powe...
File chromeos/dbus/fake_power_manager_client.h (right):

https://codereview.chromium.org/2815453007/diff/20001/chromeos/dbus/fake_powe...
chromeos/dbus/fake_power_manager_client.h:46: void set_lid_state(LidState state)
{ lid_state_ = state; }
On 2017/04/13 04:31:16, Daniel Erat wrote:
> On 2017/04/13 04:14:18, Qiang(Joe) Xu wrote:
> > set_lid_state is not used anywhere. Do we want to keep it?
> 
> i think it's there so it can be used by future code that calls
GetSwitchStates.
> feel free to remove it since your new method can also be used to change the
> state.

Done.

https://codereview.chromium.org/2815453007/diff/20001/chromeos/dbus/fake_powe...
chromeos/dbus/fake_power_manager_client.h:97: void SendLidEvent(LidState state,
const base::TimeTicks& timestamp);
On 2017/04/13 04:31:16, Daniel Erat wrote:
> how about renaming this to SetLidState? that makes it clearer that it's
updating
> this class's internal state as well.

agree. Done.

[Daniel Erat, 2017-04-13 05:10:03]

lgtm w/nits

https://codereview.chromium.org/2815453007/diff/30006/ash/system/power/tablet...
File ash/system/power/tablet_power_button_controller_unittest.cc (right):

https://codereview.chromium.org/2815453007/diff/30006/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:506: // A lid open
event is received, we should get stopped forcing off backlights.
nit: s/get stopped/stop/

https://codereview.chromium.org/2815453007/diff/30006/chromeos/dbus/fake_powe...
File chromeos/dbus/fake_power_manager_client.h (right):

https://codereview.chromium.org/2815453007/diff/30006/chromeos/dbus/fake_powe...
chromeos/dbus/fake_power_manager_client.h:94: // Sets LidState and notifies
observers that LidState is switched to |state|.
nit: Sets |lid_state_| and notifies |observers_| about the change.

[Qiang(Joe) Xu, 2017-04-13 17:08:47]

done. Thanks for review.

https://codereview.chromium.org/2815453007/diff/30006/ash/system/power/tablet...
File ash/system/power/tablet_power_button_controller_unittest.cc (right):

https://codereview.chromium.org/2815453007/diff/30006/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:506: // A lid open
event is received, we should get stopped forcing off backlights.
On 2017/04/13 05:10:03, Daniel Erat wrote:
> nit: s/get stopped/stop/

Done.

https://codereview.chromium.org/2815453007/diff/30006/chromeos/dbus/fake_powe...
File chromeos/dbus/fake_power_manager_client.h (right):

https://codereview.chromium.org/2815453007/diff/30006/chromeos/dbus/fake_powe...
chromeos/dbus/fake_power_manager_client.h:94: // Sets LidState and notifies
observers that LidState is switched to |state|.
On 2017/04/13 05:10:03, Daniel Erat wrote:
> nit: Sets |lid_state_| and notifies |observers_| about the change.

Done.

[Qiang(Joe) Xu, 2017-04-13 17:08:53]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-04-13 17:08:54]

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/2815453007/#ps70001
(title: "nits")

[commit-bot: I haz the power, 2017-04-13 17:09:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 18:08:34]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1492103333973210,
"parent_rev": "b14cc10ccf83caad627156403239fc6758c2dbbb", "commit_rev":
"1360898e06a4c3505237ccf3aa9425056e9e4af6"}

[commit-bot: I haz the power, 2017-04-13 18:09:26]

Description was changed from

==========
cros: Stop forcing off display when lid events are received

Changes:
Stop forcing off display when lid events are received

BUG=711033
TEST=added test coverage, also manually test on device
==========

to

==========
cros: Stop forcing off display when lid events are received

Changes:
Stop forcing off display when lid events are received

BUG=711033
TEST=added test coverage, also manually test on device

Review-Url: https://codereview.chromium.org/2815453007
Cr-Commit-Position: refs/heads/master@{#464469}
Committed:
https://chromium.googlesource.com/chromium/src/+/1360898e06a4c3505237ccf3aa94...
==========

[commit-bot: I haz the power, 2017-04-13 18:09:27]

Committed patchset #4 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/1360898e06a4c3505237ccf3aa94...