Bug 50237 – [Gtk] Implement layoutTestController.findString

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

r72887 added layoutTestController.findString and r72889 added the one test that uses it to the Skipped list. We should implement layoutTestController.findString so that we can run that test (and any future tests that are added that use it).

[Joone Hur]

Created attachment 81221 [details]
Proposed Patch

[Martin Robinson]

Comment on attachment 81221 [details]
Proposed Patch

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.

[Joone Hur]

Created attachment 81345 [details]
Proposed Patch2

Yes, the current search API needs to be improved with the new search feature of WebCore.
https://bugs.webkit.org/show_bug.cgi?id=16624
https://bugs.webkit.org/show_bug.cgi?id=16584

Thanks, Martin.

[Martin Robinson]

Comment on attachment 81345 [details]
Proposed Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=81345&action=review

Awesome. Please consider one small change.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:39
> +enum {
> +    FindOptionsCaseInsensitive = 1 << 0,
> +    FindOptionsAtWordStarts = 1 << 1,
> +    FindOptionsTreatMedialCapitalAsWordStart = 1 << 2,
> +    FindOptionsBackwards = 1 << 3,
> +    FindOptionsWrapAround = 1 << 4,
> +    FindOptionsStartInSelection = 1 << 5
> +};

You should use something like COMPILE_ASSERT_MATCHING_ENUM from the src/WebKit/chromium source to ensure that these are equivalent to their WebCore counterparts.

[Joone Hur]

Created attachment 81397 [details]
Proposed Patch3

I have added AssertMatchingEnums.cpp like Chromium as Martin suggested.
It could be used to assert that various WebKit API enum values continue matching WebCore defined enum values.

[WebKit Review Bot]

Attachment 81397 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit/gtk/WebCoreSupport/AssertMatchingEnums.cpp:24:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 81397 [details]
Proposed Patch3

Great! Thank you!

[WebKit Commit Bot]

Comment on attachment 81397 [details]
Proposed Patch3

Clearing flags on attachment: 81397

Committed r77868: <http://trac.webkit.org/changeset/77868>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.