Need an Objective-C API or SPI for Find in Page

• Component: WebKit2
• Status: RESOLVED FIXED

[Alice Liu]

WebKit2 has a C API for finding in page, but no Cocoa one.

[Alice Liu]

Created attachment 228229 [details]
patch

patch for build bot test, first.

[Alice Liu]

Created attachment 228230 [details]
Patch

[Alice Liu]

Created attachment 228231 [details]
Patch

[WebKit Commit Bot]

Attachment 228231 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:134:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Liu]

(In reply to comment #4)
> ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:43:  Place brace on its own line for function definitions.  [whitespace/braces] [4]

This isn't incorrect style for a NS_OPTIONS typedef.  Style for functions is being applied in the wrong context. 

> ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:134:  Missing spaces around =  [whitespace/operators] [4]

This isn't incorrect style for property declarations. This matches the style of the line directly above it. Style for assignment operator is being applied in the wrong context. 

> If any of these errors are false positives, please file a bug against check-webkit-style.

How to do this?  I did not find a component by the name of "check-webkit-style" when selecting the component for a new bug.

[Alice Liu]

The gtk-wk2 build failure is irrelevant.  It was caused by http://trac.webkit.org/changeset/166553.

Comment on attachment 228231 [details]
Patch

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.

[Alice Liu]

(In reply to comment #7)
> > 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”.

I tried removing WTF:: and it resulted in a compile time error in WebPageProxy.  I think the compiler is assuming API::String if the WTF:: is not present to clarify which String class. 

thanks for reviewing.

[Anders Carlsson]

(In reply to comment #8)
> (In reply to comment #7)

> > No need to qualify String with WTF:: since WTFString.h, which you’re including, says “using WTF::String”.
> 
> I tried removing WTF:: and it resulted in a compile time error in WebPageProxy.  I think the compiler is assuming API::String if the WTF:: is not present to clarify which String class. 
> 

Yeah it is needed to resolve the API::String vs WTF::String ambiguity.

[Alice Liu]

Committed r166658: <http://trac.webkit.org/changeset/166658>