[Webkit-unassigned] [Bug 50237] [Gtk] Implement layoutTestController.findString

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50237


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #81221|review?                     |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2011-02-04 08:34:11 PST ---
(From update of attachment 81221)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.