[webkit-reviews] review denied: [Bug 112276] Allow multiple searchKeys to be passed to AXUIElementCopyParameterizedAttributeValue : [Attachment 193594] Patch v5, now with layout tests

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied  review:
Bug 112276: Allow multiple searchKeys to be passed to
AXUIElementCopyParameterizedAttributeValue
https://bugs.webkit.org/show_bug.cgi?id=112276

Attachment 193594: Patch v5, now with layout tests
https://bugs.webkit.org/attachment.cgi?id=193594&action=review

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


> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:945
> +		       if ( searchKeyParameter == nil )

this should just me if (searchKeyParameter)

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:939
> +	       JSObjectRef array = const_cast<JSObjectRef>(searchKey);

i don't think you want a const_cast. that's for removing const-ness. i think
you want a static case. however if there's a toJSObjectRef() method that would
be better

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:956
> +		       if ( searchKeyParameter == nil )

same issue here

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:958
> +		       [(NSMutableArray *)searchKeyParameter
addObject:[NSString stringWithJSStringRef:singleSearchKey]];

i think you should define the searchKeyParameter as the type your using it as
in each code block, so you don't have to cast like this

> LayoutTests/platform/mac/accessibility/search-predicate.html:266
> +	   // Heading OR link search

Comments should be a full sentence

> LayoutTests/platform/mac/accessibility/search-predicate.html:268
> +	   resultElement =
containerElement.uiElementForSearchPredicate(startElement, true,
["AXHeadingLevel2SearchKey","AXLinkSearchKey"], "");

space after the comma between the items in the array

> LayoutTests/platform/mac/accessibility/search-predicate.html:272
> +	   // continue from result element

ditto about comment

> LayoutTests/platform/mac/accessibility/search-predicate.html:273
> +	   resultElement =
containerElement.uiElementForSearchPredicate(resultElement, true,
["AXHeadingLevel2SearchKey","AXLinkSearchKey"], "");

ditto about comma spacing

> LayoutTests/platform/mac/accessibility/search-predicate.html:277
> +	   // going back should bring us back to the heading

ditto about comment

> LayoutTests/platform/mac/accessibility/search-predicate.html:278
> +	   resultElement =
containerElement.uiElementForSearchPredicate(resultElement, false,
["AXHeadingLevel2SearchKey","AXLinkSearchKey"], "");

ditto about comma spacing