TextIndicator::createWithSelectionInFrame does synchronous IPC in WebKit2

https://bugs.webkit.org/show_bug.cgi?id=139252
<rdar://problem/19140827>

Reviewed by Anders Carlsson.

It turns out contentsToScreen requires sync IPC in Mac WebKit2, which we
really don't want to be doing here (especially since the UI process will often
be sitting in waitForAndDispatchImmediately waiting for didPerformActionMenuHitTest).

Go back to keeping TextIndicator rects in "window" coordinates and do the conversion
in each of the WebKits instead of trying to share that code.

* WebCore.exp.in:
* page/TextIndicator.cpp:
(WebCore::TextIndicator::createWithSelectionInFrame):
(WebCore::TextIndicator::TextIndicator):
* page/TextIndicator.h:
(WebCore::TextIndicator::selectionRectInWindowCoordinates):
(WebCore::TextIndicator::textBoundingRectInWindowCoordinates):
(WebCore::TextIndicator::selectionRectInScreenCoordinates): Deleted.
(WebCore::TextIndicator::textBoundingRectInScreenCoordinates): Deleted.
Go back to keeping the rects in "window" coordinates.

* page/mac/TextIndicatorWindow.h:
* page/mac/TextIndicatorWindow.mm:
(-[WebTextIndicatorView initWithFrame:textIndicator:margin:]):
(WebCore::TextIndicatorWindow::setTextIndicator):
Let callers pass in the contentRect instead of trying to share the code
to compute it, since it needs to be different for legacy and modern WebKit.

* WebView/WebView.mm:
(-[WebView _setTextIndicator:fadeOut:animationCompletionHandler:]):
Adjust to the WebCore changes.

* Shared/WebCoreArgumentCoders.cpp:
(IPC::ArgumentCoder<TextIndicatorData>::encode):
(IPC::ArgumentCoder<TextIndicatorData>::decode):
* UIProcess/API/mac/WKView.mm:
(-[WKView _setTextIndicator:fadeOut:animationCompletionHandler:]):
* WebProcess/WebPage/FindController.cpp:
(WebKit::FindController::updateFindIndicator):
(WebKit::FindController::drawRect):
Adjust to the WebCore changes.

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index c2d4aab83249e52d385ec280b50cd8f83984791b..dfbe5f83d9fa282925b75f18a5e3fe0735529f03 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,29 @@
+2014-12-04  Tim Horton  <timothy_horton@apple.com>
+
+        TextIndicator::createWithSelectionInFrame does synchronous IPC in WebKit2
+        https://bugs.webkit.org/show_bug.cgi?id=139252
+        <rdar://problem/19140827>
+
+        Reviewed by Anders Carlsson.
+
+        * WebCore.exp.in:
+        * page/TextIndicator.cpp:
+        (WebCore::TextIndicator::createWithSelectionInFrame):
+        (WebCore::TextIndicator::TextIndicator):
+        * page/TextIndicator.h:
+        (WebCore::TextIndicator::selectionRectInWindowCoordinates):
+        (WebCore::TextIndicator::textBoundingRectInWindowCoordinates):
+        (WebCore::TextIndicator::selectionRectInScreenCoordinates): Deleted.
+        (WebCore::TextIndicator::textBoundingRectInScreenCoordinates): Deleted.
+        Go back to keeping the rects in "window" coordinates.
+
+        * page/mac/TextIndicatorWindow.h:
+        * page/mac/TextIndicatorWindow.mm:
+        (-[WebTextIndicatorView initWithFrame:textIndicator:margin:]):
+        (WebCore::TextIndicatorWindow::setTextIndicator):
+        Let callers pass in the contentRect instead of trying to share the code
+        to compute it, since it needs to be different for legacy and modern WebKit.
+
 2014-12-04  Oliver Hunt  <oliver@apple.com>
 
         Serialization of MapData object provides unsafe access to internal types

diff --git a/Source/WebCore/WebCore.exp.in b/Source/WebCore/WebCore.exp.in
index 7837212feb4ed5c252c6eed66cdb2543d21e43f7..c0c02844d841ab1c4273edeba42389875e608248 100644 (file)
--- a/Source/WebCore/WebCore.exp.in
+++ b/Source/WebCore/WebCore.exp.in
@@ -2311,7 +2311,7 @@ __ZN7WebCore17ScrollbarThemeMac23setUpOverhangAreaShadowEP7CALayer
 __ZN7WebCore17ScrollbarThemeMac24removeOverhangAreaShadowEP7CALayer
 __ZN7WebCore17ScrollbarThemeMac27setUpOverhangAreaBackgroundEP7CALayerRKNS_5ColorE
 __ZN7WebCore17ScrollbarThemeMac28removeOverhangAreaBackgroundEP7CALayer
-__ZN7WebCore19TextIndicatorWindow16setTextIndicatorEN3WTF10PassRefPtrINS_13TextIndicatorEEEbNSt3__18functionIFvvEEE
+__ZN7WebCore19TextIndicatorWindow16setTextIndicatorEN3WTF10PassRefPtrINS_13TextIndicatorEEE6CGRectbNSt3__18functionIFvvEEE
 __ZN7WebCore19TextIndicatorWindowC1EP6NSView
 __ZN7WebCore19TextIndicatorWindowD1Ev
 __ZN7WebCore19applicationIsSafariEv

diff --git a/Source/WebCore/page/TextIndicator.cpp b/Source/WebCore/page/TextIndicator.cpp
index befa96908870dd15ef0e828ba4cee6a3d30bfa96..ee1de9d4aa296c20556f987122c5a92889a336d9 100644 (file)
--- a/Source/WebCore/page/TextIndicator.cpp
+++ b/Source/WebCore/page/TextIndicator.cpp
@@ -146,7 +146,7 @@ PassRefPtr<TextIndicator> TextIndicator::createWithSelectionInFrame(Frame& frame
 
     // Store the selection rect in window coordinates, to be used subsequently
     // to determine if the indicator and selection still precisely overlap.
-    IntRect selectionRectInScreenCoordinates = frame.view()->contentsToScreen(selectionRect);
+    IntRect selectionRectInWindowCoordinates = frame.view()->contentsToWindow(selectionRect);
 
     Vector<FloatRect> textRects;
     frame.selection().getClippedVisibleTextRectangles(textRects);
@@ -154,23 +154,23 @@ PassRefPtr<TextIndicator> TextIndicator::createWithSelectionInFrame(Frame& frame
     // The bounding rect of all the text rects can be different than the selection
     // rect when the selection spans multiple lines; the indicator doesn't actually
     // care where the selection highlight goes, just where the text actually is.
-    FloatRect textBoundingRectInScreenCoordinates;
-    Vector<FloatRect> textRectsInScreenCoordinates;
+    FloatRect textBoundingRectInWindowCoordinates;
+    Vector<FloatRect> textRectsInWindowCoordinates;
     for (const FloatRect& textRect : textRects) {
-        FloatRect textRectInScreenCoordinates = frame.view()->contentsToScreen(enclosingIntRect(textRect));
-        textRectsInScreenCoordinates.append(textRectInScreenCoordinates);
-        textBoundingRectInScreenCoordinates.unite(textRectInScreenCoordinates);
+        FloatRect textRectInWindowCoordinates = frame.view()->contentsToWindow(enclosingIntRect(textRect));
+        textRectsInWindowCoordinates.append(textRectInWindowCoordinates);
+        textBoundingRectInWindowCoordinates.unite(textRectInWindowCoordinates);
     }
 
     Vector<FloatRect> textRectsInBoundingRectCoordinates;
-    for (auto rect : textRectsInScreenCoordinates) {
-        rect.moveBy(-textBoundingRectInScreenCoordinates.location());
+    for (auto rect : textRectsInWindowCoordinates) {
+        rect.moveBy(-textBoundingRectInWindowCoordinates.location());
         textRectsInBoundingRectCoordinates.append(rect);
     }
 
     TextIndicatorData data;
-    data.selectionRectInScreenCoordinates = selectionRectInScreenCoordinates;
-    data.textBoundingRectInScreenCoordinates = textBoundingRectInScreenCoordinates;
+    data.selectionRectInWindowCoordinates = selectionRectInWindowCoordinates;
+    data.textBoundingRectInWindowCoordinates = textBoundingRectInWindowCoordinates;
     data.textRectsInBoundingRectCoordinates = textRectsInBoundingRectCoordinates;
     data.contentImageScaleFactor = frame.page()->deviceScaleFactor();
     data.contentImage = indicatorBitmap;
@@ -183,7 +183,7 @@ PassRefPtr<TextIndicator> TextIndicator::createWithSelectionInFrame(Frame& frame
 TextIndicator::TextIndicator(const TextIndicatorData& data)
     : m_data(data)
 {
-    ASSERT(m_data.contentImageScaleFactor != 1 || m_data.contentImage->size() == enclosingIntRect(m_data.selectionRectInScreenCoordinates).size());
+    ASSERT(m_data.contentImageScaleFactor != 1 || m_data.contentImage->size() == enclosingIntRect(m_data.selectionRectInWindowCoordinates).size());
 
     if (textIndicatorsForTextRectsOverlap(m_data.textRectsInBoundingRectCoordinates)) {
         m_data.textRectsInBoundingRectCoordinates[0] = unionRect(m_data.textRectsInBoundingRectCoordinates);

diff --git a/Source/WebCore/page/TextIndicator.h b/Source/WebCore/page/TextIndicator.h
index 47ba6c323798e63964388aa6e916ba7446b81e6c..be10a3333f2fee235713ef708f0a2d81f4c68d24 100644 (file)
--- a/Source/WebCore/page/TextIndicator.h
+++ b/Source/WebCore/page/TextIndicator.h
@@ -51,8 +51,8 @@ enum class TextIndicatorPresentationTransition {
 };
 
 struct TextIndicatorData {
-    FloatRect selectionRectInScreenCoordinates;
-    FloatRect textBoundingRectInScreenCoordinates;
+    FloatRect selectionRectInWindowCoordinates;
+    FloatRect textBoundingRectInWindowCoordinates;
     Vector<FloatRect> textRectsInBoundingRectCoordinates;
     float contentImageScaleFactor;
     RefPtr<Image> contentImageWithHighlight;
@@ -68,8 +68,8 @@ public:
 
     ~TextIndicator();
 
-    FloatRect selectionRectInScreenCoordinates() const { return m_data.selectionRectInScreenCoordinates; }
-    FloatRect textBoundingRectInScreenCoordinates() const { return m_data.textBoundingRectInScreenCoordinates; }
+    FloatRect selectionRectInWindowCoordinates() const { return m_data.selectionRectInWindowCoordinates; }
+    FloatRect textBoundingRectInWindowCoordinates() const { return m_data.textBoundingRectInWindowCoordinates; }
     const Vector<FloatRect>& textRectsInBoundingRectCoordinates() const { return m_data.textRectsInBoundingRectCoordinates; }
     float contentImageScaleFactor() const { return m_data.contentImageScaleFactor; }
     Image *contentImageWithHighlight() const { return m_data.contentImageWithHighlight.get(); }

diff --git a/Source/WebCore/page/mac/TextIndicatorWindow.h b/Source/WebCore/page/mac/TextIndicatorWindow.h
index 99d2140ed7e829401794cc445cc94b627cef4db5..d4d1d22fa4e94722e44e6c135ee14eae11188169 100644 (file)
--- a/Source/WebCore/page/mac/TextIndicatorWindow.h
+++ b/Source/WebCore/page/mac/TextIndicatorWindow.h
@@ -48,7 +48,7 @@ public:
     explicit TextIndicatorWindow(NSView *);
     ~TextIndicatorWindow();
 
-    void setTextIndicator(PassRefPtr<TextIndicator>, bool fadeOut, std::function<void ()> animationCompletionHandler);
+    void setTextIndicator(PassRefPtr<TextIndicator>, NSRect contentRect, bool fadeOut, std::function<void ()> animationCompletionHandler);
 
 private:
     void closeWindow();

diff --git a/Source/WebCore/page/mac/TextIndicatorWindow.mm b/Source/WebCore/page/mac/TextIndicatorWindow.mm
index 1f9be07bc9d68c002879b74decd852ed242a913e..ac1d0207afd0be0b3a29ec8fe0829affc28417ad 100644 (file)
--- a/Source/WebCore/page/mac/TextIndicatorWindow.mm
+++ b/Source/WebCore/page/mac/TextIndicatorWindow.mm
@@ -171,7 +171,7 @@ using namespace WebCore;
         [textLayer setContents:(id)contentsImage.get()];
 
         FloatRect imageRect = textRect;
-        imageRect.move(_textIndicator->textBoundingRectInScreenCoordinates().location() - _textIndicator->selectionRectInScreenCoordinates().location());
+        imageRect.move(_textIndicator->textBoundingRectInWindowCoordinates().location() - _textIndicator->selectionRectInWindowCoordinates().location());
         [textLayer setContentsRect:CGRectMake(imageRect.x() / contentsImageLogicalSize.width(), imageRect.y() / contentsImageLogicalSize.height(), imageRect.width() / contentsImageLogicalSize.width(), imageRect.height() / contentsImageLogicalSize.height())];
         [textLayer setContentsGravity:kCAGravityCenter];
         [textLayer setContentsScale:_textIndicator->contentImageScaleFactor()];
@@ -266,7 +266,7 @@ TextIndicatorWindow::~TextIndicatorWindow()
     closeWindow();
 }
 
-void TextIndicatorWindow::setTextIndicator(PassRefPtr<TextIndicator> textIndicator, bool fadeOut, std::function<void ()> animationCompletionHandler)
+void TextIndicatorWindow::setTextIndicator(PassRefPtr<TextIndicator> textIndicator, NSRect contentRect, bool fadeOut, std::function<void ()> animationCompletionHandler)
 {
     if (m_textIndicator == textIndicator)
         return;
@@ -279,7 +279,6 @@ void TextIndicatorWindow::setTextIndicator(PassRefPtr<TextIndicator> textIndicat
     if (!m_textIndicator)
         return;
 
-    NSRect contentRect = m_textIndicator->textBoundingRectInScreenCoordinates();
     CGFloat horizontalMargin = std::max(dropShadowBlurRadius * 2 + horizontalBorder, contentRect.size.width * 2);
     CGFloat verticalMargin = std::max(dropShadowBlurRadius * 2 + verticalBorder, contentRect.size.height * 2);
 

diff --git a/Source/WebKit/mac/ChangeLog b/Source/WebKit/mac/ChangeLog
index 5e91d27b1d4c56477b7f9322b9b0956ce0c0a49a..7f03ab77bf761378439bfb132c496b1b5a989d1f 100644 (file)
--- a/Source/WebKit/mac/ChangeLog
+++ b/Source/WebKit/mac/ChangeLog
@@ -1,3 +1,15 @@
+2014-12-04  Tim Horton  <timothy_horton@apple.com>
+
+        TextIndicator::createWithSelectionInFrame does synchronous IPC in WebKit2
+        https://bugs.webkit.org/show_bug.cgi?id=139252
+        <rdar://problem/19140827>
+
+        Reviewed by Anders Carlsson.
+
+        * WebView/WebView.mm:
+        (-[WebView _setTextIndicator:fadeOut:animationCompletionHandler:]):
+        Adjust to the WebCore changes.
+
 2014-12-03  Timothy Horton  <timothy_horton@apple.com>
 
         Implement action menus for tel: URLs

diff --git a/Source/WebKit/mac/WebView/WebView.mm b/Source/WebKit/mac/WebView/WebView.mm
index 041bc146d1628729866778d6e374fbe1f83a761d..1278ccb47b6dbf841380d4d8679f8e6741964b8c 100644 (file)
--- a/Source/WebKit/mac/WebView/WebView.mm
+++ b/Source/WebKit/mac/WebView/WebView.mm
@@ -8601,7 +8601,8 @@ static void glibContextIterationCallback(CFRunLoopObserverRef, CFRunLoopActivity
     if (!_private->textIndicatorWindow)
         _private->textIndicatorWindow = std::make_unique<TextIndicatorWindow>(self);
 
-    _private->textIndicatorWindow->setTextIndicator(textIndicator, fadeOut, WTF::move(completionHandler));
+    NSRect contentRect = [self.window convertRectToScreen:textIndicator->textBoundingRectInWindowCoordinates()];
+    _private->textIndicatorWindow->setTextIndicator(textIndicator, contentRect, fadeOut, WTF::move(completionHandler));
 }
 
 - (void)_clearTextIndicator

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
index fed84688a7f0fafee3d14393e3488d899ed08ea4..54aed910a8a2fa5fb1df0e8ab1eed024aea85001 100644 (file)
--- a/Source/WebKit2/ChangeLog
+++ b/Source/WebKit2/ChangeLog
@@ -1,3 +1,28 @@
+2014-12-04  Tim Horton  <timothy_horton@apple.com>
+
+        TextIndicator::createWithSelectionInFrame does synchronous IPC in WebKit2
+        https://bugs.webkit.org/show_bug.cgi?id=139252
+        <rdar://problem/19140827>
+
+        Reviewed by Anders Carlsson.
+
+        It turns out contentsToScreen requires sync IPC in Mac WebKit2, which we
+        really don't want to be doing here (especially since the UI process will often
+        be sitting in waitForAndDispatchImmediately waiting for didPerformActionMenuHitTest).
+
+        Go back to keeping TextIndicator rects in "window" coordinates and do the conversion
+        in each of the WebKits instead of trying to share that code.
+
+        * Shared/WebCoreArgumentCoders.cpp:
+        (IPC::ArgumentCoder<TextIndicatorData>::encode):
+        (IPC::ArgumentCoder<TextIndicatorData>::decode):
+        * UIProcess/API/mac/WKView.mm:
+        (-[WKView _setTextIndicator:fadeOut:animationCompletionHandler:]):
+        * WebProcess/WebPage/FindController.cpp:
+        (WebKit::FindController::updateFindIndicator):
+        (WebKit::FindController::drawRect):
+        Adjust to the WebCore changes.
+
 2014-12-04  Anders Carlsson  <andersca@apple.com>
 
         Simplify StorageManager callback functions

diff --git a/Source/WebKit2/Shared/WebCoreArgumentCoders.cpp b/Source/WebKit2/Shared/WebCoreArgumentCoders.cpp
index d2fda12d23e6b7809c478e44bcb06b04c44745f2..2b3fb563903ead750fae810d9852300396043ce5 100644 (file)
--- a/Source/WebKit2/Shared/WebCoreArgumentCoders.cpp
+++ b/Source/WebKit2/Shared/WebCoreArgumentCoders.cpp
@@ -1963,8 +1963,8 @@ bool ArgumentCoder<BlobPart>::decode(ArgumentDecoder& decoder, BlobPart& blobPar
 
 void ArgumentCoder<TextIndicatorData>::encode(ArgumentEncoder& encoder, const TextIndicatorData& textIndicatorData)
 {
-    encoder << textIndicatorData.selectionRectInScreenCoordinates;
-    encoder << textIndicatorData.textBoundingRectInScreenCoordinates;
+    encoder << textIndicatorData.selectionRectInWindowCoordinates;
+    encoder << textIndicatorData.textBoundingRectInWindowCoordinates;
     encoder << textIndicatorData.textRectsInBoundingRectCoordinates;
     encoder << textIndicatorData.contentImageScaleFactor;
     encoder.encodeEnum(textIndicatorData.presentationTransition);
@@ -1982,10 +1982,10 @@ void ArgumentCoder<TextIndicatorData>::encode(ArgumentEncoder& encoder, const Te
 
 bool ArgumentCoder<TextIndicatorData>::decode(ArgumentDecoder& decoder, TextIndicatorData& textIndicatorData)
 {
-    if (!decoder.decode(textIndicatorData.selectionRectInScreenCoordinates))
+    if (!decoder.decode(textIndicatorData.selectionRectInWindowCoordinates))
         return false;
 
-    if (!decoder.decode(textIndicatorData.textBoundingRectInScreenCoordinates))
+    if (!decoder.decode(textIndicatorData.textBoundingRectInWindowCoordinates))
         return false;
 
     if (!decoder.decode(textIndicatorData.textRectsInBoundingRectCoordinates))

diff --git a/Source/WebKit2/UIProcess/API/mac/WKView.mm b/Source/WebKit2/UIProcess/API/mac/WKView.mm
index fdf2ba80052b801904dbcd603d04c8537b4951e3..8f9e656395d7e445cfc7c71af0fcfffa4c0e6a56 100644 (file)
--- a/Source/WebKit2/UIProcess/API/mac/WKView.mm
+++ b/Source/WebKit2/UIProcess/API/mac/WKView.mm
@@ -3081,7 +3081,8 @@ static void* keyValueObservingContext = &keyValueObservingContext;
     if (!_data->_textIndicatorWindow)
         _data->_textIndicatorWindow = std::make_unique<TextIndicatorWindow>(self);
 
-    _data->_textIndicatorWindow->setTextIndicator(textIndicator, fadeOut, WTF::move(completionHandler));
+    NSRect contentRect = [self.window convertRectToScreen:[self convertRect:textIndicator->textBoundingRectInWindowCoordinates() toView:nil]];
+    _data->_textIndicatorWindow->setTextIndicator(textIndicator, contentRect, fadeOut, WTF::move(completionHandler));
 }
 
 - (void)_setTextIndicator:(PassRefPtr<TextIndicator>)textIndicator fadeOut:(BOOL)fadeOut

diff --git a/Source/WebKit2/WebProcess/WebPage/FindController.cpp b/Source/WebKit2/WebProcess/WebPage/FindController.cpp
index 05e31776cfb3448f32f7b682a17487250d7dc218..a86b09611a8faf91f14f7376cf02f40f4aa25baa 100644 (file)
--- a/Source/WebKit2/WebProcess/WebPage/FindController.cpp
+++ b/Source/WebKit2/WebProcess/WebPage/FindController.cpp
@@ -318,7 +318,7 @@ bool FindController::updateFindIndicator(Frame& selectedFrame, bool isShowingOve
     if (!indicator)
         return false;
 
-    m_findIndicatorRect = enclosingIntRect(indicator->selectionRectInScreenCoordinates());
+    m_findIndicatorRect = enclosingIntRect(indicator->selectionRectInWindowCoordinates());
     m_webPage->send(Messages::WebPageProxy::SetTextIndicator(indicator->data(), !isShowingOverlay));
     m_isShowingFindIndicator = true;
 
@@ -450,7 +450,7 @@ void FindController::drawRect(PageOverlay&, GraphicsContext& graphicsContext, co
         return;
 
     if (Frame* selectedFrame = frameWithSelection(m_webPage->corePage())) {
-        IntRect findIndicatorRect = selectedFrame->view()->contentsToScreen(enclosingIntRect(selectedFrame->selection().selectionBounds()));
+        IntRect findIndicatorRect = selectedFrame->view()->contentsToWindow(enclosingIntRect(selectedFrame->selection().selectionBounds()));
 
         if (findIndicatorRect != m_findIndicatorRect)
             hideFindIndicator();