Add sparse accessibility attribute interface to Blink

Add sparse accessibility attribute interface to Blink

It's inefficient to have separate interfaces in blink::WebAXObject to
query for each possible attribute, especially when those attributes are
rare and obscure ARIA attributes. Calling getAttribute many times in a row
is wasteful since many Elements have a lot of attributes we don't care
about, and many ARIA attributes are quite rare.

This change introduces a new WebAXObject interface for returning sparse
attributes. Call it once and pass a callback function. The implementation
iterates over all of the attributes of the element and calls the callback
with each accessibility attribute found.

This change re-implements the following 3 ARIA attributes as sparse
attributes; they're covered by existing tests:
  aria-activedescendant
  aria-controls
  aria-flowto

BUG=675994, 644766
Review-Url: https://codereview.chromium.org/2589273002
Cr-Commit-Position: refs/heads/master@{#445527}
Committed: https://chromium.googlesource.com/chromium/src/+/22632c56f2fe702261795bb3cd3f76a717addf4a

[dmazzoni, 2016-12-20 17:30:38]

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

[dmazzoni, 2016-12-20 17:30:59]

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

[commit-bot: I haz the power, 2016-12-20 17:31:09]

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

[dmazzoni, 2016-12-20 18:24:20]

Description was changed from

==========
Implement 8 ARIA attributes using new Blink sparse attribute interface

It's inefficient to have separate interfaces in blink::WebAXObject to
query for each possible attribute, especially when those attributes are
rare and obscure ARIA attributes. Calling getAttribute many times in a row
is wasteful since many Elements have a lot of attributes we don't care
about, and many ARIA attributes are quite rare.

This change introduces a new WebAXObject interface for returning sparse
attributes. Call it once and pass a callback function. The implementation
iterates over all of the attributes of the element and calls the callback
with each accessibility attribute found.

This change re-implements the following 3 ARIA attributes as sparse
attributes; they're covered by existing tests:
  aria-activedescendant
  aria-controls
  aria-flowto

This change also implements 5 new ARIA 1.1 attributes in Blink, and the
change to fully support and test them in Chrome will follow:
  aria-details
  aria-errormessage
  aria-keyshortcuts
  aria-modal
  aria-roledescription

BUG=675994,644766
==========

to

==========
Implement 8 ARIA attributes using new Blink sparse attribute interface

It's inefficient to have separate interfaces in blink::WebAXObject to
query for each possible attribute, especially when those attributes are
rare and obscure ARIA attributes. Calling getAttribute many times in a row
is wasteful since many Elements have a lot of attributes we don't care
about, and many ARIA attributes are quite rare.

This change introduces a new WebAXObject interface for returning sparse
attributes. Call it once and pass a callback function. The implementation
iterates over all of the attributes of the element and calls the callback
with each accessibility attribute found.

This change re-implements the following 3 ARIA attributes as sparse
attributes; they're covered by existing tests:
  aria-activedescendant
  aria-controls
  aria-flowto

This change also implements 5 new ARIA 1.1 attributes in Blink, and the
change to fully support and test them in Chrome will follow in
https://codereview.chromium.org/2587343004/:
  aria-details
  aria-errormessage
  aria-keyshortcuts
  aria-modal
  aria-roledescription

BUG=675994,644766
==========

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

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[nektarios, 2016-12-22 18:33:26]

components/test_runner/web_ax_object_proxy.cc
 
+#include "base/containers/hash_tables.h"

I don’t think we are using any hash tables in this patch. Only std::map?


+class SparseAttributeMap : public blink::WebAXSparseAttributeMap {

Are we allowed to inherit from Blink classes? What coding style should we
follow, Blink’s or Chromium’s?


+  std::map<blink::WebAXBoolAttribute, bool> bool_attributes;
+  std::map<blink::WebAXStringAttribute,
+           blink::WebString> string_attributes;
+  std::map<blink::WebAXObjectAttribute,
+           blink::WebAXObject> object_attributes;
+  std::map<blink::WebAXObjectVectorAttribute,
+           blink::WebVector<blink::WebAXObject>> object_vector_attributes;

Is it time we switched to using std::unordered_map?
It’s faster than a map but it doesn’t sort its elements, so you can’t use
something like std::upper_bound.
I don’t think that attributes need to be sorted. Is there any Chromium guidance
on this?


 v8::Local<v8::Object> WebAXObjectProxy::AriaControlsElementAtIndex(
-                                                                unsigned index)
+    unsigned index)

Should this be “unsigned int”? I don’t know if Blink’s style has now been
changed after it has been incorporated into Chromium.


+  SparseAttributeMap attribute_map;
+  accessibility_object_.getSparseAXAttributes(attribute_map);
+  blink::WebVector<blink::WebAXObject> elements =
+      attribute_map.object_vector_attributes[
+          blink::WebAXObjectVectorAttribute::AriaControls];
   size_t elementCount = elements.size();

WebVector is like a heap object isn’t it? “=“ will not copy the whole vector?
Also, there are some variables that use the Blink style and some that use the
Chromium style?
Same below.



content/renderer/accessibility/blink_ax_tree_source.cc
 
+void AddIntListAttributeFromWebObjects(ui::AXIntListAttribute attr,
…
+  if (ids.size() > 0)

Use ids.empty()

Could |AddIntListAttributeFromWebObjects| be a member function on
|SparseAttributeMap|?
It takes |AXContentNode| as an argument and the class already has the node as a
field.


+
+  void addBoolAttribute(blink::WebAXBoolAttribute attribute, bool value)
+      override {
+    switch (attribute) {
+      case blink::WebAXBoolAttribute::AriaModal:
+        // TODO

There are many TODOs. I think it’s a good idea to add the ARIA 1.1 bug number in
all of them.


WebKit/Source/core/html/HTMLAttributeNames.in
+ aria-keyshortcuts

I thought it was aria-keyboardshortcuts. I guess the standard changed again?



third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp

+enum AXAttributeType {
+  AXNoneAttributeType,
+  AXStringAttributeType,
+  AXBoolAttributeType,
+  AXObjectAttributeType,
+  AXObjectVectorAttributeType,
+};

Don’t we usually put the enum name at the beginning?
E.g. , AXAttributeTypeObjectVector vs. AXObjectVectorAttributeType.



+struct AXAttributeInfo {
+  AXAttributeInfo() : type(AXNoneAttributeType) {}
+
+  bool operator!() const { return type == AXNoneAttributeType; }
+
+  AXAttributeType type;

Could we use a single base class for the different class enums and get rid of
the union?

+void MapAXAttribute(AXAttributeMap& map,
+                    QualifiedName domAttr,
+                    AXBoolAttribute axAttr) {

In the place of the  4 separate functions, could we use a single function
instead?
Enum classes can have a base class. So, could we create a base class for all
types of attributes and have the 4 more specific enums derive from it?

Also, could we construct the map via an initializer list instead of by calling
add... funcions repeatedly.

[nektarios, 2016-12-22 18:33:47]

components/test_runner/web_ax_object_proxy.cc
 
+#include "base/containers/hash_tables.h"

I don’t think we are using any hash tables in this patch. Only std::map?


+class SparseAttributeMap : public blink::WebAXSparseAttributeMap {

Are we allowed to inherit from Blink classes? What coding style should we
follow, Blink’s or Chromium’s?


+  std::map<blink::WebAXBoolAttribute, bool> bool_attributes;
+  std::map<blink::WebAXStringAttribute,
+           blink::WebString> string_attributes;
+  std::map<blink::WebAXObjectAttribute,
+           blink::WebAXObject> object_attributes;
+  std::map<blink::WebAXObjectVectorAttribute,
+           blink::WebVector<blink::WebAXObject>> object_vector_attributes;

Is it time we switched to using std::unordered_map?
It’s faster than a map but it doesn’t sort its elements, so you can’t use
something like std::upper_bound.
I don’t think that attributes need to be sorted. Is there any Chromium guidance
on this?


 v8::Local<v8::Object> WebAXObjectProxy::AriaControlsElementAtIndex(
-                                                                unsigned index)
+    unsigned index)

Should this be “unsigned int”? I don’t know if Blink’s style has now been
changed after it has been incorporated into Chromium.


+  SparseAttributeMap attribute_map;
+  accessibility_object_.getSparseAXAttributes(attribute_map);
+  blink::WebVector<blink::WebAXObject> elements =
+      attribute_map.object_vector_attributes[
+          blink::WebAXObjectVectorAttribute::AriaControls];
   size_t elementCount = elements.size();

WebVector is like a heap object isn’t it? “=“ will not copy the whole vector?
Also, there are some variables that use the Blink style and some that use the
Chromium style?
Same below.



content/renderer/accessibility/blink_ax_tree_source.cc
 
+void AddIntListAttributeFromWebObjects(ui::AXIntListAttribute attr,
…
+  if (ids.size() > 0)

Use ids.empty()

Could |AddIntListAttributeFromWebObjects| be a member function on
|SparseAttributeMap|?
It takes |AXContentNode| as an argument and the class already has the node as a
field.


+
+  void addBoolAttribute(blink::WebAXBoolAttribute attribute, bool value)
+      override {
+    switch (attribute) {
+      case blink::WebAXBoolAttribute::AriaModal:
+        // TODO

There are many TODOs. I think it’s a good idea to add the ARIA 1.1 bug number in
all of them.


WebKit/Source/core/html/HTMLAttributeNames.in
+ aria-keyshortcuts

I thought it was aria-keyboardshortcuts. I guess the standard changed again?



third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp

+enum AXAttributeType {
+  AXNoneAttributeType,
+  AXStringAttributeType,
+  AXBoolAttributeType,
+  AXObjectAttributeType,
+  AXObjectVectorAttributeType,
+};

Don’t we usually put the enum name at the beginning?
E.g. , AXAttributeTypeObjectVector vs. AXObjectVectorAttributeType.



+struct AXAttributeInfo {
+  AXAttributeInfo() : type(AXNoneAttributeType) {}
+
+  bool operator!() const { return type == AXNoneAttributeType; }
+
+  AXAttributeType type;

Could we use a single base class for the different class enums and get rid of
the union?

+void MapAXAttribute(AXAttributeMap& map,
+                    QualifiedName domAttr,
+                    AXBoolAttribute axAttr) {

In the place of the  4 separate functions, could we use a single function
instead?
Enum classes can have a base class. So, could we create a base class for all
types of attributes and have the 4 more specific enums derive from it?

Also, could we construct the map via an initializer list instead of by calling
add... funcions repeatedly.

[nektarios, 2016-12-22 18:33:47]

components/test_runner/web_ax_object_proxy.cc
 
+#include "base/containers/hash_tables.h"

I don’t think we are using any hash tables in this patch. Only std::map?


+class SparseAttributeMap : public blink::WebAXSparseAttributeMap {

Are we allowed to inherit from Blink classes? What coding style should we
follow, Blink’s or Chromium’s?


+  std::map<blink::WebAXBoolAttribute, bool> bool_attributes;
+  std::map<blink::WebAXStringAttribute,
+           blink::WebString> string_attributes;
+  std::map<blink::WebAXObjectAttribute,
+           blink::WebAXObject> object_attributes;
+  std::map<blink::WebAXObjectVectorAttribute,
+           blink::WebVector<blink::WebAXObject>> object_vector_attributes;

Is it time we switched to using std::unordered_map?
It’s faster than a map but it doesn’t sort its elements, so you can’t use
something like std::upper_bound.
I don’t think that attributes need to be sorted. Is there any Chromium guidance
on this?


 v8::Local<v8::Object> WebAXObjectProxy::AriaControlsElementAtIndex(
-                                                                unsigned index)
+    unsigned index)

Should this be “unsigned int”? I don’t know if Blink’s style has now been
changed after it has been incorporated into Chromium.


+  SparseAttributeMap attribute_map;
+  accessibility_object_.getSparseAXAttributes(attribute_map);
+  blink::WebVector<blink::WebAXObject> elements =
+      attribute_map.object_vector_attributes[
+          blink::WebAXObjectVectorAttribute::AriaControls];
   size_t elementCount = elements.size();

WebVector is like a heap object isn’t it? “=“ will not copy the whole vector?
Also, there are some variables that use the Blink style and some that use the
Chromium style?
Same below.



content/renderer/accessibility/blink_ax_tree_source.cc
 
+void AddIntListAttributeFromWebObjects(ui::AXIntListAttribute attr,
…
+  if (ids.size() > 0)

Use ids.empty()

Could |AddIntListAttributeFromWebObjects| be a member function on
|SparseAttributeMap|?
It takes |AXContentNode| as an argument and the class already has the node as a
field.


+
+  void addBoolAttribute(blink::WebAXBoolAttribute attribute, bool value)
+      override {
+    switch (attribute) {
+      case blink::WebAXBoolAttribute::AriaModal:
+        // TODO

There are many TODOs. I think it’s a good idea to add the ARIA 1.1 bug number in
all of them.


WebKit/Source/core/html/HTMLAttributeNames.in
+ aria-keyshortcuts

I thought it was aria-keyboardshortcuts. I guess the standard changed again?



third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp

+enum AXAttributeType {
+  AXNoneAttributeType,
+  AXStringAttributeType,
+  AXBoolAttributeType,
+  AXObjectAttributeType,
+  AXObjectVectorAttributeType,
+};

Don’t we usually put the enum name at the beginning?
E.g. , AXAttributeTypeObjectVector vs. AXObjectVectorAttributeType.



+struct AXAttributeInfo {
+  AXAttributeInfo() : type(AXNoneAttributeType) {}
+
+  bool operator!() const { return type == AXNoneAttributeType; }
+
+  AXAttributeType type;

Could we use a single base class for the different class enums and get rid of
the union?

+void MapAXAttribute(AXAttributeMap& map,
+                    QualifiedName domAttr,
+                    AXBoolAttribute axAttr) {

In the place of the  4 separate functions, could we use a single function
instead?
Enum classes can have a base class. So, could we create a base class for all
types of attributes and have the 4 more specific enums derive from it?

Also, could we construct the map via an initializer list instead of by calling
add... funcions repeatedly.

[dmazzoni, 2016-12-28 23:51:32]

On 2016/12/22 18:33:26, nektarios wrote:
> +#include "base/containers/hash_tables.h"
> 
> I don’t think we are using any hash tables in this patch. Only std::map?

You're right, done.

> 
> +class SparseAttributeMap : public blink::WebAXSparseAttributeMap {
> 
> Are we allowed to inherit from Blink classes? What coding style should we
> follow, Blink’s or Chromium’s?

Yes, it's really common to inherit from Blink classes in Chromium code.
We use the Chromium style.


> +  std::map<blink::WebAXBoolAttribute, bool> bool_attributes;
> +  std::map<blink::WebAXStringAttribute,
> +           blink::WebString> string_attributes;
> +  std::map<blink::WebAXObjectAttribute,
> +           blink::WebAXObject> object_attributes;
> +  std::map<blink::WebAXObjectVectorAttribute,
> +           blink::WebVector<blink::WebAXObject>> object_vector_attributes;
> 
> Is it time we switched to using std::unordered_map?
> It’s faster than a map but it doesn’t sort its elements, so you can’t use
> something like std::upper_bound.
> I don’t think that attributes need to be sorted. Is there any Chromium
guidance
> on this?

Keep in mind this is just test code. Efficiency doesn't matter at all, I just
want something that's as simple and readable as possible.

>  v8::Local<v8::Object> WebAXObjectProxy::AriaControlsElementAtIndex(
> -                                                                unsigned
index)
> +    unsigned index)
> 
> Should this be “unsigned int”? I don’t know if Blink’s style has now been
> changed after it has been incorporated into Chromium.

Looking through third_party/WebKit/public/web I see hundreds of instances
of "unsigned" as a return value and no "unsigned int". I think we should
match the type here, just like we use LONG when storing the return value
from a win32 API.

> +  SparseAttributeMap attribute_map;
> +  accessibility_object_.getSparseAXAttributes(attribute_map);
> +  blink::WebVector<blink::WebAXObject> elements =
> +      attribute_map.object_vector_attributes[
> +          blink::WebAXObjectVectorAttribute::AriaControls];
>    size_t elementCount = elements.size();
> 
> WebVector is like a heap object isn’t it? “=“ will not copy the whole vector?

Yes, but this is test code. I really don't care about efficiency here.
Look at blink_ax_tree_source for the real implementation, that one is
designed to be efficient. This one is just glue code for tests.

> Also, there are some variables that use the Blink style and some that use the
> Chromium style?
> Same below.

I don't see any that use the wrong style, can you be more specific?

> content/renderer/accessibility/blink_ax_tree_source.cc
>  
> +void AddIntListAttributeFromWebObjects(ui::AXIntListAttribute attr,
> …
> +  if (ids.size() > 0)
> 
> Use ids.empty()

Done

> Could |AddIntListAttributeFromWebObjects| be a member function on
> |SparseAttributeMap|?
> It takes |AXContentNode| as an argument and the class already has the node as
a
> field.

Yes, but we're also calling the same helper from outside of SparseAttributeMap.
It existed before this change, I just moved it up in the file.

> 
> 
> +
> +  void addBoolAttribute(blink::WebAXBoolAttribute attribute, bool value)
> +      override {
> +    switch (attribute) {
> +      case blink::WebAXBoolAttribute::AriaModal:
> +        // TODO
> 
> There are many TODOs. I think it’s a good idea to add the ARIA 1.1 bug number
in
> all of them.

Done, but note that I already uploaded change 2587343004 which implements
them. I just split them up to make review easier.

> WebKit/Source/core/html/HTMLAttributeNames.in
> + aria-keyshortcuts
> 
> I thought it was aria-keyboardshortcuts. I guess the standard changed again?

This is from the latest editor's draft.

> third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
> 
> +enum AXAttributeType {
> +  AXNoneAttributeType,
> +  AXStringAttributeType,
> +  AXBoolAttributeType,
> +  AXObjectAttributeType,
> +  AXObjectVectorAttributeType,
> +};
> 
> Don’t we usually put the enum name at the beginning?
> E.g. , AXAttributeTypeObjectVector vs. AXObjectVectorAttributeType.

You're right, good idea. Done.

> +struct AXAttributeInfo {
> +  AXAttributeInfo() : type(AXNoneAttributeType) {}
> +
> +  bool operator!() const { return type == AXNoneAttributeType; }
> +
> +  AXAttributeType type;
> 
> Could we use a single base class for the different class enums and get rid of
> the union?

It's true that would be slightly more type-safe but at the expense of lots more
boilerplate code. Do you think it would really help much? This is a static class
only used within one source file, and this seems like a classic textbook case of
when to use a union.

Perhaps as an alternative we could create a map from QualifiedName to a
base::Closure (those are allowed in Blink now), and bind to a helper function
that calls the appropriate method on AXSparseAttributeMap.

Do you think that would be better?

> +void MapAXAttribute(AXAttributeMap& map,
> +                    QualifiedName domAttr,
> +                    AXBoolAttribute axAttr) {
> 
> In the place of the  4 separate functions, could we use a single function
> instead?
> Enum classes can have a base class. So, could we create a base class for all
> types of attributes and have the 4 more specific enums derive from it?

Same question as above, I think - basically avoiding the union.

I don't really like the idea of having 5 classes when the whole point is
just to map an attribute to a single function call. How about a map
from attribute to a function call and no classes?

[dmazzoni, 2016-12-29 06:42:19]

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

[commit-bot: I haz the power, 2016-12-29 06:42:28]

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

[dmazzoni, 2016-12-29 06:43:15]

OK, take a look - I'm not sure if this is exactly what you had in
mind, but it turns out we can't use base::Callback inside blink/core
even though it's used in some parts of blink, so instead I went with
a base class and four subclasses, similar to your suggestion.

[commit-bot: I haz the power, 2016-12-29 06:43:59]

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

[commit-bot: I haz the power, 2016-12-29 06:44:00]

Dry run: Try jobs failed on following builders:
  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-29 07:18:30]

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

[commit-bot: I haz the power, 2016-12-29 07:18:36]

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-29 08:15:48]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dcheng, 2016-12-29 09:11:12]

On 2016/12/29 06:43:15, dmazzoni wrote:
> OK, take a look - I'm not sure if this is exactly what you had in
> mind, but it turns out we can't use base::Callback inside blink/core
> even though it's used in some parts of blink, so instead I went with
> a base class and four subclasses, similar to your suggestion.
Drive-by:
For callbacks in Blink, you should use WTF::bind and friends from
WTF/Functional.h.

[dmazzoni, 2016-12-29 16:07:27]

On 2016/12/29 09:11:12, dcheng wrote:
> On 2016/12/29 06:43:15, dmazzoni wrote:
> > OK, take a look - I'm not sure if this is exactly what you had in
> > mind, but it turns out we can't use base::Callback inside blink/core
> > even though it's used in some parts of blink, so instead I went with
> > a base class and four subclasses, similar to your suggestion.
> Drive-by:
> For callbacks in Blink, you should use WTF::bind and friends from
> WTF/Functional.h.

I was trying to make a HashMap from QualifiedName to WTF::Function,
but it seemed like a poor fit, since WTF::bind returns a unique_ptr, but
WTF::HashMap seems to work well with types that are copyable and
have a default value.

It looks like there are several other examples in Blink of a HashMap
from QualifiedName to a function pointer or custom functor, so I
basically went with a custom functor class.

[dmazzoni, 2016-12-29 16:15:35]

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

[commit-bot: I haz the power, 2016-12-29 16:15:44]

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-29 17:58:10]

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

[commit-bot: I haz the power, 2016-12-29 17:58:11]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-01-09 22:22:33]

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

[dmazzoni, 2017-01-09 22:22:34]

+aboxhall since Nektarios is still on vacation

[aboxhall, 2017-01-10 05:59:59]

It took me a really long time to get my head around what was going on here -
there seem to be an awful lot of classes and subclasses and adapters and
helpers.

The core ideas seem to be:

- AXObjects can write rarely used attributes to a data object which implements a
particular interface (AXSparseAttributeMap) - and this is plumbed through to the
Web API which involves converting everything to Web API data types.

- This is implemented internally via a static local map
(AXSparseAttributeHelperMap) of attribute names to functors
(AXSparseAttributeHelper) which call the appropriate method on the object iff
that attribute should be set, which is iterated over to set all rarely used
attributes on the data object.

The similarity of naming between AXSparseAttributeMap and
AXSparseAttributeHelperMap is especially confusing, given they do completely
different things.

Could we leave the word "Map" off the name of the data object interface? Then
we'd end up with something like

AXObject::AXSparseAttributes
inherited by
- InspectorAccessibilityAgent::SparseAttributesAXPropertyAdapter (or, more
simply, AXPropertyAdapter?)
- WebAXObject::SparseAttributesAdapter (or
WebAXObject::WebAXSparseAttributesAdapter - longer but more consistent
foo-adapter where foo is the thing being adapted)

WebAXObject::WebAXSparseAttributes
inherited by
- ::SparseAttributes (or perhaps AXContentNodeDataAdapter to use the
"foo-adapter" scheme)

Also, perhaps "AddFooHelper" could be something like "FooSetter" ->
SparseAttributeSetter
inherited by
- BoolAttributeSetter
- StringAttributeSetter
- ObjectAttributeSetter
- ObjectVectorAttributeSetter


What do you think?

https://codereview.chromium.org/2589273002/diff/60001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/2589273002/diff/60001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:68: class
SparseAttributeMap : public blink::WebAXSparseAttributeMap {
The name "SparseAttributeMap" is confusing me a bit, since none of these (other
than the test implementation) seem to be actual data stores but instead
selectively copy data through to some other place. In particular, they are write
only, where "Map" implies that you might be able to get the data out again.

https://codereview.chromium.org/2589273002/diff/60001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:109: // TODO
Any chance we could add these values in the follow-up change instead? There's so
much going on in this change already.

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:874:
axSparseAttributeHelperMap.set(
Perhaps pull this block out into a helper function? It dominates this method
even though it should only run once.

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebAXObject.cpp:65: class
SparseAttributeMapAdapter : public AXSparseAttributeMap {
WebAX[...]Wrapper/Adapter?

[dmazzoni, 2017-01-12 03:30:35]

Description was changed from

==========
Implement 8 ARIA attributes using new Blink sparse attribute interface

It's inefficient to have separate interfaces in blink::WebAXObject to
query for each possible attribute, especially when those attributes are
rare and obscure ARIA attributes. Calling getAttribute many times in a row
is wasteful since many Elements have a lot of attributes we don't care
about, and many ARIA attributes are quite rare.

This change introduces a new WebAXObject interface for returning sparse
attributes. Call it once and pass a callback function. The implementation
iterates over all of the attributes of the element and calls the callback
with each accessibility attribute found.

This change re-implements the following 3 ARIA attributes as sparse
attributes; they're covered by existing tests:
  aria-activedescendant
  aria-controls
  aria-flowto

This change also implements 5 new ARIA 1.1 attributes in Blink, and the
change to fully support and test them in Chrome will follow in
https://codereview.chromium.org/2587343004/:
  aria-details
  aria-errormessage
  aria-keyshortcuts
  aria-modal
  aria-roledescription

BUG=675994,644766
==========

to

==========
Add sparse accessibility attribute interface to Blink

It's inefficient to have separate interfaces in blink::WebAXObject to
query for each possible attribute, especially when those attributes are
rare and obscure ARIA attributes. Calling getAttribute many times in a row
is wasteful since many Elements have a lot of attributes we don't care
about, and many ARIA attributes are quite rare.

This change introduces a new WebAXObject interface for returning sparse
attributes. Call it once and pass a callback function. The implementation
iterates over all of the attributes of the element and calls the callback
with each accessibility attribute found.

This change re-implements the following 3 ARIA attributes as sparse
attributes; they're covered by existing tests:
  aria-activedescendant
  aria-controls
  aria-flowto

BUG=675994,644766
==========

[dmazzoni, 2017-01-12 03:36:24]

I renamed the classes and pulled the 5 new ARIA attributes
out of this change, so this one is just a refactoring change.
Ready for another look.

https://codereview.chromium.org/2589273002/diff/60001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/2589273002/diff/60001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:68: class
SparseAttributeMap : public blink::WebAXSparseAttributeMap {
On 2017/01/10 05:59:59, aboxhall wrote:
> The name "SparseAttributeMap" is confusing me a bit, since none of these
(other
> than the test implementation) seem to be actual data stores but instead
> selectively copy data through to some other place. In particular, they are
write
> only, where "Map" implies that you might be able to get the data out again.

Done.

https://codereview.chromium.org/2589273002/diff/60001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:109: // TODO
On 2017/01/10 05:59:59, aboxhall wrote:
> Any chance we could add these values in the follow-up change instead? There's
so
> much going on in this change already.

Sure. I'll pull the new attributes into a middle change in-between this one and
the
one that was going to follow.

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:874:
axSparseAttributeHelperMap.set(
On 2017/01/10 05:59:59, aboxhall wrote:
> Perhaps pull this block out into a helper function? It dominates this method
> even though it should only run once.

Done.

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebAXObject.cpp:65: class
SparseAttributeMapAdapter : public AXSparseAttributeMap {
This is now SparseAttributeClientAdapter

I'm not sure putting the WebAX prefix makes sense since it's just a helper class

[dmazzoni, 2017-01-12 03:38:44]

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

[commit-bot: I haz the power, 2017-01-12 03:38:57]

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-01-12 03:47:34]

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

[commit-bot: I haz the power, 2017-01-12 03:47:36]

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

[aboxhall, 2017-01-12 04:26:52]

lgtm but please consider renaming the two "adapter" classes.

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/2589273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebAXObject.cpp:65: class
SparseAttributeMapAdapter : public AXSparseAttributeMap {
On 2017/01/12 03:36:24, dmazzoni wrote:
> This is now SparseAttributeClientAdapter
> 
> I'm not sure putting the WebAX prefix makes sense since it's just a helper
class

I meant because it's adapting WebAXSparseAttributeClient (and similarly with
AXContentNodeData in BlinkAXTreeSource -> could be AXContentNodeDataAdapter
instead of SparseAttributeAdapter; also why is it a SparseAttributeAdapter there
but a SparseAttributeClientAdapter here?). I think the thing it's wrapping is
more interesting than the interface it implements given you can tell the latter
from context.

[dmazzoni, 2017-01-12 07:08:51]

On 2017/01/12 04:26:52, aboxhall wrote:
> I meant because it's adapting WebAXSparseAttributeClient (and similarly with
> AXContentNodeData in BlinkAXTreeSource -> could be AXContentNodeDataAdapter
> instead of SparseAttributeAdapter; also why is it a SparseAttributeAdapter
there
> but a SparseAttributeClientAdapter here?). I think the thing it's wrapping is
> more interesting than the interface it implements given you can tell the
latter
> from context.

Got it, will do. Thanks!

[dmazzoni, 2017-01-12 07:08:51]

On 2017/01/12 04:26:52, aboxhall wrote:
> I meant because it's adapting WebAXSparseAttributeClient (and similarly with
> AXContentNodeData in BlinkAXTreeSource -> could be AXContentNodeDataAdapter
> instead of SparseAttributeAdapter; also why is it a SparseAttributeAdapter
there
> but a SparseAttributeClientAdapter here?). I think the thing it's wrapping is
> more interesting than the interface it implements given you can tell the
latter
> from context.

Got it, will do. Thanks!

[dmazzoni, 2017-01-12 07:09:16]

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

[dmazzoni, 2017-01-12 07:09:16]

+dglazkov for owners review

[dmazzoni, 2017-01-13 20:24:24]

Renaming done.

[dmazzoni, 2017-01-13 20:24:29]

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

[commit-bot: I haz the power, 2017-01-13 20:25:15]

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-01-13 23:28:38]

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

[commit-bot: I haz the power, 2017-01-13 23:28:39]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-01-18 16:23:43]

dmazzoni@chromium.org changed reviewers:
+ dcheng@chromium.org, mkwst@chromium.org

[dmazzoni, 2017-01-18 16:23:44]

Ping for OWNERS review of WebKit/public
and adding mkwst and dcheng...

[nektarios, 2017-01-18 18:45:19]

Sorry, I have some comments too.
Over all I liked the idea of using maps with functions that should be executed
for each type of attribute.


components/test_runner/web_ax_object_proxy.cc
b/components/test_runner/web_ax_object_proxy.cc

+  std::map<blink::WebAXBoolAttribute, bool> bool_attributes;
+  std::map<blink::WebAXStringAttribute, blink::WebString> string_attributes;
+  std::map<blink::WebAXObjectAttribute, blink::WebAXObject> object_attributes;

Shouldn't these variables be private and accessed via accessors?


+  blink::WebVector<blink::WebAXObject> elements =
+      attribute_adapter.object_vector_attributes
+          [blink::WebAXObjectVectorAttribute::AriaControls];
   
   Shouldn't the "[" be at the end of the second line?

   
content/renderer/accessibility/blink_ax_tree_source.cc

+class AXContentNodeDataSparseAttributeAdapter
+    : public blink::WebAXSparseAttributeClient {
+ public:
+  AXContentNodeDataSparseAttributeAdapter(AXContentNodeData* dst) : dst_(dst)
{}

I prefer to pass in a reference to AXContentNodeData instead of a pointer.
Safeguards against creating an adapter with a nullptr node.


third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp

+class SparseAttributeSetter {
+ public:
+  virtual void Run(const AXObject*,
+                   AXSparseAttributeClient,

Shouldn't "Run" be with a small "r"?
Isn't it better if AXObject is a reference?
Shouldn't AXSparseAttributeClient be a reference?
How come "Run" is a public method in the base class, but is private in sub
classes?


+static AXSparseAttributeSetterMap& GetSparseAttributeSetterMap() {

Shouldn't the function start with a small letter?


+typedef HashMap<QualifiedName, SparseAttributeSetter*>
+    AXSparseAttributeSetterMap;

Why is the attribute setter a pointer? Isn't the hash map cabable of storing
things by value?


+static AXSparseAttributeSetterMap& GetSparseAttributeSetterMap() {

I believe that "copy elision" would take care of not copying the map when
returned, so the reference might be unnecessary here?


+        aria_activedescendantAttr,
+        new ObjectAttributeSetter(AXObjectAttribute::AriaActiveDescendant));

Who is responsible for cleaning up the setter objects that have been created and
placed in the map?


third_party/WebKit/Source/modules/accessibility/AXObject.h

+  virtual void addStringAttribute(AXStringAttribute, const String) = 0;
Shouldn't String be a reference?

+  virtual void addObjectAttribute(AXObjectAttribute, AXObject*) = 0;
Shouldn't object be a reference?

[nektarios, 2017-01-18 20:38:33]

third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp

+class SparseAttributeAXPropertyAdapter
+    : public GarbageCollected<SparseAttributeAXPropertyAdapter>,

AXObject needs finalization. I think you should inherit from
GarbageCollectedFinalized<T>.


 
+  Member<SparseAttributeAXPropertyAdapter> adapter =
+      new SparseAttributeAXPropertyAdapter(axObject, *(properties.get()));
+  axObject.getSparseAXAttributes(*adapter.get());

"adapter" is a stack object, isn't it? No "member<T>" necessary.

[haraken, 2017-01-18 23:39:59]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-01-18 23:40:00]

On 2017/01/18 20:38:33, nektarios wrote:
>
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
> 
> +class SparseAttributeAXPropertyAdapter
> +    : public GarbageCollected<SparseAttributeAXPropertyAdapter>,
> 
> AXObject needs finalization. I think you should inherit from
> GarbageCollectedFinalized<T>.

Member<AXObject> doesn't need finalization. So GarbageCollected will be fine.

> 
> 
>  
> +  Member<SparseAttributeAXPropertyAdapter> adapter =
> +      new SparseAttributeAXPropertyAdapter(axObject, *(properties.get()));
> +  axObject.getSparseAXAttributes(*adapter.get());
> 
> "adapter" is a stack object, isn't it? No "member<T>" necessary.

Right. Just use SparseAttributeAXPropertyAdapter*.

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:74: class
SparseAttributeSetter {

Add USING_FAST_MALLOC().

[Mike West, 2017-01-19 07:32:56]

WebKit/public LGTM.

[dcheng, 2017-01-19 23:02:45]

LGTM with some minor nits

https://codereview.chromium.org/2589273002/diff/100001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/2589273002/diff/100001/content/renderer/acces...
content/renderer/accessibility/blink_ax_tree_source.cc:59:
WebVector<WebAXObject> objects,
Nit: pass by const ref

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:175:
AXSparseAttributeSetterMap;
Nit: prefer using AXSpareAttributeSetterMap = HashMap<...> in new code.

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:574:
new SparseAttributeAXPropertyAdapter(axObject, *(properties.get()));
Nit: just write *properties

[dmazzoni, 2017-01-21 00:16:24]

On 2017/01/18 18:45:19, nektarios wrote:
> components/test_runner/web_ax_object_proxy.cc
> 
> +  std::map<blink::WebAXBoolAttribute, bool> bool_attributes;
> +  std::map<blink::WebAXStringAttribute, blink::WebString> string_attributes;
> +  std::map<blink::WebAXObjectAttribute, blink::WebAXObject>
object_attributes;
> 
> Shouldn't these variables be private and accessed via accessors?

This is a helper class for test code. It's not even declared in a header file
so there's no worry about code being dependent on its interface. Don't you
think it's reasonable to keep it as concise as possible like this?

> +  blink::WebVector<blink::WebAXObject> elements =
> +      attribute_adapter.object_vector_attributes
> +          [blink::WebAXObjectVectorAttribute::AriaControls];
>    
>    Shouldn't the "[" be at the end of the second line?

Seems weird to me too, but this was formatted by "git cl format".
I just tried to move the bracket and ran git cl format again, and it
moved it right back.

> content/renderer/accessibility/blink_ax_tree_source.cc
> 
> +class AXContentNodeDataSparseAttributeAdapter
> +    : public blink::WebAXSparseAttributeClient {
> + public:
> +  AXContentNodeDataSparseAttributeAdapter(AXContentNodeData* dst) : dst_(dst)
> {}
> 
> I prefer to pass in a reference to AXContentNodeData instead of a pointer.
> Safeguards against creating an adapter with a nullptr node.

Google's C++ style guide says that "All parameters passed by reference must be
labeled const" and specifically pointer arguments are modifiable and reference
arguments are not. Note that Blink doesn't have this same guidance. In this
case since the class is modifying AXContentNodeData I think a pointer is
appropriate.

I added a DCHECK to make it clear that it shouldn't be null.

> 
> third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
> 
> +class SparseAttributeSetter {
> + public:
> +  virtual void Run(const AXObject*,
> +                   AXSparseAttributeClient,
> 
> Shouldn't "Run" be with a small "r"?

Yes, done.

> Isn't it better if AXObject is a reference?

Sure, done.

> Shouldn't AXSparseAttributeClient be a reference?

It already is.

> How come "Run" is a public method in the base class, but is private in sub
> classes?

It's basically indicating that there's no good reason you'd want to call this
directly
on an instance of the subclass, you're supposed to call it on the base class and
let the subclass implement it.

> +static AXSparseAttributeSetterMap& GetSparseAttributeSetterMap() {
> 
> Shouldn't the function start with a small letter?

Yes, done.

> +typedef HashMap<QualifiedName, SparseAttributeSetter*>
> +    AXSparseAttributeSetterMap;
> 
> Why is the attribute setter a pointer? Isn't the hash map cabable of storing
> things by value?

Blink's HashMap doesn't seem to support that. The first error I hit is that
SparseAttributeSetter is an abstract class. It seems to require that the value
type have a default constructor so that it can implement get() and have it
return a value even if the key isn't in the map.

> +static AXSparseAttributeSetterMap& GetSparseAttributeSetterMap() {
> 
> I believe that "copy elision" would take care of not copying the map when
> returned, so the reference might be unnecessary here?

I think you're right, but it doesn't hurt, right?

> +        aria_activedescendantAttr,
> +        new ObjectAttributeSetter(AXObjectAttribute::AriaActiveDescendant));
> 
> Who is responsible for cleaning up the setter objects that have been created
and
> placed in the map?

Nobody, they're designed to leak on purpose.

> third_party/WebKit/Source/modules/accessibility/AXObject.h
> 
> +  virtual void addStringAttribute(AXStringAttribute, const String) = 0;
> Shouldn't String be a reference?

Done.

> +  virtual void addObjectAttribute(AXObjectAttribute, AXObject*) = 0;
> Shouldn't object be a reference?

Done

On 2017/01/18 20:38:33, nektarios wrote:
>
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
> 
> +class SparseAttributeAXPropertyAdapter
> +    : public GarbageCollected<SparseAttributeAXPropertyAdapter>,
> 
> AXObject needs finalization. I think you should inherit from
> GarbageCollectedFinalized<T>.

Not needed.

> +  Member<SparseAttributeAXPropertyAdapter> adapter =
> +      new SparseAttributeAXPropertyAdapter(axObject, *(properties.get()));
> +  axObject.getSparseAXAttributes(*adapter.get());
> 
> "adapter" is a stack object, isn't it? No "member<T>" necessary.

Done.

[dmazzoni, 2017-01-21 00:16:36]

https://codereview.chromium.org/2589273002/diff/100001/content/renderer/acces...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/2589273002/diff/100001/content/renderer/acces...
content/renderer/accessibility/blink_ax_tree_source.cc:59:
WebVector<WebAXObject> objects,
On 2017/01/19 23:02:44, dcheng wrote:
> Nit: pass by const ref

Done.

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:74: class
SparseAttributeSetter {
On 2017/01/18 23:40:00, haraken wrote:
> 
> Add USING_FAST_MALLOC().

Done.

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:175:
AXSparseAttributeSetterMap;
On 2017/01/19 23:02:44, dcheng wrote:
> Nit: prefer using AXSpareAttributeSetterMap = HashMap<...> in new code.

Done.

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
(right):

https://codereview.chromium.org/2589273002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp:574:
new SparseAttributeAXPropertyAdapter(axObject, *(properties.get()));
On 2017/01/19 23:02:45, dcheng wrote:
> Nit: just write *properties

Done.

[dmazzoni, 2017-01-21 00:16:49]

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

[commit-bot: I haz the power, 2017-01-21 00:17:24]

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-01-21 03:57:06]

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

[commit-bot: I haz the power, 2017-01-21 03:57:07]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-01-23 20:43:42]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-01-23 20:43:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/2589273002/#ps120001
(title: "Address feedback")

[commit-bot: I haz the power, 2017-01-23 20:44:20]

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-01-23 22:57:45]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485204222353030,
"parent_rev": "b82ab87dd4f38c3c7ca5d6d2e8e3a96220772222", "commit_rev":
"22632c56f2fe702261795bb3cd3f76a717addf4a"}

[commit-bot: I haz the power, 2017-01-23 22:58:16]

Description was changed from

==========
Add sparse accessibility attribute interface to Blink

It's inefficient to have separate interfaces in blink::WebAXObject to
query for each possible attribute, especially when those attributes are
rare and obscure ARIA attributes. Calling getAttribute many times in a row
is wasteful since many Elements have a lot of attributes we don't care
about, and many ARIA attributes are quite rare.

This change introduces a new WebAXObject interface for returning sparse
attributes. Call it once and pass a callback function. The implementation
iterates over all of the attributes of the element and calls the callback
with each accessibility attribute found.

This change re-implements the following 3 ARIA attributes as sparse
attributes; they're covered by existing tests:
  aria-activedescendant
  aria-controls
  aria-flowto

BUG=675994,644766
==========

to

==========
Add sparse accessibility attribute interface to Blink

It's inefficient to have separate interfaces in blink::WebAXObject to
query for each possible attribute, especially when those attributes are
rare and obscure ARIA attributes. Calling getAttribute many times in a row
is wasteful since many Elements have a lot of attributes we don't care
about, and many ARIA attributes are quite rare.

This change introduces a new WebAXObject interface for returning sparse
attributes. Call it once and pass a callback function. The implementation
iterates over all of the attributes of the element and calls the callback
with each accessibility attribute found.

This change re-implements the following 3 ARIA attributes as sparse
attributes; they're covered by existing tests:
  aria-activedescendant
  aria-controls
  aria-flowto

BUG=675994,644766

Review-Url: https://codereview.chromium.org/2589273002
Cr-Commit-Position: refs/heads/master@{#445527}
Committed:
https://chromium.googlesource.com/chromium/src/+/22632c56f2fe702261795bb3cd3f...
==========

[commit-bot: I haz the power, 2017-01-23 22:58:18]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/22632c56f2fe702261795bb3cd3f...