|
|
Created:
4 years ago by dmazzoni Modified:
3 years, 11 months ago CC:
aboxhall, aboxhall+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, einbinder+watch-test-runner_chromium.org, haraken, jam, je_julie, jochen+watch_chromium.org, kinuko+watch, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd 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
Patch Set 1 #Patch Set 2 : Use classes instead of a union type #Patch Set 3 : Rebase #Patch Set 4 : Try to fix win component build compile #
Total comments: 9
Patch Set 5 : Rename as suggested, and remove new ARIA attributes from this change #Patch Set 6 : Renamed #
Total comments: 8
Patch Set 7 : Address feedback #Messages
Total messages: 65 (39 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + nektar@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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.
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.
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.
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?
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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.
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.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dmazzoni@chromium.org changed reviewers: + aboxhall@chromium.org
+aboxhall since Nektarios is still on vacation
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?
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 ==========
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
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
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!
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@chromium.org changed reviewers: + dglazkov@chromium.org
+dglazkov for owners review
Renaming done.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dmazzoni@chromium.org changed reviewers: + dcheng@chromium.org, mkwst@chromium.org
Ping for OWNERS review of WebKit/public and adding mkwst and dcheng...
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?
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@chromium.org changed reviewers: + haraken@chromium.org
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().
WebKit/public LGTM.
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
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.
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.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dmazzoni@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485204222353030, "parent_rev": "b82ab87dd4f38c3c7ca5d6d2e8e3a96220772222", "commit_rev": "22632c56f2fe702261795bb3cd3f76a717addf4a"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/22632c56f2fe702261795bb3cd3f... |