Bug 50234 – [Win] Implement layoutTestController.findString()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[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).

[Adam Roben (:aroben)]

<rdar://problem/8710643>

[Hyungwook Lee]

Created attachment 255477 [details]
Patch

[Csaba Osztrogonác]

It seems it is a Windows only patch, because it was the last
platform hasn't implemented layoutTestController.findString yet.

It would be great to add [Win] prefix to the title (and the changelog too).

[Hyungwook Lee]

Thanks for your feedback.

[Hyungwook Lee]

Created attachment 255480 [details]
Patch

[Brent Fulgham]

Comment on attachment 255480 [details]
Patch

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

This patch looks great! I have to reject it because we need to move your new method to a new IWebViewPrivate interface to avoid breaking existing Windows clients.

> Source/WebKit/win/Interfaces/IWebViewPrivate.idl:310
> +    HRESULT findString([in] BSTR string, [in] WebFindOptions options, [out] BOOL* found);

This needs to be moved to a new interface, IWebViewPrivate2. We have existing IWebViewPrivate clients that rely on the COM binary interface to remain unchanged.

> Tools/DumpRenderTree/win/TestRunnerWin.cpp:930
> +    COMPtr<IWebViewPrivate> viewPrivate;

... this should become IWebViewPrivate2 so you can use the new method.

> Tools/DumpRenderTree/win/TestRunnerWin.cpp:966
> +    viewPrivate->findString(targetBSTR, static_cast<WebFindOptions>(options), &found);

Ideally, you should be checking the RESULT from 'findString' and handling the error case. But that may not matter in this test code. However, I think you should initialize 'found' to FALSE in case 'findString' bails out early leaving 'found' in an uninitialized state.

> LayoutTests/platform/win/TestExpectations:-1216
> -webkit.org/b/50234 editing/text-iterator/findString.html [ Skip ]

Yay!

[Brent Fulgham]

Created attachment 274621 [details]
Revised patch based on ToT and passing tests.

[Brent Fulgham]

Comment on attachment 274621 [details]
Revised patch based on ToT and passing tests.

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

> LayoutTests/platform/win/TestExpectations:138
> +#http/tests/download/ [ Skip ]

Whoops! Don't want this.

[Brent Fulgham]

Comment on attachment 274621 [details]
Revised patch based on ToT and passing tests.

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

> Source/WebKit/win/ChangeLog:22
> +        * WebDownloadCFNet.cpp: Remove extra CFRelease.

This second chunk of stuff shouldn't be here.

[Alex Christensen]

Comment on attachment 274621 [details]
Revised patch based on ToT and passing tests.

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

> Tools/ChangeLog:-1
> -2016-03-20  Dan Bernstein  <[e-mail]>

Don't want this, either.

[Brent Fulgham]

Committed in r198501 <http://trac.webkit.org/changeset/198501>.