Bug 131510

Summary: [iOS WebKit2] Implement find-in-page indicator
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, alice.barraclough, commit-queue, enrica, sam, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 131560, 131563    
Bug Blocks:    
Attachments:
Description Flags
patch simon.fraser: review+

Description Tim Horton 2014-04-10 15:17:42 PDT
The current implementation is NSWindow based; that won't work.

<rdar://problem/16547777>
Comment 1 Tim Horton 2014-04-11 15:59:08 PDT
Created attachment 229172 [details]
patch
Comment 2 WebKit Commit Bot 2014-04-11 16:00:49 PDT
Attachment 229172 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/ios/FindIndicatorOverlayClientIOS.h:40:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/ios/FindIndicatorOverlayClientIOS.h:41:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/ios/FindIndicatorOverlayClientIOS.h:42:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Simon Fraser (smfr) 2014-04-11 16:04:30 PDT
Comment on attachment 229172 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229172&action=review

> Source/WebCore/ChangeLog:3
> +        [iOS WebKit2] Yellow find-in-page indicator

Does it being yellow matter?

> Source/WebCore/ChangeLog:9
> +        No new tests (OOPS!).

OOPS
Comment 4 Enrica Casucci 2014-04-11 16:12:48 PDT
Comment on attachment 229172 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229172&action=review

Looks good to me.

> Source/WebKit2/WebProcess/WebPage/ios/FindControllerIOS.mm:81
> +{

It would be nice to use Frame& and make Andreas happy :-)

> Source/WebKit2/WebProcess/WebPage/ios/FindControllerIOS.mm:121
> +{

You should probably add a FIXME to deal with subframes in the future.

> Source/WebKit2/WebProcess/WebPage/ios/FindIndicatorOverlayClientIOS.h:37
> +    void setFrame(WebCore::Frame* frame) { m_frame = frame; }

Reference?

> Source/WebKit2/WebProcess/WebPage/ios/FindIndicatorOverlayClientIOS.h:42
> +    virtual void didMoveToWebPage(PageOverlay*, WebPage*) override {}

Style requires a space between {}
Comment 5 Tim Horton 2014-04-11 16:32:02 PDT
http://trac.webkit.org/changeset/167169