[webkit-reviews] review granted: [Bug 129781] Web Inspector: AXI: Expose checked/disabled/expanded/pressed/readonly/selected : [Attachment 226331] patch with review feedback

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted James Craig
<jcraig at apple.com>'s request for review:
Bug 129781: Web Inspector: AXI: Expose
checked/disabled/expanded/pressed/readonly/selected
https://bugs.webkit.org/show_bug.cgi?id=129781

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

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=226331&action=review


Looks good, but a lot of style nits and comments. r=me, but feel free to put up
another patch addressing these comments.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1441
> +		   int checkValue = axObject->checkboxOrRadioValue(); //
element using aria-checked

Comment style should be a sentence starting with a capital, ending with a
period.

    // Element using aria-checked.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1446
> +		   else if (axObject->isChecked()) // native checkbox

Ditto.

> Source/WebCore/inspector/protocol/DOM.json:64
> +		   { "name": "checked", "type": "string", "optional": true,
"enum": ["true", "false", "mixed"], "description": "Returns checked state if
supported." },
> +		   { "name": "disabled", "type": "boolean", "optional": true,
"description": "Returns disabled state if supported." },
>		   { "name": "exists", "type": "boolean", "description":
"Returns whether there is an existing AX object for the DOM node. If this is
false, all the other properties will be default values." },

Now that I think about it, these comments all say "Returns ..." but they aren't
returning anything. They are properties.

So:

    "Returns checked state if supported"

Can just be something like:

    "Checked state. Not available if not supported."

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:244
> +    _refreshAccessibility: (function(){

Nit: Although what you're doing here (running an anonymous function) is
probably fine, I would recommend against it. Just go with the normal pattern.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:249
> +	   var booleanValueToLocalizedStringIfTrue = function(property) {

Style: For each of these, change:

    var functionName = function() { ... }

To:

    function functionName() { ... }

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:270
> +	       // make sure the current set of properties is available in the
closure scope for the helper functions

Style: Same comment about comments being sentences. Capital and period. This
applies to all comments in the patch.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:363
> +	   var refreshAX = function(){

Style: Space between parens and opening brace: "function() {"

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:368
> +	      
domNode.accessibilityProperties(accessibilityPropertiesCallback.bind(this));

Oops. We should be feature checking if the backend supports
DOMAgent.getAccessibilityPropertiesForNode before attempting to do any of this
code at all, otherwise iOS 6 and iOS 7 will run this code and throw an
exception. I'll file a separate bug on this.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:372
> +

Style: Unnecessary blank line.

>
LayoutTests/inspector-protocol/dom/getAccessibilityPropertiesForNode-expected.t
xt:7
> +    label: 

There are no tests for label. Or if there are, label is always empty in the
output. Might be good to include a test for it.