[webkit-reviews] review denied: [Bug 50237] [Gtk] Implement layoutTestController.findString : [Attachment 81221] Proposed Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Joone Hur <joone at kldp.org>'s
request for review:
Bug 50237: [Gtk] Implement layoutTestController.findString
https://bugs.webkit.org/show_bug.cgi?id=50237

Attachment 81221: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=81221&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81221&action=review

Thanks for implementing this! I think in this case, the next step is to ensure
that this functionality is part of the public API and then remove it from
DumpRenderTreeSupportGtk. This patch also needs to unskip tests which use this
functionality.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:548
> +/**
> + * findString:
> + * @webView: a #WebKitWebView
> + * @context: a JavaScript context 
> + * @targetString: a string to look for
> + * @optionsArray: an arrary containing search options
> + *
> + * Return value: TRUE if the string was found 
> + */

I know that we've added documentation like this to the file before, but I think
we can just remove this. It doesn't add anything because it just restates the
argument names.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:577
> +    JSRetainPtr<JSStringRef> lengthPropertyName(Adopt,
JSStringCreateWithUTF8CString("length"));
> +    JSValueRef lengthValue = JSObjectGetProperty(context, optionsArray,
lengthPropertyName.get(), 0); 
> +    if (!JSValueIsNumber(context, lengthValue))
> +	   return false;
> +
> +    WebCore::FindOptions options = 0;
> +    size_t length = static_cast<size_t>(JSValueToNumber(context,
lengthValue, 0));
> +    for (size_t i = 0; i < length; ++i) {
> +	   JSValueRef value = JSObjectGetPropertyAtIndex(context, optionsArray,
i, 0); 
> +	   if (!JSValueIsString(context, value))
> +	       continue;
> +		   
> +	   JSRetainPtr<JSStringRef> optionName(Adopt,
JSValueToStringCopy(context, value, 0));
> +
> +	   if (JSStringIsEqualToUTF8CString(optionName.get(),
"CaseInsensitive"))
> +	       options |= WebCore::CaseInsensitive;
> +	   else if (JSStringIsEqualToUTF8CString(optionName.get(),
"AtWordStarts"))
> +	       options |= WebCore::AtWordStarts;
> +	   else if (JSStringIsEqualToUTF8CString(optionName.get(),
"TreatMedialCapitalAsWordStart"))
> +	       options |= WebCore::TreatMedialCapitalAsWordStart;
> +	   else if (JSStringIsEqualToUTF8CString(optionName.get(),
"Backwards"))
> +	       options |= WebCore::Backwards;
> +	   else if (JSStringIsEqualToUTF8CString(optionName.get(),
"WrapAround"))
> +	       options |= WebCore::WrapAround;
> +	   else if (JSStringIsEqualToUTF8CString(optionName.get(),
"StartInSelection"))
> +	       options |= WebCore::StartInSelection;
> +    }

We shouldn't pass the JSC objects to DumpRenderSupportGtk unless we are forced
to. This is an implementation detail that DRTSupport shouldn't need to know.
Instead parse the values in LayoutTestController and use an enum defined in
DRTSupportGtk.