Summary: | [Win] Implement layoutTestController.findString() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||||
Component: | Tools / Tests | Assignee: | Hyungwook Lee <hyungwook.lee> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, hyungwook.lee, mitz, ossy | ||||||||
Priority: | P2 | Keywords: | InRadar, LayoutTestFailure, PlatformOnly | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Windows XP | ||||||||||
Attachments: |
|
Description
Adam Roben (:aroben)
2010-11-30 06:52:08 PST
Created attachment 255477 [details]
Patch
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). Thanks for your feedback. Created attachment 255480 [details]
Patch
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! Created attachment 274621 [details]
Revised patch based on ToT and passing tests.
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 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 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. Committed in r198501 <http://trac.webkit.org/changeset/198501>. |