[webkit-reviews] review denied: [Bug 127447] Web Inspector: AX: Accessibility Node Inspection : [Attachment 223506] patch with test coverage

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied  review:
Bug 127447: Web Inspector: AX: Accessibility Node Inspection
https://bugs.webkit.org/show_bug.cgi?id=127447

Attachment 223506: patch with test coverage
https://bugs.webkit.org/attachment.cgi?id=223506&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223506&action=review


> LayoutTests/accessibility/roles-computedRoleString-expected.txt:2
> +This tests that native elements and ARIA overrides result in the same ARIA
computed role, regardless of platform.

You should add an entry in TestExpectation in GTK marking this as failing until
computedRoleString is implemented

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2067
> +- (NSString*)computedRoleString

need a space before the *

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2113
> +    if ([attributeName isEqualToString: @"AXAriaRole"])

This should be AXARIARole and no space after the ":" even though other lines
are doing that.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:879
> +    Element* element = assertElement(errorString, nodeId);

Is there a reason you need to get the element here? you don't see to use it.
It seems just getting the node, and checking if that's nil should be good
enough

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1412
> +PassRefPtr<TypeBuilder::DOM::AccessibilityProperties>
InspectorDOMAgent::buildObjectForAccessibilityProperties(Node* node)

you should probably ASSERT(node) in here, and 
if (!node)
return nullptr

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1424
> +    String label = "";

i don't think you need to initialize these strings to anything

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1429
> +		   isAccessibilityObject = true;

this should probably check accessibilityIsIgnored() to determine true/false

> Source/WebCore/inspector/protocol/DOM.json:55
> +	       "description": "A structure holding EventListener properties."

this seems unrelated to this patch

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:598
> +    NSString* computedRoleString = descriptionOfValue([m_element
accessibilityAttributeValue:@"AXAriaRole"], m_element);

* on the wrong side

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:642
> +    NSString* computedRoleString = descriptionOfValue([m_element
accessibilityAttributeValue:@"AXAriaRole"], m_element);

* on wrong side