[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]

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.