[webkit-reviews] review granted: [Bug 131000] Need an Objective-C API or SPI for Find in Page : [Attachment 228231] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 1 20:38:31 PDT 2014
mitz at webkit.org <mitz at webkit.org> has granted Alice Liu <alice.liu at apple.com>'s
request for review:
Bug 131000: Need an Objective-C API or SPI for Find in Page
https://bugs.webkit.org/show_bug.cgi?id=131000
Attachment 228231: Patch
https://bugs.webkit.org/attachment.cgi?id=228231&action=review
------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=228231&action=review
Looks great! I can imagine a version of this API where there is no delegate,
and the “find” and “count” methods take completion handler blocks, but as SPI
this is a great start.
> Source/WebKit2/UIProcess/API/APIFindClient.h:43
> + virtual void didCountStringMatches(WebKit::WebPageProxy*, const
WTF::String&, uint32_t matchCount) { }
> + virtual void didFindString(WebKit::WebPageProxy*, const WTF::String&,
uint32_t matchCount) { }
> + virtual void didFailToFindString(WebKit::WebPageProxy*, const
WTF::String&) { }
No need to qualify String with WTF:: since WTFString.h, which you’re including,
says “using WTF::String”.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1227
> +- (UIView *)_hostForFindUI
Why not call this _viewForFindUI? “Host” has other meanings, and isn’t a term
we normally use to describe the role of a view.
> Source/WebKit2/UIProcess/Cocoa/FindClient.h:45
> + RetainPtr<id <_WKFindDelegate> > delegate();
No need for the space between the > >.
> Source/WebKit2/UIProcess/Cocoa/FindClient.h:55
> + WeakObjCPtr<id <_WKFindDelegate> > m_delegate;
Ditto.
> Source/WebKit2/UIProcess/Cocoa/FindClient.mm:40
> +RetainPtr<id <_WKFindDelegate> > FindClient::delegate()
Ditto.
> Source/WebKit2/UIProcess/Cocoa/FindClient.mm:45
> +void FindClient::setDelegate(id <_WKFindDelegate> delegate)
Ditto.
More information about the webkit-reviews
mailing list