Add support for faster, more limited accessibility modes.

Add support for faster, more limited accessibility modes.

See bug for context. This patch begins the implementation of
more limited accessibility modes by only fetching a subset of
accessibility attributes from Blink unless more are explicitly
needed.

Only Windows is affected initially. By default now when
accessibility support is enabled on Windows, only a few
key accessibility attributes are fetched, like role, state, name,
value, and location. As individual APIs are accessed that go
beyond these, Chrome's accessibility mode is automatically
changed to start fetching more.

Screen readers are unaffected by this change. I manually
tested JAWS and NVDA.

Password managers and single-sign-on software should
continue to work fine, but users should experience a
performance boost.

BUG=672205
Committed: https://crrev.com/368ea13e43a18dec62a7f72610398a2f9757b1c9
Cr-Commit-Position: refs/heads/master@{#439750}

[dmazzoni, 2016-12-14 23:19:43]

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

[commit-bot: I haz the power, 2016-12-14 23:20:32]

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, 2016-12-14 23:23:33]

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

[commit-bot: I haz the power, 2016-12-14 23:23:33]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dmazzoni, 2016-12-15 16:50:05]

Description was changed from

==========
Add support for partial accessibility modes.

BUG=672205
==========

to

==========
Add support for faster, more limited accessibility modes.

See bug for context. This patch begins the implementation of
more limited accessibility modes by only fetching a subset of
accessibility attributes from Blink unless more are explicitly
needed.

Only Windows is affected initially. By default now when
accessibility support is enabled on Windows, only a few
key accessibility attributes are fetched, like role, state, name,
value, and location. As individual APIs are accessed that go
beyond these, Chrome's accessibility mode is automatically
changed to start fetching more.

Screen readers are unaffected by this change. I manually
tested JAWS and NVDA.

Password managers and single-sign-on software should
continue to work fine, but users should experience a
performance boost.

BUG=672205
==========

[dmazzoni, 2016-12-15 16:50:57]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org, nektar@chromium.org

[dmazzoni, 2016-12-15 16:50:57]

Ready for an initial look. I need to add a test.

[aboxhall, 2016-12-15 17:37:29]

On 2016/12/15 16:50:57, dmazzoni wrote:
> Ready for an initial look. I need to add a test.

Generally looks good! Seems pretty straightforward. Exciting to see this
happening.

[dmazzoni, 2016-12-15 22:33:16]

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

[commit-bot: I haz the power, 2016-12-15 22:34:31]

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, 2016-12-15 22:53:07]

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

[commit-bot: I haz the power, 2016-12-15 22:53:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-12-16 19:14:27]

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

[commit-bot: I haz the power, 2016-12-16 19:15:34]

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, 2016-12-16 19:44:27]

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

[commit-bot: I haz the power, 2016-12-16 19:44:27]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dmazzoni, 2016-12-16 21:18:13]

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

[commit-bot: I haz the power, 2016-12-16 21:19:51]

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, 2016-12-16 21:49:50]

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

[commit-bot: I haz the power, 2016-12-16 21:49:51]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dmazzoni, 2016-12-16 22:01:44]

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

[dmazzoni, 2016-12-16 22:02:42]

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

[dmazzoni, 2016-12-16 22:02:42]

+isherman: metrics

Alice, Nektarios: I added some tests and this is ready
for complete review.

[commit-bot: I haz the power, 2016-12-16 22:03:33]

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, 2016-12-16 22:58:29]

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

[commit-bot: I haz the power, 2016-12-16 22:58:30]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ilya Sherman, 2016-12-17 00:57:12]

https://codereview.chromium.org/2581483002/diff/100001/content/browser/access...
File content/browser/accessibility/browser_accessibility_state_impl.cc (right):

https://codereview.chromium.org/2581483002/diff/100001/content/browser/access...
content/browser/accessibility/browser_accessibility_state_impl.cc:160:
UMA_AX_MODE_FLAG_MAX);
nit: Please factor out a function for recording to this histogram, invoked as
"RecordAccessibilityModeFlag(UMA_AX_MODE_FLAG_NATIVE_APIS)".

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:273: +    flag is flipped from false to
true.
Hmm, this seems like it might be a bit confusing to work with, if there are any
flags that users regularly toggle on and off again.  Is that a concern?

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:284: +    Accessibility.ModeFlag is
potentially more useful.
Is the new metrics more useful enough to where we should deprecate this one?

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:75242: +  <summary>Track individual
accessibility mdoe flags that are enabled.</summary>
nit: s/mdoe/mode

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:75243: +  <int value="0"
label="UMA_AX_MODE_FLAG_NATIVE_APIS">Native APIs</int>
nit: The "Native APIs" text is only shown on hover, and is pretty clear from the
label anyway.  I'd write this as either

  <int value="0" label="Native APIs"/>

or

  <int value="0" label="UMA_AX_MODE_FLAG_NATIVE_APIS"/>

[nektarios, 2016-12-17 21:58:27]

+  BrowserAccessibility* FindNode(ui::AXRole role,
+                                 const std::string& name) {
+    BrowserAccessibility* root = GetManager()->GetRoot();
+    CHECK(root);

I would instead do:
+    ASSERT_NE(nullptr, root);
+    ASSERT_EQ(nullptr, root);

+ private:
+  BrowserAccessibility* FindNodeInSubtree(
+      BrowserAccessibility& node,
+      ui::AXRole role,
+      const std::string& name) {

Nit. I would prefer to make the Node argument const as well.



In several places in
content/browser/accessibility/accessibility_mode_browsertest.cc I see:
+  EXPECT_TRUE(GetManager());
+  EXPECT_FALSE(GetManager());

Shouldn't this be
+  EXPECT_NE(nullptr, GetManager());


 static const int kAccessibilityHistogramDelaySecs = 45;
 Style guide says that new constants should use all caps. Not part of this patch
but I thought of mentioning it.
 
 
 
 +  AccessibilityMode previous_mode = accessibility_mode_;
   accessibility_mode_ |= mode;
+  if (accessibility_mode_ == previous_mode)
+    return;
+
+  AccessibilityMode new_mode_flags = accessibility_mode_ & (~previous_mode);
Please add a comment above the last line:
// Retrieve only the newly added modes for the purposes of logging.


   WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_GET_GROUP_POSITION);
+  AddAccessibilityModeFlags(ACCESSIBILITY_MODE_FLAG_SCREEN_READER);
   if (!instance_active())
     return E_FAIL;
 
Isn't it better to call |AddAccessibilityModeFlags| after checking if instance
is active?
It doesn't really matter for now, but it frightens me a bit if someone less
careful in the future adds logic in |AddAccessibilityModeFlags| that manipulates
the object without realizing that the instance is destroyed.
Up to you.


+    BrowserAccessibilityStateImpl::GetInstance()->AddAccessibilityModeFlags(
+        ACCESSIBILITY_MODE_FLAG_NATIVE_APIS |
+        ACCESSIBILITY_MODE_FLAG_WEB_CONTENTS);
     return static_cast<LRESULT>(0L);
   Isn't that S_OK? Not part of this patch but possible good cleanup.
   
   
   +  explicit RenderAccessibilityImpl(RenderFrameImpl* render_frame,
+                                   AccessibilityMode mode);
explicit no longer needed according to the C++ Style Guide. Only for single
argument constructors.


+  if (accessibility_mode_ & ACCESSIBILITY_MODE_FLAG_WEB_CONTENTS) {
+    LOG(ERROR) << "Mode: " << accessibility_mode_;
Will the log message be easily understood? Or do we need to print some text
representation of the bit flags.

[nektarios, 2016-12-17 21:58:42]

lgtm

[dmazzoni, 2016-12-19 22:38:01]

On 2016/12/17 21:58:27, nektarios wrote:
> +  BrowserAccessibility* FindNode(ui::AXRole role,
> +                                 const std::string& name) {
> +    BrowserAccessibility* root = GetManager()->GetRoot();
> +    CHECK(root);
> 
> I would instead do:
> +    ASSERT_NE(nullptr, root);
> +    ASSERT_EQ(nullptr, root);

It turns out that the ASSERT macros call return, so they only
work in helper functions that return void.

> + private:
> +  BrowserAccessibility* FindNodeInSubtree(
> +      BrowserAccessibility& node,
> +      ui::AXRole role,
> +      const std::string& name) {
> 
> Nit. I would prefer to make the Node argument const as well.

Done.

> In several places in
> content/browser/accessibility/accessibility_mode_browsertest.cc I see:
> +  EXPECT_TRUE(GetManager());
> +  EXPECT_FALSE(GetManager());
> 
> Shouldn't this be
> +  EXPECT_NE(nullptr, GetManager());

I've seen both, but comparing to nullptr could be more clear. Done.

>  static const int kAccessibilityHistogramDelaySecs = 45;
>  Style guide says that new constants should use all caps. Not part of this
patch
> but I thought of mentioning it.

Done.

>  +  AccessibilityMode previous_mode = accessibility_mode_;
>    accessibility_mode_ |= mode;
> +  if (accessibility_mode_ == previous_mode)
> +    return;
> +
> +  AccessibilityMode new_mode_flags = accessibility_mode_ & (~previous_mode);
> Please add a comment above the last line:
> // Retrieve only the newly added modes for the purposes of logging.

Done.

>    WIN_ACCESSIBILITY_API_HISTOGRAM(UMA_API_GET_GROUP_POSITION);
> +  AddAccessibilityModeFlags(ACCESSIBILITY_MODE_FLAG_SCREEN_READER);
>    if (!instance_active())
>      return E_FAIL;
>  
> Isn't it better to call |AddAccessibilityModeFlags| after checking if instance
> is active?
> It doesn't really matter for now, but it frightens me a bit if someone less
> careful in the future adds logic in |AddAccessibilityModeFlags| that
manipulates
> the object without realizing that the instance is destroyed.
> Up to you.

My thinking is that expressing the intent to call a certain API, even on
an object that happens to be invalid, is a pretty good signal that we
should enable support for that API.

>    +  explicit RenderAccessibilityImpl(RenderFrameImpl* render_frame,
> +                                   AccessibilityMode mode);
> explicit no longer needed according to the C++ Style Guide. Only for single
> argument constructors.

Good catch, done.

> 
> +  if (accessibility_mode_ & ACCESSIBILITY_MODE_FLAG_WEB_CONTENTS) {
> +    LOG(ERROR) << "Mode: " << accessibility_mode_;
> Will the log message be easily understood? Or do we need to print some text
> representation of the bit flags.

Removed, it was more for my own debugging.

[dmazzoni, 2016-12-19 22:51:51]

https://codereview.chromium.org/2581483002/diff/100001/content/browser/access...
File content/browser/accessibility/browser_accessibility_state_impl.cc (right):

https://codereview.chromium.org/2581483002/diff/100001/content/browser/access...
content/browser/accessibility/browser_accessibility_state_impl.cc:160:
UMA_AX_MODE_FLAG_MAX);
On 2016/12/17 00:57:12, Ilya Sherman wrote:
> nit: Please factor out a function for recording to this histogram, invoked as
> "RecordAccessibilityModeFlag(UMA_AX_MODE_FLAG_NATIVE_APIS)".

Done.

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:273: +    flag is flipped from false to
true.
On 2016/12/17 00:57:12, Ilya Sherman wrote:
> Hmm, this seems like it might be a bit confusing to work with, if there are
any
> flags that users regularly toggle on and off again.  Is that a concern?

I clarified in the comment that the flags are flipped on automatically,
and they can only be flipped off using an internal page, so it's not my goal to
track that right now. Perhaps I could also add an action.

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:284: +    Accessibility.ModeFlag is
potentially more useful.
On 2016/12/17 00:57:12, Ilya Sherman wrote:
> Is the new metrics more useful enough to where we should deprecate this one?

Sure, done. The main reason to keep it a bit longer is for a bit of continuity.
After a few releases we can stop tracking Accessibility.State.

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:75242: +  <summary>Track individual
accessibility mdoe flags that are enabled.</summary>
On 2016/12/17 00:57:12, Ilya Sherman wrote:
> nit: s/mdoe/mode

Done.

https://codereview.chromium.org/2581483002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:75243: +  <int value="0"
label="UMA_AX_MODE_FLAG_NATIVE_APIS">Native APIs</int>
On 2016/12/17 00:57:12, Ilya Sherman wrote:
> nit: The "Native APIs" text is only shown on hover, and is pretty clear from
the
> label anyway.  I'd write this as either
> 
>   <int value="0" label="Native APIs"/>
> 
> or
> 
>   <int value="0" label="UMA_AX_MODE_FLAG_NATIVE_APIS"/>

Sounds good, thanks.

[dmazzoni, 2016-12-19 22:52:57]

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

[dmazzoni, 2016-12-19 22:52:58]

+jam for owners review of:

content/browser/renderer_host/legacy_render_widget_host_win.cc
content/renderer/render_frame_impl.cc

[dmazzoni, 2016-12-19 22:54:01]

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

[commit-bot: I haz the power, 2016-12-19 22:54:48]

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

[Ilya Sherman, 2016-12-19 22:58:30]

Metrics LGTM, thanks!

https://codereview.chromium.org/2581483002/diff/140001/content/browser/access...
File content/browser/accessibility/browser_accessibility_state_impl.cc (right):

https://codereview.chromium.org/2581483002/diff/140001/content/browser/access...
content/browser/accessibility/browser_accessibility_state_impl.cc:138:
UMA_HISTOGRAM_BOOLEAN("Accessibility.State", IsAccessibleBrowser());
Please add a TODO to remove this histogram, along with a target milestone.

[dmazzoni, 2016-12-19 23:02:15]

https://codereview.chromium.org/2581483002/diff/140001/content/browser/access...
File content/browser/accessibility/browser_accessibility_state_impl.cc (right):

https://codereview.chromium.org/2581483002/diff/140001/content/browser/access...
content/browser/accessibility/browser_accessibility_state_impl.cc:138:
UMA_HISTOGRAM_BOOLEAN("Accessibility.State", IsAccessibleBrowser());
On 2016/12/19 22:58:29, Ilya Sherman wrote:
> Please add a TODO to remove this histogram, along with a target milestone.

Done.

[dmazzoni, 2016-12-19 23:02:19]

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

[commit-bot: I haz the power, 2016-12-19 23:03:06]

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

[jam, 2016-12-19 23:52:46]

lgtm

[commit-bot: I haz the power, 2016-12-20 00:46:53]

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

[commit-bot: I haz the power, 2016-12-20 00:46:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmazzoni, 2016-12-20 06:03:55]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-12-20 06:03:56]

The patchset sent to the CQ was uploaded after l-g-t-m from nektar@chromium.org,
isherman@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2581483002/#ps180001
(title: "Disable tests that can't pass on Android with TODO")

[commit-bot: I haz the power, 2016-12-20 06:04:23]

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, 2016-12-20 08:22:47]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1482213835758620,
"parent_rev": "991a775764a32fddaed55cda396fddfa2e652b6f", "commit_rev":
"bb463547fc111958af4d125fdcacb82eb7007a42"}

[commit-bot: I haz the power, 2016-12-20 08:24:13]

Description was changed from

==========
Add support for faster, more limited accessibility modes.

See bug for context. This patch begins the implementation of
more limited accessibility modes by only fetching a subset of
accessibility attributes from Blink unless more are explicitly
needed.

Only Windows is affected initially. By default now when
accessibility support is enabled on Windows, only a few
key accessibility attributes are fetched, like role, state, name,
value, and location. As individual APIs are accessed that go
beyond these, Chrome's accessibility mode is automatically
changed to start fetching more.

Screen readers are unaffected by this change. I manually
tested JAWS and NVDA.

Password managers and single-sign-on software should
continue to work fine, but users should experience a
performance boost.

BUG=672205
==========

to

==========
Add support for faster, more limited accessibility modes.

See bug for context. This patch begins the implementation of
more limited accessibility modes by only fetching a subset of
accessibility attributes from Blink unless more are explicitly
needed.

Only Windows is affected initially. By default now when
accessibility support is enabled on Windows, only a few
key accessibility attributes are fetched, like role, state, name,
value, and location. As individual APIs are accessed that go
beyond these, Chrome's accessibility mode is automatically
changed to start fetching more.

Screen readers are unaffected by this change. I manually
tested JAWS and NVDA.

Password managers and single-sign-on software should
continue to work fine, but users should experience a
performance boost.

BUG=672205

Review-Url: https://codereview.chromium.org/2581483002
==========

[commit-bot: I haz the power, 2016-12-20 08:24:14]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-12-20 08:29:06]

Description was changed from

==========
Add support for faster, more limited accessibility modes.

See bug for context. This patch begins the implementation of
more limited accessibility modes by only fetching a subset of
accessibility attributes from Blink unless more are explicitly
needed.

Only Windows is affected initially. By default now when
accessibility support is enabled on Windows, only a few
key accessibility attributes are fetched, like role, state, name,
value, and location. As individual APIs are accessed that go
beyond these, Chrome's accessibility mode is automatically
changed to start fetching more.

Screen readers are unaffected by this change. I manually
tested JAWS and NVDA.

Password managers and single-sign-on software should
continue to work fine, but users should experience a
performance boost.

BUG=672205

Review-Url: https://codereview.chromium.org/2581483002
==========

to

==========
Add support for faster, more limited accessibility modes.

See bug for context. This patch begins the implementation of
more limited accessibility modes by only fetching a subset of
accessibility attributes from Blink unless more are explicitly
needed.

Only Windows is affected initially. By default now when
accessibility support is enabled on Windows, only a few
key accessibility attributes are fetched, like role, state, name,
value, and location. As individual APIs are accessed that go
beyond these, Chrome's accessibility mode is automatically
changed to start fetching more.

Screen readers are unaffected by this change. I manually
tested JAWS and NVDA.

Password managers and single-sign-on software should
continue to work fine, but users should experience a
performance boost.

BUG=672205

Committed: https://crrev.com/368ea13e43a18dec62a7f72610398a2f9757b1c9
Cr-Commit-Position: refs/heads/master@{#439750}
==========

[commit-bot: I haz the power, 2016-12-20 08:29:08]

Patchset 10 (id:??) landed as
https://crrev.com/368ea13e43a18dec62a7f72610398a2f9757b1c9
Cr-Commit-Position: refs/heads/master@{#439750}