Hi David, I don't understand #1. Who owns this code now?

Hi, #1 indicates the issue is available for anyone to fix. The advantage of doing this way was it actually turns off the display entirely including touch.

Is there some way we could address your concerns regarding using the call?

Can you add more details about the requirements for darkenScreen so I can provide suggestions for better ways to do this? Alternately, is the accessibilityPrivate API documented anywhere?
chrome/common/extensions/api/accessibility_private.json doesn't have many details.

It sounds from #3 like you want to turn the display off and also disable the touchscreen. Should the touchpad be disabled too?

How long should this persist? If Chrome crashes and is restarted, should the display stay off or turn back on? (If it stays off, does Chrome have any way to know the state the display is in?)

If the user signs out, should the display stay off or turn on?

What about if the system reboots?

Would you like to code review the original change, since you object to the change in general and we can re-do the pieces you don't think work?

As for how the feature should work, that's in the change:
- keys are bound from an accessibility extension, ChromeVox to darken the screen (perhaps a better name is to disable the screen), but we're following naming conventions from other screen readers. I'm fine to rename the private api to disable screen. A separate keystroke re-enables the screen (undarken screen)
- if invoked, the screen should be turned off (or back on)
- if ChromeVox is toggled off, the screen should come back on
- on reboot, the screen is back on

Also, do you have a reproducable bug that I can fix here? I'm a little confused as to what you are asking for or the specific concerns.

Thanks for the details!

I'm traveling and don't have a system to test this on right now, but I'm pretty sure that something like the following will cause problems on a convertible Chromebook:

1. Use the ChromeVox hotkey to turn the screen off.
2. Tap the power button on the side of the device.
3. Tap the power button on the side of the device again.

At this point, I think the screen will be turned back on, since the tablet power button code doesn't expect anything else to be calling SetBacklightsForcedOff. Will this be unexpected for users?

Alternately, this sequence will probably also interact poorly:

1. Tap the power button.
2. Use ChromeVox to turn the screen off.
3. Tap the power button again.

Again, I think that the screen will be turned on now. If the user hits the ChromeVox hotkey at this point, I suspect that ChromeVox will try to turn the screen *on* (even though it's already on), since ChromeVox doesn't know that the power button code has already turned it on.

My concern isn't about how any of this is named; it's that the darkenScreen API and ash::PowerButtonDisplayController are each tracking the last state that they requested but unaware that the actual state might've been changed by each other in the meantime.

Let me know if that explanation makes sense. I'll think some more about possible ways to fix this.

I see. Thanks for explaining.

Currently, the darken screen api holds no state. 
    Review-Url: https://codereview.chromium.org/2768703002

Therefore, in ChromeVox, we had to bind two shortcuts (one to enable, one to disable), rather than a toggle.

You are right about the power button turning on the screen back on being unexpected for users. 

Uploaded https://crrev.com/c/815762.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/477c56da2daf91ce8cb1f8abacad62e70f73bde7

commit 477c56da2daf91ce8cb1f8abacad62e70f73bde7
Author: Daniel Erat <derat@chromium.org>
Date: Wed Dec 13 21:42:54 2017

chromeos: Make darkenScreen a11y API work with other callers

Update the chrome.accessibilityPrivate.darkenScreen API to
use ash::BacklightsForcedOffSetter to force backlights off
rather than calling powerd's SetBacklightsForcedOff D-Bus
method directly.

Otherwise, the backlights can get into an unexpected state
due to ChromeVox and ash's power button code both calling
the D-Bus method, which was only designed for a single
caller.

      Search+Shift+F7 darkens the screen and Search+F7
      undarkens it

Bug:  760730 ,  793112 
Test: enabled ChromeVox with Ctrl+Alt+Z and checked that
Change-Id: I6e5d2f11746ce7ae2e34987a413f2655747e9532
Reviewed-on: https://chromium-review.googlesource.com/815762
Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523890}
[modify] https://crrev.com/477c56da2daf91ce8cb1f8abacad62e70f73bde7/ash/shell.h
[modify] https://crrev.com/477c56da2daf91ce8cb1f8abacad62e70f73bde7/chrome/browser/accessibility/accessibility_extension_api.cc
[modify] https://crrev.com/477c56da2daf91ce8cb1f8abacad62e70f73bde7/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/477c56da2daf91ce8cb1f8abacad62e70f73bde7/chrome/browser/chromeos/accessibility/accessibility_manager.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7542fc382f738ee412dccf862d25a3bfa53e6495

commit 7542fc382f738ee412dccf862d25a3bfa53e6495
Author: Daniel Erat <derat@chromium.org>
Date: Wed Dec 13 23:56:36 2017

chromeos: Warn against calling SetBacklightsForcedOff.

Document that chromeos::PowerManagerClient's
SetBacklightsForcedOff method doesn't support multiple
callers, and that ash::BacklightsForcedOffSetter should be
used instead.

Bug:  760730 
Change-Id: Id083f9e1b3d1b252fa259ae519cf12c07135fa5f
Reviewed-on: https://chromium-review.googlesource.com/825948
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523934}
[modify] https://crrev.com/7542fc382f738ee412dccf862d25a3bfa53e6495/chromeos/dbus/power_manager_client.h