[webkit-reviews] review denied: [Bug 127447] Web Inspector: AX: Accessibility Node Inspection : [Attachment 223619] patch with review feedback

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied James Craig <jcraig at apple.com>'s
request for review:
Bug 127447: Web Inspector: AX: Accessibility Node Inspection
https://bugs.webkit.org/show_bug.cgi?id=127447

Attachment 223619: patch with review feedback
https://bugs.webkit.org/attachment.cgi?id=223619&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223619&action=review


> Source/WebCore/accessibility/AccessibilityObject.cpp:1582
> +static ARIARoleMap* ariaRoleMap()

This should be: static ARIARoleMap& ariaRoleMap().

> Source/WebCore/accessibility/AccessibilityObject.cpp:1588
> +static ARIAReverseRoleMap* reverseAriaRoleMap()

This should be: static ARIAReverseRoleMap& reverseAriaRoleMap()

> Source/WebCore/accessibility/AccessibilityObject.cpp:1598
> +    ARIARoleMap* roleMap = ariaRoleMap();

Would use a reference here then. We are trying to remove pointers where
possible.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1618
> +    ARIAReverseRoleMap* roleMap = reverseAriaRoleMap();
> +    return roleMap->get(roleValue());

No need for the local, put on one line.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2114
> +    // AXARIARole is only used by DumpRenderTree (so far).
> +    if ([attributeName isEqualToString:@"AXARIARole"])
> +	   return [self computedRoleString];

Is this exposed to the web?

> Source/WebCore/inspector/protocol/DOM.json:61
> +		   { "name": "accessibilityIsIgnored", "type": "boolean",
"description": "Returns whether the DOM node is exposed to the accessibility
API." },

accessibilityIsIgnored is kind of wordy given that it is in
AccessibilityProperties object. This could just be "ignored" or "hidden"?

> Source/WebInspectorUI/UserInterface/DOMNode.js:447
> +    accessibilityProperties: function(callback)
> +    {
> +	   DOMAgent.getAccessibilityPropertiesForNode(this.id, callback);
> +    },

This leaks the protocol callback and result object into the Inspector. We try
to insulate the other classes by keeping all the protocol objects and callbacks
in the Manager classes. Some of the older DOM code still does not do this, but
new code should.

This should have a local callback that gets passed to
DOMAgent.getAccessibilityPropertiesForNode and that local callback checks for
errors, and makes a new object for the properties before calling the client
callback. This allows us to insulate the calls from future compatibility
changes.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:255
> +			   role = WebInspector.UIString("%s
(default)").replace("%s", role);

Use: WebInspector.UIString("%s (default)").format(role);

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:257
> +			   role = WebInspector.UIString("%s
(computed)").replace("%s", role);

Ditto.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:262
> +		   if (label && label != domNode.getAttribute("aria-label"))

!==

> Source/WebInspectorUI/UserInterface/DetailsSection.css:126
> -    table-layout: fixed;
> +    /* Removed 'table-layout: fixed;' because of WebKit CSS bug:
http://webkit.org/b/128294 */

Just remove the whole line. We don't commit commented out code. We might not
even need to add it back, layout seems fine without it.