cros: Update touchscreen status with backlights forced off state during start

cros: Update touchscreen status with backlights forced off state during start

Changes:
Sync powerd's backlights forced off state with touchscreen local pref and update touchscreen.

BUG=709209
TEST=added test coverage

Review-Url: https://codereview.chromium.org/2855033004
Cr-Commit-Position: refs/heads/master@{#468895}
Committed: https://chromium.googlesource.com/chromium/src/+/750b068d2182bbb0bafe9c236dae7aea02f0431e

[Qiang(Joe) Xu, 2017-05-03 00:37:15]

Description was changed from

==========
cros: Update touchscreen status with backlights forced off state

BUG=709209
TEST=none
==========

to

==========
cros: Update touchscreen status with backlights forced off state during start

Changes:
Sync powerd's backlights forced off state with touchscreen local pref and update
touchscreen.

BUG=709209
TEST=none
==========

[Qiang(Joe) Xu, 2017-05-03 00:37:16]

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

[Qiang(Joe) Xu, 2017-05-03 00:39:28]

PTAL, thanks!

I don't have a servo setup, so I didn't test it. I believe this is the root
cause if powerd does change state in its side. Overall, it should guarantee the
local pref is in correct phase.

[Daniel Erat, 2017-05-03 00:48:47]

please add a unit test for this.

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
File ash/system/power/tablet_power_button_controller.cc (right):

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
ash/system/power/tablet_power_button_controller.cc:58: // Update Touchscreen
status based on the provided |local_pref_enabled|, which
nit: s/Touchscreen/touchscreen/

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
ash/system/power/tablet_power_button_controller.cc:59: // is a reverse state of
|backlights_forced_off_|.
instead of referring to backlights_forced_off_ (which doesn't really make sense
in this anon function), how about something like this?

  // Enables or disables the touchscreen, also writing its state
  // to a pref in local state. The touchscreen is disabled when
  // backlights are forced off.

alternately, would it be cleaner for this to just be a private non-static method
in TabletPowerButtonController that doesn't take a param and instead just uses
backlights_forced_off_ to figure out what it should do?

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
ash/system/power/tablet_power_button_controller.cc:60: void
UpdateTouchscreenStatus(bool local_pref_enabled) {
local_pref_enabled is a bit confusing. can you rename it to just be "enabled"?

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
ash/system/power/tablet_power_button_controller.cc:232: 
nit: delete blank line?

[Qiang(Joe) Xu, 2017-05-03 03:14:22]

Hi Dan, ptal, thanks!

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
File ash/system/power/tablet_power_button_controller.cc (right):

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
ash/system/power/tablet_power_button_controller.cc:58: // Update Touchscreen
status based on the provided |local_pref_enabled|, which
On 2017/05/03 00:48:46, Daniel Erat wrote:
> nit: s/Touchscreen/touchscreen/

Done.

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
ash/system/power/tablet_power_button_controller.cc:59: // is a reverse state of
|backlights_forced_off_|.
On 2017/05/03 00:48:46, Daniel Erat wrote:
> instead of referring to backlights_forced_off_ (which doesn't really make
sense
> in this anon function), how about something like this?
> 
>   // Enables or disables the touchscreen, also writing its state
>   // to a pref in local state. The touchscreen is disabled when
>   // backlights are forced off.
> 
> alternately, would it be cleaner for this to just be a private non-static
method
> in TabletPowerButtonController that doesn't take a param and instead just uses
> backlights_forced_off_ to figure out what it should do?

done by moving this to private method. That is better. Thanks!

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
ash/system/power/tablet_power_button_controller.cc:60: void
UpdateTouchscreenStatus(bool local_pref_enabled) {
On 2017/05/03 00:48:46, Daniel Erat wrote:
> local_pref_enabled is a bit confusing. can you rename it to just be "enabled"?

Done.

https://codereview.chromium.org/2855033004/diff/1/ash/system/power/tablet_pow...
ash/system/power/tablet_power_button_controller.cc:232: 
On 2017/05/03 00:48:46, Daniel Erat wrote:
> nit: delete blank line?

Done.

[Daniel Erat, 2017-05-03 03:28:39]

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

https://codereview.chromium.org/2855033004/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:517:
ASSERT_FALSE(shell_delegate_->IsTouchscreenEnabledInPrefs(true));
i'm worried that this isn't testing the real case. it'd be more realistic to not
do this in the test and instead set the local state pref before the
TabletPowerButtonController is initialized.

[Qiang(Joe) Xu, 2017-05-03 03:53:27]

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

https://codereview.chromium.org/2855033004/diff/20001/ash/system/power/tablet...
ash/system/power/tablet_power_button_controller_unittest.cc:517:
ASSERT_FALSE(shell_delegate_->IsTouchscreenEnabledInPrefs(true));
On 2017/05/03 03:28:39, Daniel Erat wrote:
> i'm worried that this isn't testing the real case. it'd be more realistic to
not
> do this in the test and instead set the local state pref before the
> TabletPowerButtonController is initialized.

makes sense. Updated.

[Qiang(Joe) Xu, 2017-05-03 03:53:46]

Description was changed from

==========
cros: Update touchscreen status with backlights forced off state during start

Changes:
Sync powerd's backlights forced off state with touchscreen local pref and update
touchscreen.

BUG=709209
TEST=none
==========

to

==========
cros: Update touchscreen status with backlights forced off state during start

Changes:
Sync powerd's backlights forced off state with touchscreen local pref and update
touchscreen.

BUG=709209
TEST=added test coverage
==========

[Daniel Erat, 2017-05-03 03:59:37]

lgtm

https://codereview.chromium.org/2855033004/diff/40001/ash/wm/power_button_con...
File ash/wm/power_button_controller.h (right):

https://codereview.chromium.org/2855033004/diff/40001/ash/wm/power_button_con...
ash/wm/power_button_controller.h:55: void
ResetTabletPowerButtonControllerForTest();
nit: document what this does, e.g.

// Resets |tablet_controller_| to hold a new object to simulate
// Chrome starting.

[Qiang(Joe) Xu, 2017-05-03 04:17:34]

Thanks for review!

https://codereview.chromium.org/2855033004/diff/40001/ash/wm/power_button_con...
File ash/wm/power_button_controller.h (right):

https://codereview.chromium.org/2855033004/diff/40001/ash/wm/power_button_con...
ash/wm/power_button_controller.h:55: void
ResetTabletPowerButtonControllerForTest();
On 2017/05/03 03:59:37, Daniel Erat wrote:
> nit: document what this does, e.g.
> 
> // Resets |tablet_controller_| to hold a new object to simulate
> // Chrome starting.

Done.

[Qiang(Joe) Xu, 2017-05-03 04:17:39]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-05-03 04:17:40]

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

[commit-bot: I haz the power, 2017-05-03 04:17:53]

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-05-03 04:53:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493785059884430,
"parent_rev": "aaa7094990b832b7fc7c8aa9b650dd4285cc313c", "commit_rev":
"750b068d2182bbb0bafe9c236dae7aea02f0431e"}

[commit-bot: I haz the power, 2017-05-03 04:53:29]

Description was changed from

==========
cros: Update touchscreen status with backlights forced off state during start

Changes:
Sync powerd's backlights forced off state with touchscreen local pref and update
touchscreen.

BUG=709209
TEST=added test coverage
==========

to

==========
cros: Update touchscreen status with backlights forced off state during start

Changes:
Sync powerd's backlights forced off state with touchscreen local pref and update
touchscreen.

BUG=709209
TEST=added test coverage

Review-Url: https://codereview.chromium.org/2855033004
Cr-Commit-Position: refs/heads/master@{#468895}
Committed:
https://chromium.googlesource.com/chromium/src/+/750b068d2182bbb0bafe9c236dae...
==========

[commit-bot: I haz the power, 2017-05-03 04:53:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/750b068d2182bbb0bafe9c236dae...