TextIndicator::createWithSelectionInFrame does synchronous IPC in WebKit2
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Dec 2014 19:58:14 +0000 (19:58 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Dec 2014 19:58:14 +0000 (19:58 +0000)
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

12 files changed:
Source/WebCore/ChangeLog
Source/WebCore/WebCore.exp.in
Source/WebCore/page/TextIndicator.cpp
Source/WebCore/page/TextIndicator.h
Source/WebCore/page/mac/TextIndicatorWindow.h
Source/WebCore/page/mac/TextIndicatorWindow.mm
Source/WebKit/mac/ChangeLog
Source/WebKit/mac/WebView/WebView.mm
Source/WebKit2/ChangeLog
Source/WebKit2/Shared/WebCoreArgumentCoders.cpp
Source/WebKit2/UIProcess/API/mac/WKView.mm
Source/WebKit2/WebProcess/WebPage/FindController.cpp

index c2d4aab..dfbe5f8 100644 (file)
@@ -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
index 7837212..c0c0284 100644 (file)
@@ -2311,7 +2311,7 @@ __ZN7WebCore17ScrollbarThemeMac23setUpOverhangAreaShadowEP7CALayer
 __ZN7WebCore17ScrollbarThemeMac24removeOverhangAreaShadowEP7CALayer
 __ZN7WebCore17ScrollbarThemeMac27setUpOverhangAreaBackgroundEP7CALayerRKNS_5ColorE
 __ZN7WebCore17ScrollbarThemeMac28removeOverhangAreaBackgroundEP7CALayer
-__ZN7WebCore19TextIndicatorWindow16setTextIndicatorEN3WTF10PassRefPtrINS_13TextIndicatorEEEbNSt3__18functionIFvvEEE
+__ZN7WebCore19TextIndicatorWindow16setTextIndicatorEN3WTF10PassRefPtrINS_13TextIndicatorEEE6CGRectbNSt3__18functionIFvvEEE
 __ZN7WebCore19TextIndicatorWindowC1EP6NSView
 __ZN7WebCore19TextIndicatorWindowD1Ev
 __ZN7WebCore19applicationIsSafariEv
index befa969..ee1de9d 100644 (file)
@@ -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);
index 47ba6c3..be10a33 100644 (file)
@@ -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(); }
index 99d2140..d4d1d22 100644 (file)
@@ -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();
index 1f9be07..ac1d020 100644 (file)
@@ -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);
 
index 5e91d27..7f03ab7 100644 (file)
@@ -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
index 041bc14..1278ccb 100644 (file)
@@ -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
index fed8468..54aed91 100644 (file)
@@ -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
index d2fda12..2b3fb56 100644 (file)
@@ -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))
index fdf2ba8..8f9e656 100644 (file)
@@ -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
index 05e3177..a86b096 100644 (file)
@@ -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();