[Mac] -[WKWebView findMatchesForString:relativeToMatch:findOptions:maxResults:resultC...
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Apr 2017 19:59:52 +0000 (19:59 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Apr 2017 19:59:52 +0000 (19:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165801
<rdar://problem/29649535>

Reviewed by Wenson Hsieh.

New API tests: WebKit2.FindInPageWrapping*

Previously, when doing an incremental find that wrapped, we would
say that it did not, leading NSTextFinder to not provide its usual
wrapping UI, and other clients of the NSTextFinderClient protocol to
get confused by the lack of wrapping.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didFindString):
* UIProcess/WebPageProxy.h:
* UIProcess/WebPageProxy.messages.in:
* UIProcess/API/APIFindClient.h:
(API::FindClient::didFindString):
* UIProcess/API/C/WKPage.cpp:
(WKPageSetPageFindClient):
* UIProcess/Cocoa/FindClient.h:
* UIProcess/Cocoa/FindClient.mm:
(WebKit::FindClient::didFindString):
* WebProcess/WebPage/FindController.cpp:
(WebKit::FindController::updateFindUIAfterPageScroll):
(WebKit::FindController::findString):
* WebProcess/WebPage/FindController.h:
Plumb DidWrap from FindController's call to findString back through
the DidFindString message.

* UIProcess/mac/WKTextFinderClient.mm:
(-[WKTextFinderClient didFindStringMatchesWithRects:didWrapAround:]):
(-[WKTextFinderClient didFindStringMatchesWithRects:]): Deleted.
Make use of the new DidWrap information to stop lying to NSTextFinder
about whether a wrap actually occurred.

* page/FrameTree.cpp:
(WebCore::FrameTree::traverseNextWithWrap):
(WebCore::FrameTree::traversePreviousWithWrap):
(WebCore::FrameTree::traverseNextInPostOrderWithWrap):
* page/FrameTree.h:
Add CanWrap and DidWrap boolean enums, and add an optional out argument
to traverse*WithWrap indicating whether a wrap actually occurred.

* history/CachedPage.cpp:
(WebCore::firePageShowAndPopStateEvents):
* history/PageCache.cpp:
(WebCore::destroyRenderTree):
Adjust to the new CanWrap enum.

* page/Page.cpp:
(WebCore::incrementFrame):
(WebCore::Page::findString):
(WebCore::Page::findStringMatchingRanges):
(WebCore::Page::rangeOfString):
(WebCore::Page::findMatchesForText):
(WebCore::Page::unmarkAllTextMatches):
* page/Page.h:
Adjust to the new CanWrap enum, and optionally plumb DidWrap through
to callers of findString().

* WebView/WebView.mm:
(incrementFrame):
Adjust to the new CanWrap enum.

* TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:
(TEST):
Add some tests for wrapping finds.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@214893 268f45cc-cd09-0410-ab3c-d52691b4dbfc

24 files changed:
Source/WebCore/ChangeLog
Source/WebCore/history/CachedPage.cpp
Source/WebCore/history/PageCache.cpp
Source/WebCore/page/FrameTree.cpp
Source/WebCore/page/FrameTree.h
Source/WebCore/page/Page.cpp
Source/WebCore/page/Page.h
Source/WebKit/mac/ChangeLog
Source/WebKit/mac/WebView/WebView.mm
Source/WebKit/win/WebView.cpp
Source/WebKit2/ChangeLog
Source/WebKit2/UIProcess/API/APIFindClient.h
Source/WebKit2/UIProcess/API/C/WKPage.cpp
Source/WebKit2/UIProcess/Cocoa/FindClient.h
Source/WebKit2/UIProcess/Cocoa/FindClient.mm
Source/WebKit2/UIProcess/WebPageProxy.cpp
Source/WebKit2/UIProcess/WebPageProxy.h
Source/WebKit2/UIProcess/WebPageProxy.messages.in
Source/WebKit2/UIProcess/ios/WKPDFView.mm
Source/WebKit2/UIProcess/mac/WKTextFinderClient.mm
Source/WebKit2/WebProcess/WebPage/FindController.cpp
Source/WebKit2/WebProcess/WebPage/FindController.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm

index 78a0bbe..34fb115 100644 (file)
@@ -1,3 +1,38 @@
+2017-04-04  Tim Horton  <timothy_horton@apple.com>
+
+        [Mac] -[WKWebView findMatchesForString:relativeToMatch:findOptions:maxResults:resultCollector:] invokes the resultCollector with didWrap = NO even when it wraps
+        https://bugs.webkit.org/show_bug.cgi?id=165801
+        <rdar://problem/29649535>
+
+        Reviewed by Wenson Hsieh.
+
+        New API tests: WebKit2.FindInPageWrapping*
+
+        * page/FrameTree.cpp:
+        (WebCore::FrameTree::traverseNextWithWrap):
+        (WebCore::FrameTree::traversePreviousWithWrap):
+        (WebCore::FrameTree::traverseNextInPostOrderWithWrap):
+        * page/FrameTree.h:
+        Add CanWrap and DidWrap boolean enums, and add an optional out argument
+        to traverse*WithWrap indicating whether a wrap actually occurred.
+
+        * history/CachedPage.cpp:
+        (WebCore::firePageShowAndPopStateEvents):
+        * history/PageCache.cpp:
+        (WebCore::destroyRenderTree):
+        Adjust to the new CanWrap enum.
+        
+        * page/Page.cpp:
+        (WebCore::incrementFrame):
+        (WebCore::Page::findString):
+        (WebCore::Page::findStringMatchingRanges):
+        (WebCore::Page::rangeOfString):
+        (WebCore::Page::findMatchesForText):
+        (WebCore::Page::unmarkAllTextMatches):
+        * page/Page.h:
+        Adjust to the new CanWrap enum, and optionally plumb DidWrap through
+        to callers of findString().
+
 2017-04-04  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK] PLATFORM(GTK) && !USE(COORDINATED_GRAPHICS_THREADED) is no longer possible
index 3ddd9c1..34d8ad0 100644 (file)
@@ -78,7 +78,7 @@ static void firePageShowAndPopStateEvents(Page& page)
     // Dispatching JavaScript events can cause frame destruction.
     auto& mainFrame = page.mainFrame();
     Vector<Ref<Frame>> childFrames;
-    for (auto* child = mainFrame.tree().traverseNextInPostOrderWithWrap(true); child; child = child->tree().traverseNextInPostOrderWithWrap(false))
+    for (auto* child = mainFrame.tree().traverseNextInPostOrder(CanWrap::Yes); child; child = child->tree().traverseNextInPostOrder(CanWrap::No))
         childFrames.append(*child);
 
     for (auto& child : childFrames) {
index abb03ff..4b347de 100644 (file)
@@ -374,7 +374,7 @@ static void setPageCacheState(Page& page, Document::PageCacheState pageCacheStat
 // Note that destruction happens bottom-up so that the main frame's tree dies last.
 static void destroyRenderTree(MainFrame& mainFrame)
 {
-    for (Frame* frame = mainFrame.tree().traversePreviousWithWrap(true); frame; frame = frame->tree().traversePreviousWithWrap(false)) {
+    for (Frame* frame = mainFrame.tree().traversePrevious(CanWrap::Yes); frame; frame = frame->tree().traversePrevious(CanWrap::No)) {
         if (!frame->document())
             continue;
         auto& document = *frame->document();
index 4e1bc29..b3b9f12 100644 (file)
@@ -403,18 +403,21 @@ Frame* FrameTree::traverseNextRendered(const Frame* stayWithin) const
     return nullptr;
 }
 
-Frame* FrameTree::traverseNextWithWrap(bool wrap) const
+Frame* FrameTree::traverseNext(CanWrap canWrap, DidWrap* didWrap) const
 {
     if (Frame* result = traverseNext())
         return result;
 
-    if (wrap)
+    if (canWrap == CanWrap::Yes) {
+        if (didWrap)
+            *didWrap = DidWrap::Yes;
         return &m_thisFrame.mainFrame();
+    }
 
     return nullptr;
 }
 
-Frame* FrameTree::traversePreviousWithWrap(bool wrap) const
+Frame* FrameTree::traversePrevious(CanWrap canWrap, DidWrap* didWrap) const
 {
     // FIXME: besides the wrap feature, this is just the traversePreviousNode algorithm
 
@@ -424,20 +427,23 @@ Frame* FrameTree::traversePreviousWithWrap(bool wrap) const
         return parentFrame;
     
     // no siblings, no parent, self==top
-    if (wrap)
+    if (canWrap == CanWrap::Yes) {
+        if (didWrap)
+            *didWrap = DidWrap::Yes;
         return deepLastChild();
+    }
 
     // top view is always the last one in this ordering, so prev is nil without wrap
     return nullptr;
 }
 
-Frame* FrameTree::traverseNextInPostOrderWithWrap(bool wrap) const
+Frame* FrameTree::traverseNextInPostOrder(CanWrap canWrap) const
 {
     if (m_nextSibling)
         return m_nextSibling->tree().deepFirstChild();
     if (m_parent)
         return m_parent;
-    if (wrap)
+    if (canWrap == CanWrap::Yes)
         return deepFirstChild();
     return nullptr;
 }
index 052f360..c63b4d1 100644 (file)
@@ -23,6 +23,9 @@
 
 namespace WebCore {
 
+enum class CanWrap : bool { No, Yes };
+enum class DidWrap : bool { No, Yes };
+
 class Frame;
 class TreeScope;
 
@@ -62,10 +65,10 @@ public:
     WEBCORE_EXPORT Frame* traverseNext(const Frame* stayWithin = nullptr) const;
     // Rendered means being the main frame or having an ownerRenderer. It may not have been parented in the Widget tree yet (see WidgetHierarchyUpdatesSuspensionScope).
     WEBCORE_EXPORT Frame* traverseNextRendered(const Frame* stayWithin = nullptr) const;
-    WEBCORE_EXPORT Frame* traverseNextWithWrap(bool) const;
-    WEBCORE_EXPORT Frame* traversePreviousWithWrap(bool) const;
+    WEBCORE_EXPORT Frame* traverseNext(CanWrap, DidWrap* = nullptr) const;
+    WEBCORE_EXPORT Frame* traversePrevious(CanWrap, DidWrap* = nullptr) const;
 
-    Frame* traverseNextInPostOrderWithWrap(bool) const;
+    Frame* traverseNextInPostOrder(CanWrap) const;
 
     WEBCORE_EXPORT void appendChild(Frame&);
     void detachFromParent() { m_parent = nullptr; }
index 30f0812..01da0c2 100644 (file)
@@ -647,19 +647,19 @@ void Page::setCanStartMedia(bool canStartMedia)
     }
 }
 
-static Frame* incrementFrame(Frame* curr, bool forward, bool wrapFlag)
+static Frame* incrementFrame(Frame* curr, bool forward, CanWrap canWrap, DidWrap* didWrap = nullptr)
 {
     return forward
-        ? curr->tree().traverseNextWithWrap(wrapFlag)
-        : curr->tree().traversePreviousWithWrap(wrapFlag);
+        ? curr->tree().traverseNext(canWrap, didWrap)
+        : curr->tree().traversePrevious(canWrap, didWrap);
 }
 
-bool Page::findString(const String& target, FindOptions options)
+bool Page::findString(const String& target, FindOptions options, DidWrap* didWrap)
 {
     if (target.isEmpty())
         return false;
 
-    bool shouldWrap = options & WrapAround;
+    CanWrap canWrap = options & WrapAround ? CanWrap::Yes : CanWrap::No;
     Frame* frame = &focusController().focusedOrMainFrame();
     Frame* startFrame = frame;
     do {
@@ -669,12 +669,14 @@ bool Page::findString(const String& target, FindOptions options)
             focusController().setFocusedFrame(frame);
             return true;
         }
-        frame = incrementFrame(frame, !(options & Backwards), shouldWrap);
+        frame = incrementFrame(frame, !(options & Backwards), canWrap, didWrap);
     } while (frame && frame != startFrame);
 
     // Search contents of startFrame, on the other side of the selection that we did earlier.
     // We cheat a bit and just research with wrap on
-    if (shouldWrap && !startFrame->selection().isNone()) {
+    if (canWrap == CanWrap::Yes && !startFrame->selection().isNone()) {
+        if (didWrap)
+            *didWrap = DidWrap::Yes;
         bool found = startFrame->editor().findString(target, options | WrapAround | StartInSelection);
         focusController().setFocusedFrame(frame);
         return found;
@@ -693,7 +695,7 @@ void Page::findStringMatchingRanges(const String& target, FindOptions options, i
         frame->editor().countMatchesForText(target, 0, options, limit ? (limit - matchRanges.size()) : 0, true, &matchRanges);
         if (frame->selection().isRange())
             frameWithSelection = frame;
-        frame = incrementFrame(frame, true, false);
+        frame = incrementFrame(frame, true, CanWrap::No);
     } while (frame);
 
     if (matchRanges.isEmpty())
@@ -735,19 +737,19 @@ RefPtr<Range> Page::rangeOfString(const String& target, Range* referenceRange, F
     if (referenceRange && referenceRange->ownerDocument().page() != this)
         return nullptr;
 
-    bool shouldWrap = options & WrapAround;
+    CanWrap canWrap = options & WrapAround ? CanWrap::Yes : CanWrap::No;
     Frame* frame = referenceRange ? referenceRange->ownerDocument().frame() : &mainFrame();
     Frame* startFrame = frame;
     do {
         if (RefPtr<Range> resultRange = frame->editor().rangeOfString(target, frame == startFrame ? referenceRange : 0, options & ~WrapAround))
             return resultRange;
 
-        frame = incrementFrame(frame, !(options & Backwards), shouldWrap);
+        frame = incrementFrame(frame, !(options & Backwards), canWrap);
     } while (frame && frame != startFrame);
 
     // Search contents of startFrame, on the other side of the reference range that we did earlier.
     // We cheat a bit and just search again with wrap on.
-    if (shouldWrap && referenceRange) {
+    if (canWrap == CanWrap::Yes && referenceRange) {
         if (RefPtr<Range> resultRange = startFrame->editor().rangeOfString(target, referenceRange, options | WrapAround | StartInSelection))
             return resultRange;
     }
@@ -767,7 +769,7 @@ unsigned Page::findMatchesForText(const String& target, FindOptions options, uns
         if (shouldMarkMatches == MarkMatches)
             frame->editor().setMarkedTextMatchesAreHighlighted(shouldHighlightMatches == HighlightMatches);
         matchCount += frame->editor().countMatchesForText(target, 0, options, maxMatchCount ? (maxMatchCount - matchCount) : 0, shouldMarkMatches == MarkMatches, 0);
-        frame = incrementFrame(frame, true, false);
+        frame = incrementFrame(frame, true, CanWrap::No);
     } while (frame);
 
     return matchCount;
@@ -788,7 +790,7 @@ void Page::unmarkAllTextMatches()
     Frame* frame = &mainFrame();
     do {
         frame->document()->markers().removeMarkers(DocumentMarker::TextMatch);
-        frame = incrementFrame(frame, true, false);
+        frame = incrementFrame(frame, true, CanWrap::No);
     } while (frame);
 }
 
index 1202750..c15aee6 100644 (file)
@@ -145,6 +145,9 @@ enum class EventThrottlingBehavior {
     Unresponsive
 };
 
+enum class CanWrap : bool;
+enum class DidWrap : bool;
+
 class Page : public Supplementable<Page> {
     WTF_MAKE_NONCOPYABLE(Page);
     WTF_MAKE_FAST_ALLOCATED;
@@ -264,7 +267,7 @@ public:
     void setTabKeyCyclesThroughElements(bool b) { m_tabKeyCyclesThroughElements = b; }
     bool tabKeyCyclesThroughElements() const { return m_tabKeyCyclesThroughElements; }
 
-    WEBCORE_EXPORT bool findString(const String&, FindOptions);
+    WEBCORE_EXPORT bool findString(const String&, FindOptions, DidWrap* = nullptr);
 
     WEBCORE_EXPORT RefPtr<Range> rangeOfString(const String&, Range*, FindOptions);
 
index 4c917d3..48beac7 100644 (file)
@@ -1,3 +1,15 @@
+2017-04-04  Tim Horton  <timothy_horton@apple.com>
+
+        [Mac] -[WKWebView findMatchesForString:relativeToMatch:findOptions:maxResults:resultCollector:] invokes the resultCollector with didWrap = NO even when it wraps
+        https://bugs.webkit.org/show_bug.cgi?id=165801
+        <rdar://problem/29649535>
+
+        Reviewed by Wenson Hsieh.
+
+        * WebView/WebView.mm:
+        (incrementFrame):
+        Adjust to the new CanWrap enum.
+
 2017-03-27  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Saving files should not suggest the top level directory
index caa7223..a2bdee1 100644 (file)
@@ -6796,9 +6796,10 @@ static NSString * const backingPropertyOldScaleFactorKey = @"NSBackingPropertyOl
 static WebFrame *incrementFrame(WebFrame *frame, WebFindOptions options = 0)
 {
     Frame* coreFrame = core(frame);
+    CanWrap canWrap = options & WebFindOptionsWrapAround ? CanWrap::Yes : CanWrap::No;
     return kit((options & WebFindOptionsBackwards)
-        ? coreFrame->tree().traversePreviousWithWrap(options & WebFindOptionsWrapAround)
-        : coreFrame->tree().traverseNextWithWrap(options & WebFindOptionsWrapAround));
+        ? coreFrame->tree().traversePrevious(canWrap)
+        : coreFrame->tree().traverseNext(canWrap));
 }
 
 - (BOOL)searchFor:(NSString *)string direction:(BOOL)forward caseSensitive:(BOOL)caseFlag wrap:(BOOL)wrapFlag
index 3175be3..96a2746 100644 (file)
@@ -3856,9 +3856,10 @@ HRESULT WebView::hostWindow(_Deref_opt_out_ HWND* window)
 
 static Frame *incrementFrame(Frame *curr, bool forward, bool wrapFlag)
 {
+    CanWrap canWrap = wrapFlag ? CanWrap::Yes : CanWrap::No;
     return forward
-        ? curr->tree().traverseNextWithWrap(wrapFlag)
-        : curr->tree().traversePreviousWithWrap(wrapFlag);
+        ? curr->tree().traverseNext(canWrap)
+        : curr->tree().traversePrevious(canWrap);
 }
 
 HRESULT WebView::searchFor(_In_ BSTR str, BOOL forward, BOOL caseFlag, BOOL wrapFlag, _Out_ BOOL* found)
index 966c623..56ae473 100644 (file)
@@ -1,3 +1,40 @@
+2017-04-04  Tim Horton  <timothy_horton@apple.com>
+
+        [Mac] -[WKWebView findMatchesForString:relativeToMatch:findOptions:maxResults:resultCollector:] invokes the resultCollector with didWrap = NO even when it wraps
+        https://bugs.webkit.org/show_bug.cgi?id=165801
+        <rdar://problem/29649535>
+
+        Reviewed by Wenson Hsieh.
+
+        Previously, when doing an incremental find that wrapped, we would 
+        say that it did not, leading NSTextFinder to not provide its usual
+        wrapping UI, and other clients of the NSTextFinderClient protocol to
+        get confused by the lack of wrapping.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::didFindString):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebPageProxy.messages.in:
+        * UIProcess/API/APIFindClient.h:
+        (API::FindClient::didFindString):
+        * UIProcess/API/C/WKPage.cpp:
+        (WKPageSetPageFindClient):
+        * UIProcess/Cocoa/FindClient.h:
+        * UIProcess/Cocoa/FindClient.mm:
+        (WebKit::FindClient::didFindString):
+        * WebProcess/WebPage/FindController.cpp:
+        (WebKit::FindController::updateFindUIAfterPageScroll):
+        (WebKit::FindController::findString):
+        * WebProcess/WebPage/FindController.h:
+        Plumb DidWrap from FindController's call to findString back through
+        the DidFindString message.
+
+        * UIProcess/mac/WKTextFinderClient.mm:
+        (-[WKTextFinderClient didFindStringMatchesWithRects:didWrapAround:]):
+        (-[WKTextFinderClient didFindStringMatchesWithRects:]): Deleted.
+        Make use of the new DidWrap information to stop lying to NSTextFinder
+        about whether a wrap actually occurred.
+
 2017-04-03  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         Data interaction should register type identifiers in order of priority
index 0abf8f6..f3123af 100644 (file)
@@ -43,7 +43,7 @@ public:
     virtual ~FindClient() { }
 
     virtual void didCountStringMatches(WebKit::WebPageProxy*, const WTF::String&, uint32_t) { }
-    virtual void didFindString(WebKit::WebPageProxy*, const WTF::String&, const Vector<WebCore::IntRect>& matchRects, uint32_t, int32_t) { }
+    virtual void didFindString(WebKit::WebPageProxy*, const WTF::String&, const Vector<WebCore::IntRect>& matchRects, uint32_t, int32_t, bool didWrapAround) { }
     virtual void didFailToFindString(WebKit::WebPageProxy*, const WTF::String&) { }
 };
 
index 91e15b8..136272c 100644 (file)
@@ -931,7 +931,7 @@ void WKPageSetPageFindClient(WKPageRef pageRef, const WKPageFindClientBase* wkCl
         }
 
     private:
-        void didFindString(WebPageProxy* page, const String& string, const Vector<WebCore::IntRect>&, uint32_t matchCount, int32_t) override
+        void didFindString(WebPageProxy* page, const String& string, const Vector<WebCore::IntRect>&, uint32_t matchCount, int32_t, bool didWrapAround) override
         {
             if (!m_client.didFindString)
                 return;
index aa6f709..6d46936 100644 (file)
@@ -48,7 +48,7 @@ public:
 private:
     // From API::FindClient
     virtual void didCountStringMatches(WebPageProxy*, const String&, uint32_t matchCount);
-    virtual void didFindString(WebPageProxy*, const String&, const Vector<WebCore::IntRect>&, uint32_t matchCount, int32_t matchIndex);
+    virtual void didFindString(WebPageProxy*, const String&, const Vector<WebCore::IntRect>&, uint32_t matchCount, int32_t matchIndex, bool didWrapAround);
     virtual void didFailToFindString(WebPageProxy*, const String&);
     
     WKWebView *m_webView;
index 447266c..880c4cf 100644 (file)
@@ -57,7 +57,7 @@ void FindClient::didCountStringMatches(WebPageProxy*, const String& string, uint
         [m_delegate.get() _webView:m_webView didCountMatches:matchCount forString:string];
 }
 
-void FindClient::didFindString(WebPageProxy*, const String& string, const Vector<WebCore::IntRect>&, uint32_t matchCount, int32_t matchIndex)
+void FindClient::didFindString(WebPageProxy*, const String& string, const Vector<WebCore::IntRect>&, uint32_t matchCount, int32_t matchIndex, bool)
 {
     if (m_delegateMethods.webviewDidFindString)
         [m_delegate.get() _webView:m_webView didFindMatches:matchCount forString:string withMatchIndex:matchIndex];
index 1506e5e..ae786ec 100644 (file)
@@ -4466,9 +4466,9 @@ void WebPageProxy::setTextIndicatorAnimationProgress(float progress)
 #endif
 }
 
-void WebPageProxy::didFindString(const String& string, const Vector<WebCore::IntRect>& matchRects, uint32_t matchCount, int32_t matchIndex)
+void WebPageProxy::didFindString(const String& string, const Vector<WebCore::IntRect>& matchRects, uint32_t matchCount, int32_t matchIndex, bool didWrapAround)
 {
-    m_findClient->didFindString(this, string, matchRects, matchCount, matchIndex);
+    m_findClient->didFindString(this, string, matchRects, matchCount, matchIndex, didWrapAround);
 }
 
 void WebPageProxy::didFindStringMatches(const String& string, const Vector<Vector<WebCore::IntRect>>& matchRects, int32_t firstIndexAfterSelection)
index acbdfbb..058c59b 100644 (file)
@@ -788,7 +788,7 @@ public:
     void setTextIndicator(const WebCore::TextIndicatorData&, uint64_t /* WebCore::TextIndicatorWindowLifetime */ lifetime = 0 /* Permanent */);
     void setTextIndicatorAnimationProgress(float);
     void clearTextIndicator();
-    void didFindString(const String&, const Vector<WebCore::IntRect>&, uint32_t matchCount, int32_t matchIndex);
+    void didFindString(const String&, const Vector<WebCore::IntRect>&, uint32_t matchCount, int32_t matchIndex, bool didWrapAround);
     void didFailToFindString(const String&);
     void didFindStringMatches(const String&, const Vector<Vector<WebCore::IntRect>>& matchRects, int32_t firstIndexAfterSelection);
 
index e92109a..f6c54c7 100644 (file)
@@ -241,7 +241,7 @@ messages -> WebPageProxy {
     DidCountStringMatches(String string, uint32_t matchCount)
     SetTextIndicator(struct WebCore::TextIndicatorData indicator, uint64_t lifetime)
     ClearTextIndicator()
-    DidFindString(String string, Vector<WebCore::IntRect> matchRect, uint32_t matchCount, int32_t matchIndex)
+    DidFindString(String string, Vector<WebCore::IntRect> matchRect, uint32_t matchCount, int32_t matchIndex, bool didWrapAround)
     DidFailToFindString(String string)
     DidFindStringMatches(String string, Vector<Vector<WebCore::IntRect>> matches, int32_t firstIndexAfterSelection)
     DidGetImageForFindMatch(WebKit::ShareableBitmap::Handle contentImageHandle, uint32_t matchIndex)
index a69c0c5..e292dd8 100644 (file)
@@ -614,7 +614,7 @@ static NSStringCompareOptions stringCompareOptions(_WKFindOptions options)
                 _currentFindMatchIndex = 0;
                 for (const auto& knownMatch : _cachedFindMatches) {
                     if (match.stringRange.location == [knownMatch stringRange].location && match.stringRange.length == [knownMatch stringRange].length) {
-                        page->findClient().didFindString(page.get(), string, { }, _cachedFindMatches.size(), _currentFindMatchIndex);
+                        page->findClient().didFindString(page.get(), string, { }, _cachedFindMatches.size(), _currentFindMatchIndex, false);
                         break;
                     }
                     _currentFindMatchIndex++;
index f346553..b554d17 100644 (file)
@@ -47,7 +47,7 @@ static const NSUInteger maximumFindMatches = 1000;
 
 @interface WKTextFinderClient ()
 
-- (void)didFindStringMatchesWithRects:(const Vector<Vector<IntRect>>&)rects;
+- (void)didFindStringMatchesWithRects:(const Vector<Vector<IntRect>>&)rects didWrapAround:(BOOL)didWrapAround;
 
 - (void)getImageForMatchResult:(id <NSTextFinderAsynchronousDocumentFindMatch>)findMatch completionHandler:(void (^)(NSImage *generatedImage))completionHandler;
 - (void)didGetImageForMatchResult:(WebImage *)string;
@@ -66,7 +66,7 @@ public:
 private:
     void didFindStringMatches(WebPageProxy* page, const String&, const Vector<Vector<IntRect>>& matchRects, int32_t) override
     {
-        [m_textFinderClient didFindStringMatchesWithRects:matchRects];
+        [m_textFinderClient didFindStringMatchesWithRects:matchRects didWrapAround:NO];
     }
 
     void didGetImageForMatchResult(WebPageProxy* page, WebImage* image, int32_t index) override
@@ -74,14 +74,14 @@ private:
         [m_textFinderClient didGetImageForMatchResult:image];
     }
 
-    void didFindString(WebPageProxy*, const String&, const Vector<IntRect>& matchRects, uint32_t, int32_t) override
+    void didFindString(WebPageProxy*, const String&, const Vector<IntRect>& matchRects, uint32_t, int32_t, bool didWrapAround) override
     {
-        [m_textFinderClient didFindStringMatchesWithRects:{ matchRects }];
+        [m_textFinderClient didFindStringMatchesWithRects:{ matchRects } didWrapAround:didWrapAround];
     }
 
     void didFailToFindString(WebPageProxy*, const String& string) override
     {
-        [m_textFinderClient didFindStringMatchesWithRects:{ }];
+        [m_textFinderClient didFindStringMatchesWithRects:{ } didWrapAround:NO];
     }
 
     RetainPtr<WKTextFinderClient> m_textFinderClient;
@@ -234,7 +234,7 @@ static RetainPtr<NSArray> arrayFromRects(const Vector<IntRect>& matchRects)
     return nsMatchRects;
 }
 
-- (void)didFindStringMatchesWithRects:(const Vector<Vector<IntRect>>&)rectsForMatches
+- (void)didFindStringMatchesWithRects:(const Vector<Vector<IntRect>>&)rectsForMatches didWrapAround:(BOOL)didWrapAround
 {
     if (_findReplyCallbacks.isEmpty())
         return;
@@ -248,7 +248,7 @@ static RetainPtr<NSArray> arrayFromRects(const Vector<IntRect>& matchRects)
         [matchObjects addObject:match.get()];
     }
 
-    replyCallback(matchObjects.get(), NO);
+    replyCallback(matchObjects.get(), didWrapAround);
 }
 
 - (void)didGetImageForMatchResult:(WebImage *)image
@@ -277,6 +277,9 @@ static RetainPtr<NSArray> arrayFromRects(const Vector<IntRect>& matchRects)
         Block_release(copiedImageCallback);
     });
 
+    // FIXME: There is no guarantee that this will ever result in didGetImageForMatchResult
+    // being called (and thus us calling our completion handler); we should harden this
+    // against all of the early returns in FindController::getImageForFindMatch.
     _page->getImageForFindMatch(textFinderMatch.index);
 }
 
index 33032c1..241c52f 100644 (file)
@@ -114,7 +114,7 @@ static Frame* frameWithSelection(Page* page)
     return 0;
 }
 
-void FindController::updateFindUIAfterPageScroll(bool found, const String& string, FindOptions options, unsigned maxMatchCount)
+void FindController::updateFindUIAfterPageScroll(bool found, const String& string, FindOptions options, unsigned maxMatchCount, DidWrap didWrap)
 {
     Frame* selectedFrame = frameWithSelection(m_webPage->corePage());
     
@@ -181,7 +181,7 @@ void FindController::updateFindUIAfterPageScroll(bool found, const String& strin
             m_findMatches.append(range);
         }
 
-        m_webPage->send(Messages::WebPageProxy::DidFindString(string, matchRects, matchCount, m_foundStringMatchIndex));
+        m_webPage->send(Messages::WebPageProxy::DidFindString(string, matchRects, matchCount, m_foundStringMatchIndex, didWrap == DidWrap::Yes));
 
         if (!(options & FindOptionsShowFindIndicator) || !selectedFrame || !updateFindIndicator(*selectedFrame, shouldShowOverlay))
             hideFindIndicator();
@@ -232,10 +232,11 @@ void FindController::findString(const String& string, FindOptions options, unsig
     m_findMatches.clear();
 
     bool found;
+    DidWrap didWrap = DidWrap::No;
     if (pluginView)
         found = pluginView->findString(string, coreOptions, maxMatchCount);
     else
-        found = m_webPage->corePage()->findString(string, coreOptions);
+        found = m_webPage->corePage()->findString(string, coreOptions, &didWrap);
 
     if (found) {
         didFindString();
@@ -249,8 +250,8 @@ void FindController::findString(const String& string, FindOptions options, unsig
     }
 
     RefPtr<WebPage> protectedWebPage = m_webPage;
-    m_webPage->drawingArea()->dispatchAfterEnsuringUpdatedScrollPosition([protectedWebPage, found, string, options, maxMatchCount] () {
-        protectedWebPage->findController().updateFindUIAfterPageScroll(found, string, options, maxMatchCount);
+    m_webPage->drawingArea()->dispatchAfterEnsuringUpdatedScrollPosition([protectedWebPage, found, string, options, maxMatchCount, didWrap] () {
+        protectedWebPage->findController().updateFindUIAfterPageScroll(found, string, options, maxMatchCount, didWrap);
     });
 }
 
index 7ec0d5a..619e588 100644 (file)
@@ -41,6 +41,8 @@
 namespace WebCore {
 class Frame;
 class Range;
+
+enum class DidWrap : bool;
 }
 
 namespace WebKit {
@@ -81,7 +83,7 @@ private:
     Vector<WebCore::IntRect> rectsForTextMatchesInRect(WebCore::IntRect clipRect);
     bool updateFindIndicator(WebCore::Frame& selectedFrame, bool isShowingOverlay, bool shouldAnimate = true);
 
-    void updateFindUIAfterPageScroll(bool found, const String&, FindOptions, unsigned maxMatchCount);
+    void updateFindUIAfterPageScroll(bool found, const String&, FindOptions, unsigned maxMatchCount, WebCore::DidWrap);
 
     void willFindString();
     void didFindString();
index afa9db3..348d27e 100644 (file)
@@ -1,3 +1,15 @@
+2017-04-04  Tim Horton  <timothy_horton@apple.com>
+
+        [Mac] -[WKWebView findMatchesForString:relativeToMatch:findOptions:maxResults:resultCollector:] invokes the resultCollector with didWrap = NO even when it wraps
+        https://bugs.webkit.org/show_bug.cgi?id=165801
+        <rdar://problem/29649535>
+
+        Reviewed by Wenson Hsieh.
+
+        * TestWebKitAPI/Tests/WebKit2Cocoa/FindInPage.mm:
+        (TEST):
+        Add some tests for wrapping finds.
+
 2017-04-03  Joseph Pecoraro  <pecoraro@apple.com>
 
         Add some new patterns to filter-build-webkit
index 6845bd0..c31bcae 100644 (file)
 
 typedef enum : NSUInteger {
     NSTextFinderAsynchronousDocumentFindOptionsBackwards = 1 << 0,
+    NSTextFinderAsynchronousDocumentFindOptionsWrap = 1 << 1,
 } NSTextFinderAsynchronousDocumentFindOptions;
 
+NSTextFinderAsynchronousDocumentFindOptions noFindOptions = (NSTextFinderAsynchronousDocumentFindOptions)0;
+NSTextFinderAsynchronousDocumentFindOptions backwardsFindOptions =NSTextFinderAsynchronousDocumentFindOptionsBackwards;
+NSTextFinderAsynchronousDocumentFindOptions wrapFindOptions =NSTextFinderAsynchronousDocumentFindOptionsWrap;
+NSTextFinderAsynchronousDocumentFindOptions wrapBackwardsFindOptions = (NSTextFinderAsynchronousDocumentFindOptions)(NSTextFinderAsynchronousDocumentFindOptionsWrap | NSTextFinderAsynchronousDocumentFindOptionsBackwards);
+
 @protocol NSTextFinderAsynchronousDocumentFindMatch <NSObject>
 @property (retain, nonatomic, readonly) NSArray *textRects;
 - (void)generateTextImage:(void (^)(NSImage *generatedImage))completionHandler;
 @end
 
+typedef id <NSTextFinderAsynchronousDocumentFindMatch> FindMatch;
+
 @interface WKWebView (NSTextFinderSupport)
 
-- (void)findMatchesForString:(NSString *)targetString relativeToMatch:(id <NSTextFinderAsynchronousDocumentFindMatch>)relativeMatch findOptions:(NSTextFinderAsynchronousDocumentFindOptions)findOptions maxResults:(NSUInteger)maxResults resultCollector:(void (^)(NSArray *matches, BOOL didWrap))resultCollector;
+- (void)findMatchesForString:(NSString *)targetString relativeToMatch:(FindMatch)relativeMatch findOptions:(NSTextFinderAsynchronousDocumentFindOptions)findOptions maxResults:(NSUInteger)maxResults resultCollector:(void (^)(NSArray *matches, BOOL didWrap))resultCollector;
 
 @end
 
-static bool findMatchesDone;
+typedef struct {
+    RetainPtr<NSArray> matches;
+    BOOL didWrap;
+} FindResult;
+
+static FindResult findMatches(WKWebView *webView, NSString *findString, NSTextFinderAsynchronousDocumentFindOptions findOptions = noFindOptions, NSUInteger maxResults = NSUIntegerMax)
+{
+    __block FindResult result;
+    __block bool done = false;
+
+    [webView findMatchesForString:findString relativeToMatch:nil findOptions:findOptions maxResults:maxResults resultCollector:^(NSArray *matches, BOOL didWrap) {
+        result.matches = matches;
+        result.didWrap = didWrap;
+        done = true;
+    }];
+
+    TestWebKitAPI::Util::run(&done);
+
+    return result;
+}
 
 TEST(WebKit2, FindInPage)
 {
-    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100)]);
+    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 200, 200)]);
     [webView _setOverrideDeviceScaleFactor:2];
 
     NSURLRequest *request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"lots-of-text" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
     [webView loadRequest:request];
     [webView _test_waitForDidFinishNavigation];
 
-    NSTextFinderAsynchronousDocumentFindOptions noFindOptions = (NSTextFinderAsynchronousDocumentFindOptions)0;
-
     // Find all matches.
-    [webView findMatchesForString:@"Birthday" relativeToMatch:nil findOptions:noFindOptions maxResults:NSUIntegerMax resultCollector:^(NSArray *matches, BOOL didWrap) {
-        EXPECT_EQ((NSUInteger)360, matches.count);
+    auto result = findMatches(webView.get(), @"Birthday");
+    EXPECT_EQ((NSUInteger)360, [result.matches count]);
+    RetainPtr<FindMatch> match = [result.matches objectAtIndex:0];
+    EXPECT_EQ((NSUInteger)1, [match textRects].count);
 
-        id <NSTextFinderAsynchronousDocumentFindMatch> firstMatch = [matches objectAtIndex:0];
-        EXPECT_EQ((NSUInteger)1, firstMatch.textRects.count);
-
-        findMatchesDone = true;
+    // Ensure that the generated image has the correct DPI.
+    __block bool generateTextImageDone = false;
+    [match generateTextImage:^(NSImage *image) {
+        CGImageRef CGImage = [image CGImageForProposedRect:nil context:nil hints:nil];
+        EXPECT_EQ(image.size.width, CGImageGetWidth(CGImage) / 2);
+        EXPECT_EQ(image.size.height, CGImageGetHeight(CGImage) / 2);
+        generateTextImageDone = true;
     }];
-
-    TestWebKitAPI::Util::run(&findMatchesDone);
-    findMatchesDone = false;
+    TestWebKitAPI::Util::run(&generateTextImageDone);
 
     // Find one match, doing an incremental search.
-    [webView findMatchesForString:@"Birthday" relativeToMatch:nil findOptions:noFindOptions maxResults:1 resultCollector:^(NSArray *matches, BOOL didWrap) {
-        EXPECT_EQ((NSUInteger)1, matches.count);
-
-        id <NSTextFinderAsynchronousDocumentFindMatch> firstMatch = [matches objectAtIndex:0];
-        EXPECT_EQ((NSUInteger)1, firstMatch.textRects.count);
-
-        // Find the next match in incremental mode.
-        [webView findMatchesForString:@"Birthday" relativeToMatch:nil findOptions:noFindOptions maxResults:1 resultCollector:^(NSArray *matches, BOOL didWrap) {
-            EXPECT_EQ((NSUInteger)1, matches.count);
-
-            id <NSTextFinderAsynchronousDocumentFindMatch> secondMatch = [matches objectAtIndex:0];
-            EXPECT_EQ((NSUInteger)1, secondMatch.textRects.count);
+    result = findMatches(webView.get(), @"Birthday", noFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    RetainPtr<FindMatch> firstMatch = [result.matches firstObject];
+    EXPECT_EQ((NSUInteger)1, [firstMatch textRects].count);
+
+    // Find the next match in incremental mode.
+    result = findMatches(webView.get(), @"Birthday", noFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    RetainPtr<FindMatch> secondMatch = [result.matches firstObject];
+    EXPECT_EQ((NSUInteger)1, [secondMatch textRects].count);
+    EXPECT_FALSE(NSEqualRects([[firstMatch textRects].lastObject rectValue], [[secondMatch textRects].lastObject rectValue]));
+
+    // Find the previous match in incremental mode.
+    result = findMatches(webView.get(), @"Birthday", backwardsFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    RetainPtr<FindMatch> firstMatchAgain = [result.matches firstObject];
+    EXPECT_EQ((NSUInteger)1, [firstMatchAgain textRects].count);
+    EXPECT_TRUE(NSEqualRects([[firstMatch textRects].lastObject rectValue], [[firstMatchAgain textRects].lastObject rectValue]));
 
-            EXPECT_FALSE(NSEqualRects([firstMatch.textRects.lastObject rectValue], [secondMatch.textRects.lastObject rectValue]));
-
-            // Find the previous match in incremental mode.
-            [webView findMatchesForString:@"Birthday" relativeToMatch:nil findOptions:NSTextFinderAsynchronousDocumentFindOptionsBackwards maxResults:1 resultCollector:^(NSArray *matches, BOOL didWrap) {
-                EXPECT_EQ((NSUInteger)1, matches.count);
-
-                id <NSTextFinderAsynchronousDocumentFindMatch> firstMatchAgain = [matches objectAtIndex:0];
-                EXPECT_EQ((NSUInteger)1, firstMatchAgain.textRects.count);
+    // Ensure that we cap the number of matches. There are actually 1600, but we only get the first 1000.
+    result = findMatches(webView.get(), @" ");
+    EXPECT_EQ((NSUInteger)1000, [result.matches count]);
+}
 
-                EXPECT_TRUE(NSEqualRects([firstMatch.textRects.lastObject rectValue], [firstMatchAgain.textRects.lastObject rectValue]));
-                
-                findMatchesDone = true;
-            }];
-        }];
+TEST(WebKit2, FindInPageWrapping)
+{
+    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100)]);
+    [webView _setOverrideDeviceScaleFactor:2];
 
-    }];
+    [webView loadHTMLString:@"word word" baseURL:nil];
+    [webView _test_waitForDidFinishNavigation];
 
-    TestWebKitAPI::Util::run(&findMatchesDone);
-    findMatchesDone = false;
+    // Find one match, doing an incremental search.
+    auto result = findMatches(webView.get(), @"word", wrapFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_FALSE(result.didWrap);
+
+    result = findMatches(webView.get(), @"word", wrapFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_FALSE(result.didWrap);
+
+    // The next match should wrap.
+    result = findMatches(webView.get(), @"word", wrapFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_TRUE(result.didWrap);
+
+    // Going backward after wrapping should wrap again.
+    result = findMatches(webView.get(), @"word", wrapBackwardsFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_TRUE(result.didWrap);
+}
 
-    // Ensure that the generated image has the correct DPI.
-    [webView findMatchesForString:@"Birthday" relativeToMatch:nil findOptions:noFindOptions maxResults:NSUIntegerMax resultCollector:^(NSArray *matches, BOOL didWrap) {
-        EXPECT_EQ((NSUInteger)360, matches.count);
+TEST(WebKit2, FindInPageWrappingDisabled)
+{
+    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100)]);
+    [webView _setOverrideDeviceScaleFactor:2];
 
-        id <NSTextFinderAsynchronousDocumentFindMatch> firstMatch = [matches objectAtIndex:0];
-        [firstMatch generateTextImage:^(NSImage *image) {
-            CGImageRef CGImage = [image CGImageForProposedRect:nil context:nil hints:nil];
-            EXPECT_EQ(image.size.width, CGImageGetWidth(CGImage) / 2);
-            EXPECT_EQ(image.size.height, CGImageGetHeight(CGImage) / 2);
+    [webView loadHTMLString:@"word word" baseURL:nil];
+    [webView _test_waitForDidFinishNavigation];
 
-            findMatchesDone = true;
-        }];
-    }];
+    // Find one match, doing an incremental search.
+    auto result = findMatches(webView.get(), @"word", noFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_FALSE(result.didWrap);
+
+    result = findMatches(webView.get(), @"word", noFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_FALSE(result.didWrap);
+
+    // The next match should fail, because wrapping is disabled.
+    result = findMatches(webView.get(), @"word", noFindOptions, 1);
+    EXPECT_EQ((NSUInteger)0, [result.matches count]);
+    EXPECT_FALSE(result.didWrap);
+}
 
-    TestWebKitAPI::Util::run(&findMatchesDone);
-    findMatchesDone = false;
+TEST(WebKit2, FindInPageWrappingSubframe)
+{
+    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100)]);
+    [webView _setOverrideDeviceScaleFactor:2];
 
-    // Ensure that we cap the number of matches. There are actually 1600, but we only get the first 1000.
-    [webView findMatchesForString:@" " relativeToMatch:nil findOptions:noFindOptions maxResults:NSUIntegerMax resultCollector:^(NSArray *matches, BOOL didWrap) {
-        EXPECT_EQ((NSUInteger)1000, matches.count);
+    [webView loadHTMLString:@"word <iframe srcdoc='word'>" baseURL:nil];
+    [webView _test_waitForDidFinishNavigation];
 
-        findMatchesDone = true;
-    }];
-    TestWebKitAPI::Util::run(&findMatchesDone);
-    findMatchesDone = false;
+    // Find one match, doing an incremental search.
+    auto result = findMatches(webView.get(), @"word", wrapFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_FALSE(result.didWrap);
+
+    result = findMatches(webView.get(), @"word", wrapFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_FALSE(result.didWrap);
+
+    // The next match should wrap.
+    result = findMatches(webView.get(), @"word", wrapFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_TRUE(result.didWrap);
+
+    // Going backward after wrapping should wrap again.
+    result = findMatches(webView.get(), @"word", wrapBackwardsFindOptions, 1);
+    EXPECT_EQ((NSUInteger)1, [result.matches count]);
+    EXPECT_TRUE(result.didWrap);
 }
 
 #endif