Wire up an api to darken screen for accessibility

Wire up an api to darken screen for accessibility

BUG=625741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2768703002
Cr-Commit-Position: refs/heads/master@{#459646}
Committed: https://chromium.googlesource.com/chromium/src/+/3a17f21225fc435f31e588292073eab3e4952502

[David Tseng, 2017-03-21 23:49:13]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[David Tseng, 2017-03-22 16:18:15]

Description was changed from

==========
Wire up an api to darken screen for accessibility

BUG=
==========

to

==========
Wire up an api to darken screen for accessibility

BUG=625741
==========

[David Tseng, 2017-03-22 20:25:49]

Description was changed from

==========
Wire up an api to darken screen for accessibility

BUG=625741
==========

to

==========
Wire up an api to darken screen for accessibility

BUG=625741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2017-03-22 21:40:49]

What do you think about accessibilityPrivate instead
of automation?

Arguments being:

* We probably don't want to expose this to other
  developers, even AT developers
* We don't really have a place in the automation API
  for global actions, as opposed to actions that apply
  to a particular node
* It's Chrome OS only, whereas almost everything else in
  Automation works on almost all platforms, or at least
  we hope it could work on other platforms someday

https://codereview.chromium.org/2768703002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js
(right):

https://codereview.chromium.org/2768703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:197:
desktop.toggleDarkenScreen(false);
Why call it twice?

https://codereview.chromium.org/2768703002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/automation_internal.idl (right):

https://codereview.chromium.org/2768703002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/automation_internal.idl:85: boolean darkenScreen;
Passing a bool doesn't really match "toggle". How about calling the
action setScreenDarkening instead?

[David Tseng, 2017-03-22 22:51:38]

Just responding to a few comments.

This api really belongs in systemPrivate, but that appears to have been pulled a
while ago without actually removing the impl files.


I actually think this api belongs on the desktop node exclusively. At the
omment, I only install the api on root nodes so not every node gets this api.

I'd like to move towards having a set of global api's only available on the
desktop node.

https://codereview.chromium.org/2768703002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js
(right):

https://codereview.chromium.org/2768703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:197:
desktop.toggleDarkenScreen(false);
On 2017/03/22 21:40:48, dmazzoni wrote:
> Why call it twice?

The other end of the dbus message is sticky. So, if you turned backlights off in
the past and adjusted brightness, the force backlights flag is still off, but
the screen is on. To get back to a known state, we need to clear the previous
state.

[chromium-reviews, 2017-03-24 15:46:21]

Ptal
dmazzoni@chromium.org writes:

> What do you think about accessibilityPrivate instead
> of automation?
>
> Arguments being:
>
> * We probably don't want to expose this to other
>    developers, even AT developers
> * We don't really have a place in the automation API
>    for global actions, as opposed to actions that apply
>    to a particular node
> * It's Chrome OS only, whereas almost everything else in
>    Automation works on almost all platforms, or at least
>    we hope it could work on other platforms someday
>

The idea originally was to have the desktop node contain the current
darkened state. In the course of writing the cl, that looks to be
stateful or async.

The other argument for placing on a node is it gives us flexibility in
the future (e.g. what happens in multi screen scenarios)?

For the sake of simplifying things, I moved the api to
accessibilityPrivate and did some amount of externs cleanup.

>
>
>
https://codereview.chromium.org/2768703002/diff/20001/chrome/browser/resource...
> File
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js
> (right):
>
>
https://codereview.chromium.org/2768703002/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:197:
> desktop.toggleDarkenScreen(false);
> Why call it twice?
>

Added comment.

>
https://codereview.chromium.org/2768703002/diff/20001/chrome/common/extension...
> File chrome/common/extensions/api/automation_internal.idl (right):
>
>
https://codereview.chromium.org/2768703002/diff/20001/chrome/common/extension...
> chrome/common/extensions/api/automation_internal.idl:85: boolean
> darkenScreen;
> Passing a bool doesn't really match "toggle". How about calling the
> action setScreenDarkening instead?
>

How about just |darkenScreen|. Fits the other actions (verb + noun).


> https://codereview.chromium.org/2768703002/


-- 

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2017-03-24 16:02:40]

lgtm with a couple of suggestions

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/accessib...
File chrome/browser/accessibility/accessibility_extension_api.cc (right):

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/accessib...
chrome/browser/accessibility/accessibility_extension_api.cc:142: bool enabled;
How about "darken"

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js
(right):

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:196:
// Called twice to ensure the cros end of the dbus message is in a good
I just think that detail should be handled lower down
the stack. Just call chrome.accessibilityPrivate.darkenScreen
once here, and then have accessibility_extension_api.cc call
it twice if necessary, with the comment here.

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:2711:
<message desc="Describes a command to darken the screen."
name="IDS_CHROMEVOX_DARKEN_SCREEN">
Maybe explain it in case "darken" isn't clear. How about: Describes a command to
make the screen black.

[David Tseng, 2017-03-24 18:07:42]

dtseng@chromium.org changed reviewers:
+ isherman@chromium.org, michaelpg@chromium.org, rockot@chromium.org

[David Tseng, 2017-03-24 18:07:43]

+ isherman for *historgram*
+ michaelpg for t/c/externs/accessibility_private.js
+ rockot for c/c/e/api/accessibility_private.json

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/accessib...
File chrome/browser/accessibility/accessibility_extension_api.cc (right):

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/accessib...
chrome/browser/accessibility/accessibility_extension_api.cc:142: bool enabled;
On 2017/03/24 16:02:40, dmazzoni wrote:
> How about "darken"

Done.

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js
(right):

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:196:
// Called twice to ensure the cros end of the dbus message is in a good
On 2017/03/24 16:02:40, dmazzoni wrote:
> I just think that detail should be handled lower down
> the stack. Just call chrome.accessibilityPrivate.darkenScreen
> once here, and then have accessibility_extension_api.cc call
> it twice if necessary, with the comment here.

Ok, I see this as a one off api, so fine either way though this will make
debugging a little harder at runtime.

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:2711:
<message desc="Describes a command to darken the screen."
name="IDS_CHROMEVOX_DARKEN_SCREEN">
On 2017/03/24 16:02:40, dmazzoni wrote:
> Maybe explain it in case "darken" isn't clear. How about: Describes a command
to
> make the screen black.

How about turns the visual display on/off?

[Ken Rockot(use gerrit already), 2017-03-24 19:21:06]

lgtm

[michaelpg, 2017-03-24 21:13:41]

lgtm for externs

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:2711:
<message desc="Describes a command to darken the screen."
name="IDS_CHROMEVOX_DARKEN_SCREEN">
On 2017/03/24 18:07:43, David Tseng wrote:
> On 2017/03/24 16:02:40, dmazzoni wrote:
> > Maybe explain it in case "darken" isn't clear. How about: Describes a
command
> to
> > make the screen black.
> 
> How about turns the visual display on/off?

The string itself seems ambiguous too; I would expect "darken screen" to mean
"incrementally dim the screen". Are these set in stone or would "Turn off
screen/Turn on screen" work? (As a side note, "turn on" and "turn off" are hard
to translate, but the description makes it clear.)

[dmazzoni, 2017-03-24 21:24:37]

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/2768703002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:2711:
<message desc="Describes a command to darken the screen."
name="IDS_CHROMEVOX_DARKEN_SCREEN">
On 2017/03/24 18:07:43, David Tseng wrote:
> On 2017/03/24 16:02:40, dmazzoni wrote:
> > Maybe explain it in case "darken" isn't clear. How about: Describes a
command
> to
> > make the screen black.
> 
> How about turns the visual display on/off?

Sounds good

[Ilya Sherman, 2017-03-24 21:28:31]

https://codereview.chromium.org/2768703002/diff/100001/extensions/browser/ext...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/2768703002/diff/100001/extensions/browser/ext...
extensions/browser/extension_function_histogram_value.h:1232: // python
tools/metrics/histograms/update_extension_histograms.py
^^^

[David Tseng, 2017-03-24 21:54:39]

https://codereview.chromium.org/2768703002/diff/100001/extensions/browser/ext...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/2768703002/diff/100001/extensions/browser/ext...
extensions/browser/extension_function_histogram_value.h:1232: // python
tools/metrics/histograms/update_extension_histograms.py
On 2017/03/24 21:28:29, Ilya Sherman wrote:
> ^^^

Done.

[David Tseng, 2017-03-24 21:56:31]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 21:57:17]

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

[Ilya Sherman, 2017-03-24 22:02:47]

histograms.xml lgtm

[commit-bot: I haz the power, 2017-03-24 23:18:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 23:19:01]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-03-25 00:34:08]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-03-25 00:34:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, rockot@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2768703002/#ps120001
(title: "Run python script.")

[commit-bot: I haz the power, 2017-03-25 00:34: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-03-25 00:39:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-25 00:40:04]

Failed to apply patch for
chrome/browser/chromeos/accessibility/accessibility_manager.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/chromeos/accessibility/accessibility_manager.cc:58
  error: chrome/browser/chromeos/accessibility/accessibility_manager.cc: patch
does not apply

Patch:       chrome/browser/chromeos/accessibility/accessibility_manager.cc
Index: chrome/browser/chromeos/accessibility/accessibility_manager.cc
diff --git a/chrome/browser/chromeos/accessibility/accessibility_manager.cc
b/chrome/browser/chromeos/accessibility/accessibility_manager.cc
index
23b1832d56a59308d33986e8cf662ada86ec7e08..b90112b04441da6d23139508ca4bfc14f505e56b
100644
--- a/chrome/browser/chromeos/accessibility/accessibility_manager.cc
+++ b/chrome/browser/chromeos/accessibility/accessibility_manager.cc
@@ -58,6 +58,8 @@
 #include "chrome/grit/browser_resources.h"
 #include "chromeos/audio/audio_a11y_controller.h"
 #include "chromeos/audio/chromeos_sounds.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/power_manager_client.h"
 #include "components/prefs/pref_member.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/browser_accessibility_state.h"
@@ -1361,6 +1363,11 @@ void AccessibilityManager::PostUnloadChromeVox() {
     chromevox_panel_->Close();
     chromevox_panel_ = nullptr;
   }
+
+  // In case the user darkened the screen, undarken it now.
+  chromeos::DBusThreadManager::Get()
+      ->GetPowerManagerClient()
+      ->SetBacklightsForcedOff(false);
 }
 
 void AccessibilityManager::PostSwitchChromeVoxProfile() {

[David Tseng, 2017-03-25 07:03:46]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2017-03-25 07:03:47]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
dmazzoni@chromium.org, isherman@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2768703002/#ps140001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-03-25 07:04:00]

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-03-25 08:58:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490425426962180,
"parent_rev": "fd745bdf5030fdab0ae54eb87c784b161206cd33", "commit_rev":
"3a17f21225fc435f31e588292073eab3e4952502"}

[commit-bot: I haz the power, 2017-03-25 09:00:30]

Description was changed from

==========
Wire up an api to darken screen for accessibility

BUG=625741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Wire up an api to darken screen for accessibility

BUG=625741
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2768703002
Cr-Commit-Position: refs/heads/master@{#459646}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a17f21225fc435f31e588292073...
==========

[commit-bot: I haz the power, 2017-03-25 09:00:31]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3a17f21225fc435f31e588292073...