[webkit-reviews] review granted: [Bug 179494] AX: AOM: Implement AccessibleNode class and support label and role attributes : [Attachment 326899] patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Nan Wang <n_wang at apple.com>'s
request for review:
Bug 179494: AX: AOM: Implement AccessibleNode class and support label and role
attributes
https://bugs.webkit.org/show_bug.cgi?id=179494

Attachment 326899: patch

https://bugs.webkit.org/attachment.cgi?id=326899&action=review




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

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

r=me assuming you can remove gc() call in the test.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2161
>      

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibilityObject.h:895
> +    

Nit: Trailing whitespaces.

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

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.cpp:43
> +struct AttributeEntry {
> +    AXPropertyName name;
> +    QualifiedName ariaAttribute;
> +};

There is no need to declare this time separately like this. Just declare it as
you declare the variable as in:
const struct AttributeEntry {
    AXPropertyName name;
    QualifiedName ariaAttribute;
} attributes[] = {
...
};
Or you could also use std::pair<AXPropertyName, QualifiedName> also.

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

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.cpp:118
> +	   if (m_propertyMap.contains(propertyName))
> +	       m_propertyMap.remove(propertyName);

Just call m_propertyMap.remove(propertyName).
There is no need to check contains(propertyName).

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

Nit: Trailing whitespaces.

> Source/WebCore/accessibility/AccessibleNode.h:39
> +typedef Variant<std::nullptr_t, String, bool, int> PropertyValueVariant;

When is nullptr_t used?

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

Nit: Trailing whitespaces.

> Source/WebCore/dom/Element.h:555
> +    

Nit: Trailing whitespaces.

> Source/WebCore/dom/Element.idl:106
> +    // Accessibility Object Model

There's no need to add this comment. The attribute is guarded with a runtime
flag named AccessibilityObjectModel.
Please remove.

> Source/WebCore/dom/ElementRareData.h:113
> +    

Nit: Trailing whitespaces.

> Source/WebCore/dom/ElementRareData.h:150
> +    

Nit: Trailing whitespaces.

> Source/WebCore/page/RuntimeEnabledFeatures.h:227
> +    

Nit: Trailing whitespaces.

> Source/WebCore/page/RuntimeEnabledFeatures.h:350
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:24
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:26
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:34
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:42
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:49
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:54
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:57
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:61
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:67
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:72
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:77
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:83
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:86
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:92
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:98
> +    

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:108
> +	   

Nit: Trailing whitespaces.

> LayoutTests/accessibility/accessibility-object-model.html:114
> +	   gc();

Why gc()? Something is not right if you need to call gc() to make the test run
property.

> LayoutTests/accessibility/accessibility-object-model.html:117
> +    

Nit: Trailing whitespaces.