[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 #326899|review?                     |review+
              Flags|                            |

--- 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.

-- 
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/20171115/c425d294/attachment-0001.html>