[Webkit-unassigned] [Bug 179494] AX: AOM: Implement AccessibleNode class and support label and role attributes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=179494

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #326609|review?                     |review-
              Flags|                            |

--- Comment #21 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 326609
  --> https://bugs.webkit.org/attachment.cgi?id=326609
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326609&action=review

> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:145
> -    const AtomicString& ariaLabel = getAttribute(aria_labelAttr);
> +    const AtomicString& ariaLabel = getPropertyStringValue(AXPropertyName::Label);

Again, please don't use "get" prefix. That should be only used when there is an out-argument,
or we're forced to use it because it's exposed as Web API.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2148
> +bool AccessibilityObject::hasPropertyValue(AXPropertyName propertyKey) const

We should call this hasProperty. "value" doesn't clarify anything beyond that.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2155
> +const String AccessibilityObject::getPropertyStringValue(AXPropertyName propertyKey) const

Don't use "get" prefix when a function doesn't have an out argument:
https://webkit.org/code-style-guidelines/#names-out-argument

Also, since this function returns the string value of a property
it's probably more appropriate to call it stringValueForProperty.

> Source/WebCore/accessibility/AccessibleNode.cpp:40
> +static ARIAAttributeMap* gARIAAttributeMap = nullptr;

We should use NeverDestroyed inside ariaAttributeMap.
Please see canAttachAuthorShadowRoot in Element.cpp and follow the same style.
r- because of this.

> Source/WebCore/accessibility/AccessibleNode.cpp:66
> +static bool isPropertyValueTypeString(AXPropertyName propertyName)

Is this function supposed to return true when the property value is a string?
Then it should recalled isPropertyValueString instead.

> Source/WebCore/accessibility/AccessibleNode.cpp:77
> +bool AccessibleNode::hasPropertyValue(Element* element, AXPropertyName propertyName)

Again, this should be called hasProperty.
Please make this function take Element& instead.

> Source/WebCore/accessibility/AccessibleNode.cpp:82
> +    if (AccessibleNode* accessibleNode = element->existingAccessibleNode()) {

Use auto*.

> Source/WebCore/accessibility/AccessibleNode.cpp:83
> +        if (accessibleNode->m_axPropertyMapping.contains(propertyName))

Why not just m_propertyMap? That naming matches ariaAttributeMap() better.
It certainly shouldn't have "ax" prefix since everything in AccessibleNode is related to accessibility by definition.

> Source/WebCore/accessibility/AccessibleNode.cpp:87
> +    // Fall back on the equivalent ARIA attribute.

This comment repeats what the code says. Please remove.

> Source/WebCore/accessibility/AccessibleNode.cpp:90
> +    

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.cpp:94
> +const PropertyValueVariant AccessibleNode::valueForProperty(Element* element, AXPropertyName propertyName)

Please make this take Element& instead.

> Source/WebCore/accessibility/AccessibleNode.cpp:99
> +    if (AccessibleNode* accessibleNode = element->existingAccessibleNode()) {

Use auto*.

> Source/WebCore/accessibility/AccessibleNode.cpp:103
> +    

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.cpp:109
> +    if (!element)

Use Element&.

> Source/WebCore/accessibility/AccessibleNode.cpp:119
> +    

Nit: Trailing whitespaces

> Source/WebCore/accessibility/AccessibleNode.h:67
> +    

Nit: Trailing whitespace.

> Source/WebCore/accessibility/AccessibleNode.h:70
> +    

Nit: Trailing whitespace.

> Source/WebCore/accessibility/AccessibleNode.h:73
> +    

Nit: Trailing whitespace.

> Source/WebCore/accessibility/AccessibleNode.h:76
> +    

Nit: Trailing whitespace.

> Source/WebCore/accessibility/AccessibleNode.h:87
> +    HashMap<AXPropertyName, PropertyValueVariant, WTF::IntHash<AXPropertyName>, AXPropertyHashTraits> m_axPropertyMapping;

Again, this should be called m_propertyMap.

> Source/WebCore/dom/Element.cpp:3697
> +    

Nit: Trailing whitespace.

> Source/WebCore/dom/Element.cpp:3708
> +    

Nit: Trailing whitespace.

> LayoutTests/accessibility/accessibility-object-model.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Please use modern DOCTYPE: <!DOCTYPE html>

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171114/cd7b1e05/attachment.html>