Changeset 171560 in webkit

Timestamp:

Jul 24, 2014 5:24:56 PM (10 years ago)

Author:

Simon Fraser

Message:

[iOS WK2] Header bar on nytimes articles lands in the wrong place after rubberbanding
​https://bugs.webkit.org/show_bug.cgi?id=135221
<rdar://problem/17542454>

Reviewed by Benjamin Poulain.

The call to didCommitLayerTree() can cause one or two visible rect updates,
via changes to the UIScrollView contentSize and contentOffset. As a result, we
would notify the scrolling tree about a viewport change, but using the old
scrolling tree rather than the new one, so we could move layers around for
nodes which are about to be removed from the tree.

However, we also have to ensure that programmatic scrolls are applied after
didCommitLayerTree() has updated the view size, so have RemoteScrollingCoordinatorProxy
store data about programmatic scrolls and return them to the caller, which
can apply them after didCommitLayerTree().

• UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp: Store a pointer to a RequestedScrollInfo

for the duration of the tree update, so that we can store requested scroll info in it.
(WebKit::RemoteScrollingCoordinatorProxy::RemoteScrollingCoordinatorProxy):
(WebKit::RemoteScrollingCoordinatorProxy::updateScrollingTree):
(WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeRequestsScroll):

• UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h:
• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::didCommitLayerTree): Give Mac a stub implementation.

• UIProcess/WebPageProxy.h: Group some editing-related functions together.

(WebKit::WebPageProxy::editorState):
(WebKit::WebPageProxy::canDelete):
(WebKit::WebPageProxy::hasSelectedRange):
(WebKit::WebPageProxy::isContentEditable):
(WebKit::WebPageProxy::maintainsInactiveSelection):

• UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:

(WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree): Ordering change: update
the layer tree, then call didCommitLayerTree(), then do the viewport update, followed
by any programmatic scroll.

Location:

trunk/Source/WebKit2

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp (modified) (4 diffs)
• UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h (modified) (2 diffs)
• UIProcess/WebPageProxy.cpp (modified) (1 diff)
• UIProcess/WebPageProxy.h (modified) (3 diffs)
• UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm (modified) (1 diff)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2014-07-24  Simon Fraser  <simon.fraser@apple.com>
+
+        [iOS WK2] Header bar on nytimes articles lands in the wrong place after rubberbanding
+        https://bugs.webkit.org/show_bug.cgi?id=135221
+        <rdar://problem/17542454>
+
+        Reviewed by Benjamin Poulain.
+        
+        The call to didCommitLayerTree() can cause one or two visible rect updates,
+        via changes to the UIScrollView contentSize and contentOffset. As a result, we
+        would notify the scrolling tree about a viewport change, but using the old
+        scrolling tree rather than the new one, so we could move layers around for
+        nodes which are about to be removed from the tree.
+        
+        However, we also have to ensure that programmatic scrolls are applied after
+        didCommitLayerTree() has updated the view size, so have RemoteScrollingCoordinatorProxy
+        store data about programmatic scrolls and return them to the caller, which
+        can apply them after didCommitLayerTree().
+
+        * UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp: Store a pointer to a RequestedScrollInfo
+        for the duration of the tree update, so that we can store requested scroll info in it.
+        (WebKit::RemoteScrollingCoordinatorProxy::RemoteScrollingCoordinatorProxy):
+        (WebKit::RemoteScrollingCoordinatorProxy::updateScrollingTree):
+        (WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeNodeRequestsScroll):
+        * UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::didCommitLayerTree): Give Mac a stub implementation.
+        * UIProcess/WebPageProxy.h: Group some editing-related functions together.
+        (WebKit::WebPageProxy::editorState):
+        (WebKit::WebPageProxy::canDelete):
+        (WebKit::WebPageProxy::hasSelectedRange):
+        (WebKit::WebPageProxy::isContentEditable):
+        (WebKit::WebPageProxy::maintainsInactiveSelection):
+        * UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:
+        (WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree): Ordering change: update
+        the layer tree, then call didCommitLayerTree(), then do the viewport update, followed
+        by any programmatic scroll.
+
 2014-07-24  Peyton Randolph  <prandolph@apple.com>
 

trunk/Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp

@@ -50 +50 @@
     : m_webPageProxy(webPageProxy)
     , m_scrollingTree(RemoteScrollingTree::create(*this))
+    , m_requestedScrollInfo(nullptr)
     , m_propagatesMainFrameScrolls(true)
 {
@@ -78 +79 @@
 }
 
-void RemoteScrollingCoordinatorProxy::updateScrollingTree(const RemoteScrollingCoordinatorTransaction& transaction)
+void RemoteScrollingCoordinatorProxy::updateScrollingTree(const RemoteScrollingCoordinatorTransaction& transaction, RequestedScrollInfo& requestedScrollInfo)
 {
+    m_requestedScrollInfo = &requestedScrollInfo;
+
     OwnPtr<ScrollingStateTree> stateTree = const_cast<RemoteScrollingCoordinatorTransaction&>(transaction).scrollingStateTree().release();
     
@@ -90 +93 @@
     connectStateNodeLayers(*stateTree, *layerTreeHost);
     m_scrollingTree->commitNewTreeState(stateTree.release());
+
+    m_requestedScrollInfo = nullptr;
 }
 
@@ -170 +175 @@
 void RemoteScrollingCoordinatorProxy::scrollingTreeNodeRequestsScroll(ScrollingNodeID scrolledNodeID, const FloatPoint& scrollPosition, bool representsProgrammaticScroll)
 {
-    if (scrolledNodeID == rootScrollingNodeID())
-        m_webPageProxy.requestScroll(scrollPosition, representsProgrammaticScroll);
+    if (scrolledNodeID == rootScrollingNodeID() && m_requestedScrollInfo) {
+        m_requestedScrollInfo->requestsScrollPositionUpdate = true;
+        m_requestedScrollInfo->requestIsProgrammaticScroll = representsProgrammaticScroll;
+        m_requestedScrollInfo->requestedScrollPosition = scrollPosition;
+    }
 }
 

trunk/Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.h

@@ -69 +69 @@
     const RemoteLayerTreeHost* layerTreeHost() const;
 
-    void updateScrollingTree(const RemoteScrollingCoordinatorTransaction&);
+    struct RequestedScrollInfo {
+        bool requestsScrollPositionUpdate { };
+        bool requestIsProgrammaticScroll { };
+        WebCore::FloatPoint requestedScrollPosition;
+    };
+    void updateScrollingTree(const RemoteScrollingCoordinatorTransaction&, RequestedScrollInfo&);
 
     void setPropagatesMainFrameScrolls(bool propagatesMainFrameScrolls) { m_propagatesMainFrameScrolls = propagatesMainFrameScrolls; }
@@ -87 +92 @@
     WebPageProxy& m_webPageProxy;
     RefPtr<RemoteScrollingTree> m_scrollingTree;
+    RequestedScrollInfo* m_requestedScrollInfo;
     bool m_propagatesMainFrameScrolls;
 };

trunk/Source/WebKit2/UIProcess/WebPageProxy.cpp

@@ -1314 +1314 @@
     m_process->send(Messages::WebPage::ExecuteEditCommand(commandName), m_pageID);
 }
-    
+
+#if !PLATFORM(IOS)
+void WebPageProxy::didCommitLayerTree(const RemoteLayerTreeTransaction&)
+{
+}
+#endif
+
 #if USE(TILED_BACKING_STORE)
 void WebPageProxy::commitPageTransitionViewport()

trunk/Source/WebKit2/UIProcess/WebPageProxy.h

@@ -378 +378 @@
     void executeEditCommand(const String& commandName);
     void validateCommand(const String& commandName, std::function<void (const String&, bool, int32_t, CallbackBase::Error)>);
+
+    const EditorState& editorState() const { return m_editorState; }
+    bool canDelete() const { return hasSelectedRange() && isContentEditable(); }
+    bool hasSelectedRange() const { return m_editorState.selectionIsRange; }
+    bool isContentEditable() const { return m_editorState.isContentEditable; }
+    
+    bool maintainsInactiveSelection() const { return m_maintainsInactiveSelection; }
+    void setMaintainsInactiveSelection(bool);
+
 #if PLATFORM(IOS)
     void executeEditCommand(const String& commandName, std::function<void (CallbackBase::Error)>);
@@ -403 +412 @@
     int32_t deviceOrientation() const { return m_deviceOrientation; }
     void willCommitLayerTree(uint64_t transactionID);
-    void didCommitLayerTree(const WebKit::RemoteLayerTreeTransaction&);
 
     void selectWithGesture(const WebCore::IntPoint, WebCore::TextGranularity, uint32_t gestureType, uint32_t gestureState, std::function<void (const WebCore::IntPoint&, uint32_t, uint32_t, uint32_t, CallbackBase::Error)>);
@@ -443 +451 @@
 #endif
 
-    const EditorState& editorState() const { return m_editorState; }
-    bool canDelete() const { return hasSelectedRange() && isContentEditable(); }
-    bool hasSelectedRange() const { return m_editorState.selectionIsRange; }
-    bool isContentEditable() const { return m_editorState.isContentEditable; }
-    
-    bool maintainsInactiveSelection() const { return m_maintainsInactiveSelection; }
-    void setMaintainsInactiveSelection(bool);
+    void didCommitLayerTree(const WebKit::RemoteLayerTreeTransaction&);
+
 #if USE(TILED_BACKING_STORE) 
     void didRenderFrame(const WebCore::IntSize& contentsSize, const WebCore::IntRect& coveredRect);

trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm

@@ -201 +201 @@
         m_webPageProxy->setAcceleratedCompositingRootLayer(m_remoteLayerTreeHost.rootLayer());
 
-#if PLATFORM(IOS)
+#if ENABLE(ASYNC_SCROLLING)
+    RemoteScrollingCoordinatorProxy::RequestedScrollInfo requestedScrollInfo;
+    m_webPageProxy->scrollingCoordinatorProxy()->updateScrollingTree(scrollingTreeTransaction, requestedScrollInfo);
+#endif
+
     m_webPageProxy->didCommitLayerTree(layerTreeTransaction);
-#endif
 
 #if ENABLE(ASYNC_SCROLLING)
-    m_webPageProxy->scrollingCoordinatorProxy()->updateScrollingTree(scrollingTreeTransaction);
-
 #if PLATFORM(IOS)
     if (m_webPageProxy->scrollingCoordinatorProxy()->hasFixedOrSticky()) {
+        // If we got a new layer for a fixed or sticky node, its position from the WebProcess is probably stale. We need to re-run the "viewport" changed logic to udpate it with our UI-side state.
         FloatRect customFixedPositionRect = m_webPageProxy->computeCustomFixedPositionRect(m_webPageProxy->unobscuredContentRect(), m_webPageProxy->displayedContentScale());
-        // If we got a new layer for a fixed or sticky node, its position from the WebProcess is probably stale. We need to re-run the "viewport" changed logic to udpate it with our UI-side state.
         m_webPageProxy->scrollingCoordinatorProxy()->viewportChangedViaDelegatedScrolling(m_webPageProxy->scrollingCoordinatorProxy()->rootScrollingNodeID(), customFixedPositionRect, m_webPageProxy->displayedContentScale());
     }
 #endif
+
+    // Handle requested scroll position updates from the scrolling tree transaction after didCommitLayerTree()
+    // has updated the view size based on the content size.
+    if (requestedScrollInfo.requestsScrollPositionUpdate)
+        m_webPageProxy->requestScroll(requestedScrollInfo.requestedScrollPosition, requestedScrollInfo.requestIsProgrammaticScroll);
 #endif // ENABLE(ASYNC_SCROLLING)