Bug 131510 - [iOS WebKit2] Implement find-in-page indicator
Summary: [iOS WebKit2] Implement find-in-page indicator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on: 131560 131563
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-10 15:17 PDT by Tim Horton
Modified: 2014-04-11 16:32 PDT (History)
6 users (show)

See Also:


Attachments
patch (21.42 KB, patch)
2014-04-11 15:59 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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