Add a color argument to accessibilityPrivate.setFocusRing

Add a color argument to accessibilityPrivate.setFocusRing

We're using the focus ring highlighting for multiple purposes now, this
gives us a way to use a different color for each one, or maybe make it a
user preference.

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

Review-Url: https://codereview.chromium.org/2789583004
Cr-Commit-Position: refs/heads/master@{#461499}
Committed: https://chromium.googlesource.com/chromium/src/+/0672582d53d0b7f561d129e28a30d5ae5cf28605

[dmazzoni, 2017-03-30 21:19:33]

Description was changed from

==========
Add a color argument to accessibilityPrivate.setFocusRing

We're using the focus ring highlighting for multiple purposes now, this
gives us a way to use a different color for each one, or maybe make it a
user preference.

BUG=593887
==========

to

==========
Add a color argument to accessibilityPrivate.setFocusRing

We're using the focus ring highlighting for multiple purposes now, this
gives us a way to use a different color for each one, or maybe make it a
user preference.

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

[dmazzoni, 2017-03-30 21:21:15]

dmazzoni@chromium.org changed reviewers:
+ dbeam@chromium.org, rdevlin.cronin@chromium.org, xiyuan@chromium.org

[dmazzoni, 2017-03-30 21:21:16]

xiyuan: chrome/browser/chromeos/ui
Devlin: extension API
dbeam: externs

[dmazzoni, 2017-03-30 21:21:31]

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

[commit-bot: I haz the power, 2017-03-30 21:22:09]

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

[xiyuan, 2017-03-30 21:27:39]

lgtm

https://codereview.chromium.org/2789583004/diff/1/chrome/browser/chromeos/ui/...
File chrome/browser/chromeos/ui/accessibility_focus_ring_controller.h (right):

https://codereview.chromium.org/2789583004/diff/1/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/accessibility_focus_ring_controller.h:31: void
SetFocusRingColor(SkColor color);
nit: #include "third_party/skia/include/core/SkColor.h"

https://codereview.chromium.org/2789583004/diff/1/chrome/browser/chromeos/ui/...
File chrome/browser/chromeos/ui/focus_ring_layer.cc (right):

https://codereview.chromium.org/2789583004/diff/1/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/focus_ring_layer.cc:34: compositor_(nullptr) {}
nit: move initialization to header since you are here

https://codereview.chromium.org/2789583004/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/accessibility_private.json (right):

https://codereview.chromium.org/2789583004/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/accessibility_private.json:66: {
replace tab with spaces

[dmazzoni, 2017-03-30 21:34:52]

https://codereview.chromium.org/2789583004/diff/1/chrome/browser/chromeos/ui/...
File chrome/browser/chromeos/ui/accessibility_focus_ring_controller.h (right):

https://codereview.chromium.org/2789583004/diff/1/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/accessibility_focus_ring_controller.h:31: void
SetFocusRingColor(SkColor color);
On 2017/03/30 21:27:38, xiyuan wrote:
> nit: #include "third_party/skia/include/core/SkColor.h"

Done.

https://codereview.chromium.org/2789583004/diff/1/chrome/browser/chromeos/ui/...
File chrome/browser/chromeos/ui/focus_ring_layer.cc (right):

https://codereview.chromium.org/2789583004/diff/1/chrome/browser/chromeos/ui/...
chrome/browser/chromeos/ui/focus_ring_layer.cc:34: compositor_(nullptr) {}
On 2017/03/30 21:27:38, xiyuan wrote:
> nit: move initialization to header since you are here

Done.

https://codereview.chromium.org/2789583004/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/accessibility_private.json (right):

https://codereview.chromium.org/2789583004/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/accessibility_private.json:66: {
On 2017/03/30 21:27:39, xiyuan wrote:
> replace tab with spaces

Done.

[Devlin, 2017-03-30 21:58:05]

note: I'm also an owner of externs, so Dan's optional here if we wanted to lower
his review load.

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

https://codereview.chromium.org/2789583004/diff/20001/chrome/browser/accessib...
chrome/browser/accessibility/accessibility_extension_api.cc:89:
EXTENSION_FUNCTION_VALIDATE(
This will result in killing the renderer if the string was invalid.  That
*might* be the right thing to do, since this is a private API, but it'd make me
nervous.  Is there a reason to not just return an error if the string is
invalid?

https://codereview.chromium.org/2789583004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/accessibility_focus_ring_controller.h (right):

https://codereview.chromium.org/2789583004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/ui/accessibility_focus_ring_controller.h:108: bool
has_focus_ring_color_ = false;
Drive-by: if you use a base::Optional<SkColor> for focus_ring_color_, you don't
have to have this extra member.

[dmazzoni, 2017-03-31 19:49:35]

Description was changed from

==========
Add a color argument to accessibilityPrivate.setFocusRing

We're using the focus ring highlighting for multiple purposes now, this
gives us a way to use a different color for each one, or maybe make it a
user preference.

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

to

==========
Add a color argument to accessibilityPrivate.setFocusRing

We're using the focus ring highlighting for multiple purposes now, this
gives us a way to use a different color for each one, or maybe make it a
user preference.

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

[dmazzoni, 2017-03-31 19:49:35]

dmazzoni@chromium.org changed reviewers:
- dbeam@chromium.org

[dmazzoni, 2017-03-31 20:24:05]

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

https://codereview.chromium.org/2789583004/diff/20001/chrome/browser/accessib...
chrome/browser/accessibility/accessibility_extension_api.cc:89:
EXTENSION_FUNCTION_VALIDATE(
On 2017/03/30 21:58:05, Devlin wrote:
> This will result in killing the renderer if the string was invalid.  That
> *might* be the right thing to do, since this is a private API, but it'd make
me
> nervous.  Is there a reason to not just return an error if the string is
> invalid?

Good idea, changed it to just return an error.

https://codereview.chromium.org/2789583004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/ui/accessibility_focus_ring_controller.h (right):

https://codereview.chromium.org/2789583004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/ui/accessibility_focus_ring_controller.h:108: bool
has_focus_ring_color_ = false;
On 2017/03/30 21:58:05, Devlin wrote:
> Drive-by: if you use a base::Optional<SkColor> for focus_ring_color_, you
don't
> have to have this extra member.

Good idea! That's definitely cleaner.

[dmazzoni, 2017-03-31 20:24:42]

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

[commit-bot: I haz the power, 2017-03-31 20:25:12]

Dry run: 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-31 21:37:41]

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

[commit-bot: I haz the power, 2017-03-31 21:37:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-04-03 17:11:27]

It looks like there are a few failing tests, but assuming there's no big
changes, extensions + externs lgtm

[dmazzoni, 2017-04-03 17:23:33]

Thanks! The test failures are from the dependent
patchset, which have since been fixed.

[dmazzoni, 2017-04-03 17:23:52]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-04-03 17:23:53]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2789583004/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-03 17:24:22]

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-03 19:22:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491240232789550,
"parent_rev": "f406bd367443f345db66f72c5d42ca1a7019ba80", "commit_rev":
"0672582d53d0b7f561d129e28a30d5ae5cf28605"}

[commit-bot: I haz the power, 2017-04-03 19:23:43]

Description was changed from

==========
Add a color argument to accessibilityPrivate.setFocusRing

We're using the focus ring highlighting for multiple purposes now, this
gives us a way to use a different color for each one, or maybe make it a
user preference.

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

to

==========
Add a color argument to accessibilityPrivate.setFocusRing

We're using the focus ring highlighting for multiple purposes now, this
gives us a way to use a different color for each one, or maybe make it a
user preference.

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

Review-Url: https://codereview.chromium.org/2789583004
Cr-Commit-Position: refs/heads/master@{#461499}
Committed:
https://chromium.googlesource.com/chromium/src/+/0672582d53d0b7f561d129e28a30...
==========

[commit-bot: I haz the power, 2017-04-03 19:23:45]

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