<rdar://problem/18978497> Wrong (off-by-1) navigation snapshots shown after a mix...

https://bugs.webkit.org/show_bug.cgi?id=138753

Reviewed by Tim Horton.

Source/WebCore:

Removed a FrameLoaderClient function that no one overrides anymore.

* loader/FrameLoaderClient.h:
(WebCore::FrameLoaderClient::willChangeCurrentHistoryItem): Deleted.
* loader/HistoryController.cpp:
(WebCore::HistoryController::setCurrentItem): Removed call to willChangeCurrentHistoryItem.
(WebCore::HistoryController::replaceCurrentItem): Ditto.

Source/WebKit2:

Rather than having the UI process record a snapshot jut before the Web Content process’s
notion of its current back/forward list item changes, have the UI process record a snapshot
right before its own current back/forward list item changes. This ensures that the right
snapshot gets attached to the right item in the UI process.

* UIProcess/WebBackForwardList.cpp:
(WebKit::WebBackForwardList::addItem): If there is a current item, record a navigation
snapshot of it before making the new item current.
(WebKit::WebBackForwardList::goToItem): If there is a current item, record a navigation
snapshot of it before making another item current.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::willChangeCurrentHistoryItemForMainFrame): Deleted.
* UIProcess/WebPageProxy.h:

* UIProcess/WebPageProxy.messages.in: Removed willChangeCurrentHistoryItemForMainFrame
message.

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::willChangeCurrentHistoryItem): Deleted.
* WebProcess/WebCoreSupport/WebFrameLoaderClient.h: Removed declaration of
willChangeCurrentHistoryItem override.

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::willChangeCurrentHistoryItemForMainFrame): Deleted.
* WebProcess/WebPage/WebPage.h:

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 4df6eb2..4c0df26 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,18 @@
+2014-11-14  Dan Bernstein  <mitz@apple.com>
+
+        <rdar://problem/18978497> Wrong (off-by-1) navigation snapshots shown after a mix of gesture and button back/forward navigation
+        https://bugs.webkit.org/show_bug.cgi?id=138753
+
+        Reviewed by Tim Horton.
+
+        Removed a FrameLoaderClient function that no one overrides anymore.
+
+        * loader/FrameLoaderClient.h:
+        (WebCore::FrameLoaderClient::willChangeCurrentHistoryItem): Deleted.
+        * loader/HistoryController.cpp:
+        (WebCore::HistoryController::setCurrentItem): Removed call to willChangeCurrentHistoryItem.
+        (WebCore::HistoryController::replaceCurrentItem): Ditto.
+
 2014-11-14  Jeremy Jones  <jeremyj@apple.com>
 
         Do proper teardown for optimized fullscreen

diff --git a/Source/WebCore/loader/FrameLoaderClient.h b/Source/WebCore/loader/FrameLoaderClient.h
index 3eda600..8f614c8 100644 (file)
--- a/Source/WebCore/loader/FrameLoaderClient.h
+++ b/Source/WebCore/loader/FrameLoaderClient.h
@@ -208,8 +208,6 @@ 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>.

diff --git a/Source/WebCore/loader/HistoryController.cpp b/Source/WebCore/loader/HistoryController.cpp
index fe9a30a..82424cb 100644 (file)
--- a/Source/WebCore/loader/HistoryController.cpp
+++ b/Source/WebCore/loader/HistoryController.cpp
@@ -574,8 +574,6 @@ void HistoryController::updateForFrameLoadCompleted()
 
 void HistoryController::setCurrentItem(HistoryItem* item)
 {
-    m_frame.loader().client().willChangeCurrentHistoryItem();
-
     m_frameLoadComplete = false;
     m_previousItem = m_currentItem;
     m_currentItem = item;
@@ -889,10 +887,8 @@ void HistoryController::replaceCurrentItem(HistoryItem* item)
     m_previousItem = nullptr;
     if (m_provisionalItem)
         m_provisionalItem = item;
-    else {
-        m_frame.loader().client().willChangeCurrentHistoryItem();
+    else
         m_currentItem = item;
-    }
 }
 
 } // namespace WebCore

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
index 3cf83b0..467e213 100644 (file)
--- a/Source/WebKit2/ChangeLog
+++ b/Source/WebKit2/ChangeLog
@@ -1,5 +1,39 @@
 2014-11-14  Dan Bernstein  <mitz@apple.com>
 
+        <rdar://problem/18978497> Wrong (off-by-1) navigation snapshots shown after a mix of gesture and button back/forward navigation
+        https://bugs.webkit.org/show_bug.cgi?id=138753
+
+        Reviewed by Tim Horton.
+
+        Rather than having the UI process record a snapshot jut before the Web Content process’s
+        notion of its current back/forward list item changes, have the UI process record a snapshot
+        right before its own current back/forward list item changes. This ensures that the right
+        snapshot gets attached to the right item in the UI process.
+
+        * UIProcess/WebBackForwardList.cpp:
+        (WebKit::WebBackForwardList::addItem): If there is a current item, record a navigation
+        snapshot of it before making the new item current.
+        (WebKit::WebBackForwardList::goToItem): If there is a current item, record a navigation
+        snapshot of it before making another item current.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::willChangeCurrentHistoryItemForMainFrame): Deleted.
+        * UIProcess/WebPageProxy.h:
+
+        * UIProcess/WebPageProxy.messages.in: Removed willChangeCurrentHistoryItemForMainFrame
+        message.
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::willChangeCurrentHistoryItem): Deleted.
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.h: Removed declaration of
+        willChangeCurrentHistoryItem override.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::willChangeCurrentHistoryItemForMainFrame): Deleted.
+        * WebProcess/WebPage/WebPage.h:
+
+2014-11-14  Dan Bernstein  <mitz@apple.com>
+
         Add some tracing to help investigating <rdar://problem/18905383> [iOS] Crash due to null m_webPageProxyForBackForwardListForCurrentSwipe in ViewGestureController::endSwipeGesture
         https://bugs.webkit.org/show_bug.cgi?id=138750
 

diff --git a/Source/WebKit2/UIProcess/WebBackForwardList.cpp b/Source/WebKit2/UIProcess/WebBackForwardList.cpp
index daaaa21..65f9581 100644 (file)
--- a/Source/WebKit2/UIProcess/WebBackForwardList.cpp
+++ b/Source/WebKit2/UIProcess/WebBackForwardList.cpp
@@ -92,6 +92,8 @@ void WebBackForwardList::addItem(WebBackForwardListItem* newItem)
     Vector<RefPtr<WebBackForwardListItem>> removedItems;
     
     if (m_hasCurrentIndex) {
+        m_page->recordNavigationSnapshot();
+
         // Toss everything in the forward list.
         unsigned targetSize = m_currentIndex + 1;
         removedItems.reserveCapacity(m_entries.size() - targetSize);
@@ -177,8 +179,10 @@ void WebBackForwardList::goToItem(WebBackForwardListItem* item)
     // item should remain in the list.
     WebBackForwardListItem* currentItem = m_entries[m_currentIndex].get();
     bool shouldKeepCurrentItem = true;
-    if (currentItem != item)
+    if (currentItem != item) {
+        m_page->recordNavigationSnapshot();
         shouldKeepCurrentItem = m_page->shouldKeepCurrentBackForwardListItemInList(m_entries[m_currentIndex].get());
+    }
 
     // If the client said to remove the current item, remove it and then update the target index.
     Vector<RefPtr<WebBackForwardListItem>> removedItems;

diff --git a/Source/WebKit2/UIProcess/WebPageProxy.cpp b/Source/WebKit2/UIProcess/WebPageProxy.cpp
index b40cb96..de01ca8 100644 (file)
--- a/Source/WebKit2/UIProcess/WebPageProxy.cpp
+++ b/Source/WebKit2/UIProcess/WebPageProxy.cpp
@@ -5298,11 +5298,6 @@ void WebPageProxy::navigationGestureSnapshotWasRemoved()
     m_isShowingNavigationGestureSnapshot = false;
 }
 
-void WebPageProxy::willChangeCurrentHistoryItemForMainFrame()
-{
-    recordNavigationSnapshot();
-}
-
 void WebPageProxy::isPlayingAudioDidChange(bool newIsPlayingAudio)
 {
     if (m_isPlayingAudio == newIsPlayingAudio)

diff --git a/Source/WebKit2/UIProcess/WebPageProxy.h b/Source/WebKit2/UIProcess/WebPageProxy.h
index 5fe798f..3692219 100644 (file)
--- a/Source/WebKit2/UIProcess/WebPageProxy.h
+++ b/Source/WebKit2/UIProcess/WebPageProxy.h
@@ -1077,7 +1077,6 @@ private:
     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 willChangeCurrentHistoryItemForMainFrame();
 
     void reattachToWebProcess();
     uint64_t reattachToWebProcessWithItem(WebBackForwardListItem*);

diff --git a/Source/WebKit2/UIProcess/WebPageProxy.messages.in b/Source/WebKit2/UIProcess/WebPageProxy.messages.in
index 7371609..438f0d3 100644 (file)
--- a/Source/WebKit2/UIProcess/WebPageProxy.messages.in
+++ b/Source/WebKit2/UIProcess/WebPageProxy.messages.in
@@ -408,8 +408,6 @@ messages -> WebPageProxy {
     DidFinishLoadForQuickLookDocumentInMainFrame(WebKit::QuickLookDocumentData data)
 #endif
 
-    WillChangeCurrentHistoryItemForMainFrame()
-
 #if ENABLE(CONTENT_FILTERING)
     ContentFilterDidBlockLoadForFrame(WebCore::ContentFilter contentFilter, uint64_t frameID)
 #endif

diff --git a/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp b/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
index 9ce0bef..187821d 100644 (file)
--- a/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
+++ b/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
@@ -1630,17 +1630,6 @@ PassRefPtr<FrameNetworkingContext> WebFrameLoaderClient::createNetworkingContext
     return context.release();
 }
 
-void WebFrameLoaderClient::willChangeCurrentHistoryItem()
-{
-    WebPage* webPage = m_frame->page();
-    if (!webPage)
-        return;
-    if (!m_frame->isMainFrame())
-        return;
-
-    webPage->willChangeCurrentHistoryItemForMainFrame();
-}
-
 #if ENABLE(CONTENT_FILTERING)
 void WebFrameLoaderClient::contentFilterDidBlockLoad(std::unique_ptr<WebCore::ContentFilter> contentFilter)
 {

diff --git a/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h b/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h
index 93e788a..8bf19ce 100644 (file)
--- a/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h
+++ b/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.h
@@ -130,7 +130,6 @@ private:
     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;

diff --git a/Source/WebKit2/WebProcess/WebPage/WebPage.cpp b/Source/WebKit2/WebProcess/WebPage/WebPage.cpp
index f2aa497..60c2e47 100644 (file)
--- a/Source/WebKit2/WebProcess/WebPage/WebPage.cpp
+++ b/Source/WebKit2/WebProcess/WebPage/WebPage.cpp
@@ -4811,9 +4811,4 @@ void WebPage::didChangeScrollOffsetForFrame(Frame* frame)
     updateMainFrameScrollOffsetPinning();
 }
 
-void WebPage::willChangeCurrentHistoryItemForMainFrame()
-{
-    send(Messages::WebPageProxy::WillChangeCurrentHistoryItemForMainFrame());
-}
-
 } // namespace WebKit

diff --git a/Source/WebKit2/WebProcess/WebPage/WebPage.h b/Source/WebKit2/WebProcess/WebPage/WebPage.h
index 78b2198..13524d6 100644 (file)
--- a/Source/WebKit2/WebProcess/WebPage/WebPage.h
+++ b/Source/WebKit2/WebProcess/WebPage/WebPage.h
@@ -844,8 +844,6 @@ public:
 
     void didChangeScrollOffsetForFrame(WebCore::Frame*);
 
-    void willChangeCurrentHistoryItemForMainFrame();
-
 private:
     WebPage(uint64_t pageID, const WebPageCreationParameters&);