Bug 50237 - [Gtk] Implement layoutTestController.findString
Summary: [Gtk] Implement layoutTestController.findString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, LayoutTestFailure, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-11-30 06:53 PST by Adam Roben (:aroben)
Modified: 2011-02-07 17:23 PST (History)
7 users (show)

See Also:


Attachments
Proposed Patch (7.14 KB, patch)
2011-02-04 08:28 PST, Joone Hur
mrobinson: review-
Details | Formatted Diff | Diff
Proposed Patch2 (6.92 KB, patch)
2011-02-04 20:42 PST, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch3 (9.98 KB, patch)
2011-02-06 05:22 PST, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-11-30 06:53:17 PST
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).
Comment 1 Joone Hur 2011-02-04 08:28:01 PST
Created attachment 81221 [details]
Proposed Patch
Comment 2 Martin Robinson 2011-02-04 08:34:11 PST
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.
Comment 3 Joone Hur 2011-02-04 20:42:14 PST
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.
Comment 4 Martin Robinson 2011-02-05 10:38:23 PST
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.
Comment 5 Joone Hur 2011-02-06 05:22:57 PST
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.
Comment 6 WebKit Review Bot 2011-02-06 05:25:42 PST
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.
Comment 7 Martin Robinson 2011-02-07 08:09:10 PST
Comment on attachment 81397 [details]
Proposed Patch3

Great! Thank you!
Comment 8 WebKit Commit Bot 2011-02-07 17:23:44 PST
Comment on attachment 81397 [details]
Proposed Patch3

Clearing flags on attachment: 81397

Committed r77868: <http://trac.webkit.org/changeset/77868>
Comment 9 WebKit Commit Bot 2011-02-07 17:23:49 PST
All reviewed patches have been landed.  Closing bug.