Take navigation snapshots whenever the current back-forward item is g…

…oing to change

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

Reviewed by Dan Bernstein.

Instead of trying to have the UI process figure out when to take navigation snapshots by itself,
snapshot whenever the Web process says that the current back-forward item is going to change.
This fixes snapshotting timing with pushState, and lets us bottleneck snapshotting down to
just two places instead of 5.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::goForward):
(WebKit::WebPageProxy::goBack):
(WebKit::WebPageProxy::goToBackForwardItem):
(WebKit::WebPageProxy::didStartProvisionalLoadForFrame):
We no longer need to special-case taking navigation snapshots
when the UI process changes the back forward item or upon
didStartProvisionalLoadForFrame, because we'll always snapshot
in willChangeCurrentHistoryItem in all of these cases.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::willChangeCurrentHistoryItem):
* UIProcess/WebPageProxy.h:
* UIProcess/WebPageProxy.messages.in:
Add willChangeCurrentHistoryItem message, which comes from the Web process.
When it arrives, take a navigation snapshot.

* UIProcess/ios/ViewGestureControllerIOS.mm:
(WebKit::ViewGestureController::beginSwipeGesture):
Take the pre-swipe navigation snapshot before telling WebPageProxy that we're doing a swipe,
so that it doesn't bail from taking the snapshot because we have a snapshot up.

(WebKit::ViewGestureController::endSwipeGesture):
We no longer need to explicitly disable snapshotting while navigating, because
we will avoid taking the snapshot if there's a snapshot being displayed.

* UIProcess/mac/ViewGestureControllerMac.mm:
(WebKit::ViewGestureController::~ViewGestureController):
Remove the snapshot if it's still up when ViewGestureController is destroyed.
The Mac version of ViewGestureController is destroyed on Web process crashes
because it is a message receiver, so it is not guaranteed to have the same
lifetime as the WebPageProxy and friends.

(WebKit::ViewGestureController::trackSwipeGesture):
Make use of recordNavigationSnapshot.

(WebKit::ViewGestureController::endSwipeGesture):
Ditto from the Mac version.

* UIProcess/mac/ViewSnapshotStore.h:
(WebKit::ViewSnapshotStore::disableSnapshotting): Deleted.
(WebKit::ViewSnapshotStore::enableSnapshotting): Deleted.
* UIProcess/mac/ViewSnapshotStore.mm:
(WebKit::ViewSnapshotStore::ViewSnapshotStore):
(WebKit::ViewSnapshotStore::recordSnapshot):
Remove the snapshot disabling mechanism and bail from snapshotting if we're
showing a snapshot, as mentioned above.

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::willChangeCurrentHistoryItem):
* WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::willChangeCurrentHistoryItem):
* WebProcess/WebPage/WebPage.h:
Proxy willChangeCurrentHistoryItem from HistoryController to the UI process.

* loader/HistoryController.cpp:
(WebCore::HistoryController::updateForCommit):
(WebCore::HistoryController::recursiveUpdateForCommit):
(WebCore::HistoryController::recursiveUpdateForSameDocumentNavigation):
(WebCore::HistoryController::createItem):
Use setCurrentItem instead of duplicating the contents of it inside each of these functions.

(WebCore::HistoryController::setCurrentItem):
(WebCore::HistoryController::replaceCurrentItem):
When setting or replacing the current item, let the FrameLoaderClient know that we're going
to change which history item is "current".

* loader/FrameLoaderClient.h:
(WebCore::FrameLoaderClient::willChangeCurrentHistoryItem): Added.



Canonical link: https://commits.webkit.org/152972@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@171239 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,26 @@
+2014-07-18  Tim Horton  <timothy_horton@apple.com>
+
+        Take navigation snapshots whenever the current back-forward item is going to change
+        https://bugs.webkit.org/show_bug.cgi?id=135058
+        <rdar://problem/17464515>
+
+        Reviewed by Dan Bernstein.
+
+        * loader/HistoryController.cpp:
+        (WebCore::HistoryController::updateForCommit):
+        (WebCore::HistoryController::recursiveUpdateForCommit):
+        (WebCore::HistoryController::recursiveUpdateForSameDocumentNavigation):
+        (WebCore::HistoryController::createItem):
+        Use setCurrentItem instead of duplicating the contents of it inside each of these functions.
+
+        (WebCore::HistoryController::setCurrentItem):
+        (WebCore::HistoryController::replaceCurrentItem):
+        When setting or replacing the current item, let the FrameLoaderClient know that we're going
+        to change which history item is "current".
+
+        * loader/FrameLoaderClient.h:
+        (WebCore::FrameLoaderClient::willChangeCurrentHistoryItem): Added.
+
 2014-07-18  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r171207.

Source/WebCore/loader/FrameLoaderClient.h

@@ -204,6 +204,8 @@ namespace WebCore {
         virtual bool shouldGoToHistoryItem(HistoryItem*) const = 0;
         virtual void updateGlobalHistoryItemForPage() { }
 
+        virtual void willChangeCurrentHistoryItem() { }
+
         // This frame has set its opener to null, disowning it for the lifetime of the frame.
         // See http://html.spec.whatwg.org/#dom-opener.
         // FIXME: JSC should allow disowning opener. - <https://bugs.webkit.org/show_bug.cgi?id=103913>.

Source/WebCore/loader/HistoryController.cpp

@@ -465,10 +465,8 @@ void HistoryController::updateForCommit()
         // the provisional item for restoring state.
         // Note previousItem must be set before we close the URL, which will
         // happen when the data source is made non-provisional below
-        m_frameLoadComplete = false;
-        m_previousItem = m_currentItem;
         ASSERT(m_provisionalItem);
-        m_currentItem = m_provisionalItem;
+        setCurrentItem(m_provisionalItem.get());
         m_provisionalItem = 0;
 
         // Tell all other frames in the tree to commit their provisional items and
@@ -509,9 +507,7 @@ void HistoryController::recursiveUpdateForCommit()
             view->setWasScrolledByUser(false);
 
         // Now commit the provisional item
-        m_frameLoadComplete = false;
-        m_previousItem = m_currentItem;
-        m_currentItem = m_provisionalItem;
+        setCurrentItem(m_provisionalItem.get());
         m_provisionalItem = 0;
 
         // Restore form state (works from currentItem)
@@ -560,9 +556,7 @@ void HistoryController::recursiveUpdateForSameDocumentNavigation()
         return;
 
     // Commit the provisional item.
-    m_frameLoadComplete = false;
-    m_previousItem = m_currentItem;
-    m_currentItem = m_provisionalItem;
+    setCurrentItem(m_provisionalItem.get());
     m_provisionalItem = 0;
 
     // Iterate over the rest of the tree.
@@ -580,6 +574,8 @@ void HistoryController::updateForFrameLoadCompleted()
 
 void HistoryController::setCurrentItem(HistoryItem* item)
 {
+    m_frame.loader().client().willChangeCurrentHistoryItem();
+
     m_frameLoadComplete = false;
     m_previousItem = m_currentItem;
     m_currentItem = item;
@@ -658,9 +654,7 @@ PassRefPtr<HistoryItem> HistoryController::createItem()
     initializeItem(item.get());
 
     // Set the item for which we will save document state
-    m_frameLoadComplete = false;
-    m_previousItem = m_currentItem;
-    m_currentItem = item;
+    setCurrentItem(item.get());
 
     return item.release();
 }
@@ -888,8 +882,10 @@ void HistoryController::replaceCurrentItem(HistoryItem* item)
     m_previousItem = nullptr;
     if (m_provisionalItem)
         m_provisionalItem = item;
-    else
+    else {
+        m_frame.loader().client().willChangeCurrentHistoryItem();
         m_currentItem = item;
+    }
 }
 
 } // namespace WebCore

Source/WebKit2/ChangeLog

@@ -1,3 +1,72 @@
+2014-07-18  Tim Horton  <timothy_horton@apple.com>
+
+        Take navigation snapshots whenever the current back-forward item is going to change
+        https://bugs.webkit.org/show_bug.cgi?id=135058
+        <rdar://problem/17464515>
+
+        Reviewed by Dan Bernstein.
+
+        Instead of trying to have the UI process figure out when to take navigation snapshots by itself,
+        snapshot whenever the Web process says that the current back-forward item is going to change.
+        This fixes snapshotting timing with pushState, and lets us bottleneck snapshotting down to
+        just two places instead of 5.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::goForward):
+        (WebKit::WebPageProxy::goBack):
+        (WebKit::WebPageProxy::goToBackForwardItem):
+        (WebKit::WebPageProxy::didStartProvisionalLoadForFrame):
+        We no longer need to special-case taking navigation snapshots
+        when the UI process changes the back forward item or upon
+        didStartProvisionalLoadForFrame, because we'll always snapshot
+        in willChangeCurrentHistoryItem in all of these cases.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::willChangeCurrentHistoryItem):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebPageProxy.messages.in:
+        Add willChangeCurrentHistoryItem message, which comes from the Web process.
+        When it arrives, take a navigation snapshot.
+
+        * UIProcess/ios/ViewGestureControllerIOS.mm:
+        (WebKit::ViewGestureController::beginSwipeGesture):
+        Take the pre-swipe navigation snapshot before telling WebPageProxy that we're doing a swipe,
+        so that it doesn't bail from taking the snapshot because we have a snapshot up.
+
+        (WebKit::ViewGestureController::endSwipeGesture):
+        We no longer need to explicitly disable snapshotting while navigating, because
+        we will avoid taking the snapshot if there's a snapshot being displayed.
+
+        * UIProcess/mac/ViewGestureControllerMac.mm:
+        (WebKit::ViewGestureController::~ViewGestureController):
+        Remove the snapshot if it's still up when ViewGestureController is destroyed.
+        The Mac version of ViewGestureController is destroyed on Web process crashes
+        because it is a message receiver, so it is not guaranteed to have the same
+        lifetime as the WebPageProxy and friends.
+
+        (WebKit::ViewGestureController::trackSwipeGesture):
+        Make use of recordNavigationSnapshot.
+
+        (WebKit::ViewGestureController::endSwipeGesture):
+        Ditto from the Mac version.
+
+        * UIProcess/mac/ViewSnapshotStore.h:
+        (WebKit::ViewSnapshotStore::disableSnapshotting): Deleted.
+        (WebKit::ViewSnapshotStore::enableSnapshotting): Deleted.
+        * UIProcess/mac/ViewSnapshotStore.mm:
+        (WebKit::ViewSnapshotStore::ViewSnapshotStore):
+        (WebKit::ViewSnapshotStore::recordSnapshot):
+        Remove the snapshot disabling mechanism and bail from snapshotting if we're
+        showing a snapshot, as mentioned above.
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::willChangeCurrentHistoryItem):
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::willChangeCurrentHistoryItem):
+        * WebProcess/WebPage/WebPage.h:
+        Proxy willChangeCurrentHistoryItem from HistoryController to the UI process.
+
 2014-07-18  Jon Honeycutt  <jhoneycutt@apple.com>
 
         REGRESSION: Crash when typing into text field that clears itself on iOS

Source/WebKit2/UIProcess/WebPageProxy.cpp

@@ -893,8 +893,6 @@ uint64_t WebPageProxy::goForward()
     if (!forwardItem)
         return 0;
 
-    recordNavigationSnapshot();
-
     auto transaction = m_pageLoadState.transaction();
 
     m_pageLoadState.setPendingAPIRequestURL(transaction, forwardItem->url());
@@ -916,8 +914,6 @@ uint64_t WebPageProxy::goBack()
     if (!backItem)
         return 0;
 
-    recordNavigationSnapshot();
-
     auto transaction = m_pageLoadState.transaction();
 
     m_pageLoadState.setPendingAPIRequestURL(transaction, backItem->url());
@@ -938,8 +934,6 @@ uint64_t WebPageProxy::goToBackForwardItem(WebBackForwardListItem* item)
     if (!isValid())
         return reattachToWebProcessWithItem(item);
 
-    recordNavigationSnapshot();
-
     auto transaction = m_pageLoadState.transaction();
 
     m_pageLoadState.setPendingAPIRequestURL(transaction, item->url());
@@ -2508,10 +2502,8 @@ void WebPageProxy::didStartProvisionalLoadForFrame(uint64_t frameID, uint64_t na
     MESSAGE_CHECK(frame);
     MESSAGE_CHECK_URL(url);
 
-    if (frame->isMainFrame()) {
-        recordNavigationSnapshot();
+    if (frame->isMainFrame())
         m_pageLoadState.didStartProvisionalLoad(transaction, url, unreachableURL);
-    }
 
     frame->setUnreachableURL(unreachableURL);
     frame->didStartProvisionalLoad(url);
@@ -5160,4 +5152,9 @@ void WebPageProxy::navigationGestureSnapshotWasRemoved()
     m_isShowingNavigationGestureSnapshot = false;
 }
 
+void WebPageProxy::willChangeCurrentHistoryItem()
+{
+    recordNavigationSnapshot();
+}
+
 } // namespace WebKit

Source/WebKit2/UIProcess/WebPageProxy.h

@@ -1031,6 +1031,7 @@ class WebPageProxy : public API::ObjectImpl<API::Object::Type::Page>
     void didBlockInsecurePluginVersion(const String& mimeType, const String& pluginURLString, const String& frameURLString, const String& pageURLString, bool replacementObscured);
 #endif // ENABLE(NETSCAPE_PLUGIN_API)
     void setCanShortCircuitHorizontalWheelEvents(bool canShortCircuitHorizontalWheelEvents) { m_canShortCircuitHorizontalWheelEvents = canShortCircuitHorizontalWheelEvents; }
+    void willChangeCurrentHistoryItem();
 
     void reattachToWebProcess();
     uint64_t reattachToWebProcessWithItem(WebBackForwardListItem*);

Source/WebKit2/UIProcess/WebPageProxy.messages.in

@@ -403,4 +403,6 @@ messages -> WebPageProxy {
     DidStartLoadForQuickLookDocumentInMainFrame(String fileName, String uti)
     DidFinishLoadForQuickLookDocumentInMainFrame(WebKit::QuickLookDocumentData data)
 #endif
+
+    WillChangeCurrentHistoryItem()
 }

Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm

@@ -165,12 +165,11 @@ - (UIPanGestureRecognizer *)gestureRecognizerForInteractiveTransition:(_UINaviga
     if (m_activeGestureType != ViewGestureType::None)
         return;
 
-    m_webPageProxyForBackForwardListForCurrentSwipe = m_alternateBackForwardListSourceView.get() ? m_alternateBackForwardListSourceView.get()->_page : &m_webPageProxy;
+    m_webPageProxy.recordNavigationSnapshot();
 
+    m_webPageProxyForBackForwardListForCurrentSwipe = m_alternateBackForwardListSourceView.get() ? m_alternateBackForwardListSourceView.get()->_page : &m_webPageProxy;
     m_webPageProxyForBackForwardListForCurrentSwipe->navigationGestureDidBegin();
 
-    m_webPageProxy.recordNavigationSnapshot();
-
     auto backForwardList = m_webPageProxyForBackForwardListForCurrentSwipe->backForwardList();
 
     // Copy the snapshot from this view to the one that owns the back forward list, so that
@@ -273,16 +272,9 @@ - (UIPanGestureRecognizer *)gestureRecognizerForInteractiveTransition:(_UINaviga
     if (ViewSnapshot* snapshot = targetItem->snapshot())
         m_snapshotRemovalTargetRenderTreeSize = snapshot->renderTreeSize() * swipeSnapshotRemovalRenderTreeSizeTargetFraction;
 
-    // We don't want to replace the current back-forward item's snapshot
-    // like we normally would when going back or forward, because we are
-    // displaying the destination item's snapshot.
-    ViewSnapshotStore::shared().disableSnapshotting();
-
     m_webPageProxyForBackForwardListForCurrentSwipe->navigationGestureDidEnd(true, *targetItem);
     m_webPageProxyForBackForwardListForCurrentSwipe->goToBackForwardItem(targetItem);
 
-    ViewSnapshotStore::shared().enableSnapshotting();
-
     uint64_t pageID = m_webPageProxy.pageID();
     m_webPageProxy.drawingArea()->dispatchAfterEnsuringDrawing([pageID] (CallbackBase::Error error) {
         auto gestureControllerIter = viewGestureControllersForAllPages().find(pageID);

Source/WebKit2/UIProcess/mac/ViewGestureControllerMac.mm

@@ -114,6 +114,9 @@ @implementation WKSwipeCancellationTracker
     if (m_swipeCancellationTracker)
         [m_swipeCancellationTracker setIsCancelled:YES];
 
+    if (m_activeGestureType == ViewGestureType::Swipe)
+        removeSwipeSnapshot();
+
     m_webPageProxy.process().removeMessageReceiver(Messages::ViewGestureController::messageReceiverName(), m_webPageProxy.pageID());
 }
 
@@ -366,7 +369,7 @@ static float maximumRectangleComponentDelta(FloatRect a, FloatRect b)
 
 void ViewGestureController::trackSwipeGesture(NSEvent *event, SwipeDirection direction)
 {
-    ViewSnapshotStore::shared().recordSnapshot(m_webPageProxy);
+    m_webPageProxy.recordNavigationSnapshot();
 
     CGFloat maxProgress = (direction == SwipeDirection::Left) ? 1 : 0;
     CGFloat minProgress = (direction == SwipeDirection::Right) ? -1 : 0;
@@ -638,13 +641,8 @@ static bool layerGeometryFlippedToRoot(CALayer *layer)
 
     m_webPageProxy.process().send(Messages::ViewGestureGeometryCollector::SetRenderTreeSizeNotificationThreshold(renderTreeSize * swipeSnapshotRemovalRenderTreeSizeTargetFraction), m_webPageProxy.pageID());
 
-    // We don't want to replace the current back-forward item's snapshot
-    // like we normally would when going back or forward, because we are
-    // displaying the destination item's snapshot.
-    ViewSnapshotStore::shared().disableSnapshotting();
     m_webPageProxy.navigationGestureDidEnd(true, *targetItem);
     m_webPageProxy.goToBackForwardItem(targetItem);
-    ViewSnapshotStore::shared().enableSnapshotting();
 
     if (!renderTreeSize) {
         removeSwipeSnapshot();

Source/WebKit2/UIProcess/mac/ViewSnapshotStore.h

@@ -119,9 +119,6 @@ class ViewSnapshotStore {
 
     void recordSnapshot(WebPageProxy&);
 
-    void disableSnapshotting() { m_enabled = false; }
-    void enableSnapshotting() { m_enabled = true; }
-
     void discardSnapshotImages();
 
 #if USE_RENDER_SERVER_VIEW_SNAPSHOTS
@@ -133,7 +130,6 @@ class ViewSnapshotStore {
     void willRemoveImageFromSnapshot(ViewSnapshot&);
     void pruneSnapshots(WebPageProxy&);
 
-    bool m_enabled;
     size_t m_snapshotCacheSize;
 
     ListHashSet<ViewSnapshot*> m_snapshotsWithImages;

Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm

@@ -47,8 +47,7 @@
 namespace WebKit {
 
 ViewSnapshotStore::ViewSnapshotStore()
-    : m_enabled(true)
-    , m_snapshotCacheSize(0)
+    : m_snapshotCacheSize(0)
 {
 }
 
@@ -109,11 +108,11 @@
 
 void ViewSnapshotStore::recordSnapshot(WebPageProxy& webPageProxy)
 {
-    WebBackForwardListItem* item = webPageProxy.backForwardList().currentItem();
-
-    if (!m_enabled)
+    if (webPageProxy.isShowingNavigationGestureSnapshot())
         return;
 
+    WebBackForwardListItem* item = webPageProxy.backForwardList().currentItem();
+
     if (!item)
         return;
 

Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

@@ -1621,4 +1621,13 @@ PassRefPtr<FrameNetworkingContext> WebFrameLoaderClient::createNetworkingContext
     return context.release();
 }
 
+void WebFrameLoaderClient::willChangeCurrentHistoryItem()
+{
+    WebPage* webPage = m_frame->page();
+    if (!webPage)
+        return;
+
+    webPage->willChangeCurrentHistoryItem();
+}
+
 } // namespace WebKit

Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h

@@ -130,6 +130,7 @@ class WebFrameLoaderClient : public WebCore::FrameLoaderClient {
     virtual void updateGlobalHistoryRedirectLinks() override;
 
     virtual bool shouldGoToHistoryItem(WebCore::HistoryItem*) const override;
+    virtual void willChangeCurrentHistoryItem() override;
 
     virtual void didDisplayInsecureContent() override;
     virtual void didRunInsecureContent(WebCore::SecurityOrigin*, const WebCore::URL&) override;

Source/WebKit2/WebProcess/WebPage/WebPage.cpp

@@ -4830,4 +4830,9 @@ void WebPage::didChangeScrollOffsetForFrame(Frame* frame)
     updateMainFrameScrollOffsetPinning();
 }
 
+void WebPage::willChangeCurrentHistoryItem()
+{
+    send(Messages::WebPageProxy::WillChangeCurrentHistoryItem());
+}
+
 } // namespace WebKit

Source/WebKit2/WebProcess/WebPage/WebPage.h

@@ -839,6 +839,8 @@ class WebPage : public API::ObjectImpl<API::Object::Type::BundlePage>, public IP
 
     void didChangeScrollOffsetForFrame(WebCore::Frame*);
 
+    void willChangeCurrentHistoryItem();
+
 private:
     WebPage(uint64_t pageID, const WebPageCreationParameters&);