Bug 50234 - [Win] Implement layoutTestController.findString()
Summary: [Win] Implement layoutTestController.findString()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Hyungwook Lee
URL:
Keywords: InRadar, LayoutTestFailure, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-11-30 06:52 PST by Adam Roben (:aroben)
Modified: 2016-03-21 15:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.44 KB, patch)
2015-06-24 06:01 PDT, Hyungwook Lee
no flags Details | Formatted Diff | Diff
Patch (7.46 KB, patch)
2015-06-24 07:12 PDT, Hyungwook Lee
bfulgham: review-
Details | Formatted Diff | Diff
Revised patch based on ToT and passing tests. (16.15 KB, patch)
2016-03-21 13:53 PDT, Brent Fulgham
achristensen: review+
bfulgham: commit-queue-
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:52:08 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 Adam Roben (:aroben) 2010-11-30 06:55:05 PST
<rdar://problem/8710643>
Comment 2 Hyungwook Lee 2015-06-24 06:01:10 PDT
Created attachment 255477 [details]
Patch
Comment 3 Csaba Osztrogonác 2015-06-24 06:20:03 PDT
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).
Comment 4 Hyungwook Lee 2015-06-24 07:06:13 PDT
Thanks for your feedback.
Comment 5 Hyungwook Lee 2015-06-24 07:12:14 PDT
Created attachment 255480 [details]
Patch
Comment 6 Brent Fulgham 2015-08-13 09:02:00 PDT
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!
Comment 7 Brent Fulgham 2016-03-21 13:53:40 PDT
Created attachment 274621 [details]
Revised patch based on ToT and passing tests.
Comment 8 Brent Fulgham 2016-03-21 13:54:03 PDT
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.
Comment 9 Brent Fulgham 2016-03-21 14:32:23 PDT
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.
Comment 10 Alex Christensen 2016-03-21 14:48:49 PDT
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  <mitz@apple.com>

Don't want this, either.
Comment 11 Brent Fulgham 2016-03-21 15:22:45 PDT
Committed in r198501 <http://trac.webkit.org/changeset/198501>.