LayoutTests:
authorbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Apr 2007 00:00:21 +0000 (00:00 +0000)
committerbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 Apr 2007 00:00:21 +0000 (00:00 +0000)
        Reviewed by Darin

        Layout test for the fix for <rdar://4921797> and http://bugs.webkit.org/show_bug.cgi?id=12005

        * http/tests/navigation/multiple-back-forward-entries-expected.txt: Added.
        * http/tests/navigation/multiple-back-forward-entries.html: Added.
        * http/tests/navigation/resources/slow-resource.pl: Added.

WebCore:

        Reviewed by Darin

        Fixes <rdar://4921797> and http://bugs.webkit.org/show_bug.cgi?id=12005

        The original regression was to claim that more loads were the result of a "user gesture" than really
        were.  A lot of the ways a frame load could be kicked off didn't properly set up this flag, and it
        wasn't properly propagated and respected where it should've been.

        This patch cleans much of that up.  One loose end is the "treatAsUserGesture" flag which is a stop
        gap measure to keep "slow redirects" working to create a new history item.  In the future, we need
        to cleanup the meaning and use of "userGesture" and "lockHistory."  This includes integrating them
        in to FrameLoadRequest and being very clear of what their meaning actually is at different stages of
        the Frame load process.

        * dom/Document.cpp:
        (WebCore::Document::processHttpEquiv): Pass only the delay for the redirect

        * html/HTMLAnchorElement.cpp:
        (WebCore::HTMLAnchorElement::defaultEventHandler): Pass "lockHistory" false, "userGesture" true

        * ksvg2/svg/SVGAElement.cpp:
        (WebCore::SVGAElement::defaultEventHandler): Pass "lockHistory" false, "userGesture" true

        * loader/FrameLoader.cpp:
        (WebCore::ScheduledRedirection::ScheduledRedirection): Figure "lockHistory" and "userGesture" from the
          delay here, instead of at 3 other different sites that call this method
        (WebCore::FrameLoader::changeLocation): Set userGesture correctly
        (WebCore::FrameLoader::urlSelected): Propagate userGesture down
        (WebCore::FrameLoader::requestFrame):
        (WebCore::FrameLoader::receivedFirstData):
        (WebCore::FrameLoader::scheduleRedirection): Pass only the delay here
        (WebCore::FrameLoader::redirectionTimerFired): Set userGesture correctly
        (WebCore::FrameLoader::load):
        (WebCore::FrameLoader::updateHistoryForInternalLoad): Insteading of asserting we aren't a redirect,
          handle the case where we *are* a redirect by updating the previous history item
        * loader/FrameLoader.h:

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

LayoutTests/ChangeLog
LayoutTests/http/tests/navigation/multiple-back-forward-entries-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/navigation/multiple-back-forward-entries.html [new file with mode: 0644]
LayoutTests/http/tests/navigation/resources/slow-resource.pl [new file with mode: 0755]
WebCore/ChangeLog
WebCore/dom/Document.cpp
WebCore/html/HTMLAnchorElement.cpp
WebCore/ksvg2/svg/SVGAElement.cpp
WebCore/loader/FrameLoader.cpp
WebCore/loader/FrameLoader.h

index e912d71..f1ba887 100644 (file)
@@ -1,3 +1,13 @@
+2007-04-09  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Darin
+
+        Layout test for the fix for <rdar://4921797> and http://bugs.webkit.org/show_bug.cgi?id=12005
+
+        * http/tests/navigation/multiple-back-forward-entries-expected.txt: Added.
+        * http/tests/navigation/multiple-back-forward-entries.html: Added.
+        * http/tests/navigation/resources/slow-resource.pl: Added.
+
 2007-04-09  Justin Garcia  <justin.garcia@apple.com>
 
         Reviewed by ggaren
diff --git a/LayoutTests/http/tests/navigation/multiple-back-forward-entries-expected.txt b/LayoutTests/http/tests/navigation/multiple-back-forward-entries-expected.txt
new file mode 100644 (file)
index 0000000..ffa9740
--- /dev/null
@@ -0,0 +1,7 @@
+This test creates an iFrame via document.write(), then immediately changes the source of that iframe while the main document is still being parsed. The src change should not result in a new back/forward list entry.
+
+
+============== Back Forward List ==============
+curr->  http://127.0.0.1:8000/navigation/multiple-back-forward-entries.html  **nav target**
+            http://127.0.0.1:8000/navigation/resources/slow-resource.pl (in frame "frame")
+===============================================
diff --git a/LayoutTests/http/tests/navigation/multiple-back-forward-entries.html b/LayoutTests/http/tests/navigation/multiple-back-forward-entries.html
new file mode 100644 (file)
index 0000000..1dab98a
--- /dev/null
@@ -0,0 +1,24 @@
+<html>
+<head>
+<title>Multiple Back/Forward Entries</title>
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.dumpAsText();
+        layoutTestController.dumpBackForwardList();
+        layoutTestController.waitUntilDone();
+    }
+    function loadDone() {
+        if (window.layoutTestController)
+            layoutTestController.notifyDone();
+    }
+</script>
+</head>
+<body>
+This test creates an iFrame via document.write(), then immediately changes the source of that iframe while the main document is still being parsed.  The src change should not result in a new back/forward list entry.<br>
+    <script>
+        document.write('<iframe id="frame" src="http://127.0.0.1:8000/navigation/resources/slow-resource.pl" width="500" height="500"></iframe>');
+        document.getElementById('frame').src = "http://127.0.0.1:8000/navigation/resources/slow-resource.pl";
+        setTimeout("loadDone();", 2000);
+    </script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/navigation/resources/slow-resource.pl b/LayoutTests/http/tests/navigation/resources/slow-resource.pl
new file mode 100755 (executable)
index 0000000..4893e85
--- /dev/null
@@ -0,0 +1,16 @@
+#!/usr/bin/perl -w
+
+# flush the buffers after each print
+select (STDOUT);
+$| = 1;
+
+print "Content-Type: text/plain\n";
+print "Expires: Thu, 01 Dec 2003 16:00:00 GMT\n";
+print "Cache-Control: no-store, no-cache, must-revalidate\n";
+print "Pragma: no-cache\n";
+print "\n";
+
+print "Test page for back/forward list that takes awhile to\n";
+sleep 5;
+print "Finish loading\n";
+
index c139aae..d21fa86 100644 (file)
@@ -1,3 +1,42 @@
+2007-04-09  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Darin
+
+        Fixes <rdar://4921797> and http://bugs.webkit.org/show_bug.cgi?id=12005
+
+        The original regression was to claim that more loads were the result of a "user gesture" than really
+        were.  A lot of the ways a frame load could be kicked off didn't properly set up this flag, and it 
+        wasn't properly propagated and respected where it should've been.
+
+        This patch cleans much of that up.  One loose end is the "treatAsUserGesture" flag which is a stop
+        gap measure to keep "slow redirects" working to create a new history item.  In the future, we need
+        to cleanup the meaning and use of "userGesture" and "lockHistory."  This includes integrating them 
+        in to FrameLoadRequest and being very clear of what their meaning actually is at different stages of
+        the Frame load process.
+
+        * dom/Document.cpp:
+        (WebCore::Document::processHttpEquiv): Pass only the delay for the redirect
+
+        * html/HTMLAnchorElement.cpp:
+        (WebCore::HTMLAnchorElement::defaultEventHandler): Pass "lockHistory" false, "userGesture" true
+
+        * ksvg2/svg/SVGAElement.cpp:
+        (WebCore::SVGAElement::defaultEventHandler): Pass "lockHistory" false, "userGesture" true
+
+        * loader/FrameLoader.cpp:
+        (WebCore::ScheduledRedirection::ScheduledRedirection): Figure "lockHistory" and "userGesture" from the 
+          delay here, instead of at 3 other different sites that call this method
+        (WebCore::FrameLoader::changeLocation): Set userGesture correctly
+        (WebCore::FrameLoader::urlSelected): Propagate userGesture down
+        (WebCore::FrameLoader::requestFrame): 
+        (WebCore::FrameLoader::receivedFirstData):
+        (WebCore::FrameLoader::scheduleRedirection): Pass only the delay here
+        (WebCore::FrameLoader::redirectionTimerFired): Set userGesture correctly
+        (WebCore::FrameLoader::load):
+        (WebCore::FrameLoader::updateHistoryForInternalLoad): Insteading of asserting we aren't a redirect,
+          handle the case where we *are* a redirect by updating the previous history item
+        * loader/FrameLoader.h:
+
 2007-04-09  Anders Carlsson  <andersca@apple.com>
 
         Reviewed by Darin.
index 20891b0..077632d 100644 (file)
@@ -1745,10 +1745,10 @@ void Document::processHttpEquiv(const String &equiv, const String &content)
         String url;
         if (frame && parseHTTPRefresh(content, true, delay, url)) {
             if (url.isEmpty())
-                frame->loader()->scheduleRedirection(delay, frame->loader()->url().url(), delay <= 1);
+                url = frame->loader()->url().url();
             else
-                // We want a new history item if the refresh timeout > 1 second
-                frame->loader()->scheduleRedirection(delay, completeURL(url), delay <= 1);
+                url = completeURL(url);
+            frame->loader()->scheduleRedirection(delay, url);
         }
     } else if (equalIgnoringCase(equiv, "set-cookie")) {
         // FIXME: make setCookie work on XML documents too; e.g. in case of <html:meta .....>
index a44238b..18b92c8 100644 (file)
@@ -215,7 +215,7 @@ void HTMLAnchorElement::defaultEventHandler(Event* evt)
         }
 
         if (!evt->defaultPrevented() && document()->frame())
-            document()->frame()->loader()->urlSelected(document()->completeURL(url), target, evt);
+            document()->frame()->loader()->urlSelected(document()->completeURL(url), target, evt, false, true);
 
         evt->setDefaultHandled();
     } else if (m_isLink && isContentEditable()) {
index f4d5f99..99e1ed6 100644 (file)
@@ -115,7 +115,7 @@ void SVGAElement::defaultEventHandler(Event* evt)
         String url = parseURL(href());
         if (!evt->defaultPrevented())
             if (document()->frame())
-                document()->frame()->loader()->urlSelected(document()->completeURL(url), target, evt);
+                document()->frame()->loader()->urlSelected(document()->completeURL(url), target, evt, false, true);
 
         evt->setDefaultHandled();
     }
index cbb8604..9e60ffe 100644 (file)
@@ -123,13 +123,13 @@ struct ScheduledRedirection {
     bool lockHistory;
     bool wasUserGesture;
 
-    ScheduledRedirection(double redirectDelay, const String& redirectURL, bool redirectLockHistory)
+    ScheduledRedirection(double redirectDelay, const String& redirectURL, bool redirectLockHistory, bool userGesture)
         : type(redirection)
         , delay(redirectDelay)
         , URL(redirectURL)
         , historySteps(0)
         , lockHistory(redirectLockHistory)
-        , wasUserGesture(false)
+        , wasUserGesture(userGesture)
     {
     }
 
@@ -329,10 +329,10 @@ void FrameLoader::changeLocation(const String& URL, const String& referrer, bool
         ? ReloadIgnoringCacheData : UseProtocolCachePolicy;
     ResourceRequest request(completeURL(URL), referrer, policy);
     
-    urlSelected(request, "_self", 0, lockHistory);
+    urlSelected(request, "_self", 0, lockHistory, userGesture);
 }
 
-void FrameLoader::urlSelected(const ResourceRequest& request, const String& _target, Event* triggeringEvent, bool lockHistory)
+void FrameLoader::urlSelected(const ResourceRequest& request, const String& _target, Event* triggeringEvent, bool lockHistory, bool userGesture)
 {
     String target = _target;
     if (target.isEmpty() && m_frame->document())
@@ -352,7 +352,7 @@ void FrameLoader::urlSelected(const ResourceRequest& request, const String& _tar
     if (frameRequest.resourceRequest().httpReferrer().isEmpty())
         frameRequest.resourceRequest().setHTTPReferrer(m_outgoingReferrer);
 
-    urlSelected(frameRequest, triggeringEvent);
+    urlSelected(frameRequest, triggeringEvent, userGesture);
 }
 
 bool FrameLoader::requestFrame(HTMLFrameOwnerElement* ownerElement, const String& urlString, const AtomicString& frameName)
@@ -368,7 +368,7 @@ bool FrameLoader::requestFrame(HTMLFrameOwnerElement* ownerElement, const String
 
     Frame* frame = m_frame->tree()->child(frameName);
     if (frame)
-        frame->loader()->scheduleLocationChange(url.url(), m_outgoingReferrer, false, false);
+        frame->loader()->scheduleLocationChange(url.url(), m_outgoingReferrer, false, userGestureHint());
     else
         frame = loadSubframe(ownerElement, url, frameName, m_outgoingReferrer);
     
@@ -772,8 +772,7 @@ void FrameLoader::receivedFirstData()
     else
         URL = m_frame->document()->completeURL(URL);
 
-    // We want a new history item if the refresh timeout > 1 second
-    scheduleRedirection(delay, URL, delay <= 1);
+    scheduleRedirection(delay, URL);
 }
 
 const String& FrameLoader::responseMIMEType() const
@@ -1155,12 +1154,15 @@ KURL FrameLoader::completeURL(const String& url)
     return m_frame->document()->completeURL(url).deprecatedString();
 }
 
-void FrameLoader::scheduleRedirection(double delay, const String& url, bool doLockHistory)
+void FrameLoader::scheduleRedirection(double delay, const String& url)
 {
     if (delay < 0 || delay > INT_MAX / 1000)
         return;
+        
+    // We want a new history item if the refresh timeout > 1 second.  We accomplish this
+    // by pretending a slow redirect is a user gesture and passing false for lockHistory
     if (!m_scheduledRedirection || delay <= m_scheduledRedirection->delay)
-        scheduleRedirection(new ScheduledRedirection(delay, url, doLockHistory));
+        scheduleRedirection(new ScheduledRedirection(delay, url, delay <= 1, delay > 1));
 }
 
 void FrameLoader::scheduleLocationChange(const String& url, const String& referrer, bool lockHistory, bool wasUserGesture)
@@ -1283,7 +1285,7 @@ void FrameLoader::redirectionTimerFired(Timer<FrameLoader>*)
         case ScheduledRedirection::historyNavigation:
             if (redirection->historySteps == 0) {
                 // Special case for go(0) from a frame -> reload only the frame
-                urlSelected(m_URL, "", 0);
+                urlSelected(m_URL, "", 0, redirection->lockHistory, redirection->wasUserGesture);
                 return;
             }
             // go(i!=0) from a frame navigates into the history of the frame only,
@@ -2828,14 +2830,13 @@ void FrameLoader::submitForm(const FrameLoadRequest& request, Event* event)
     clearRecordedFormValues();
 }
 
-void FrameLoader::urlSelected(const FrameLoadRequest& request, Event* event)
+void FrameLoader::urlSelected(const FrameLoadRequest& request, Event* event, bool userGesture)
 {
     FrameLoadRequest copy = request;
     if (copy.resourceRequest().httpReferrer().isEmpty())
         copy.resourceRequest().setHTTPReferrer(m_outgoingReferrer);
 
-    // FIXME: Why do we always pass true for userGesture?
-    load(copy, true, event, 0, HashMap<String, String>());
+    load(copy, userGesture, event, 0, HashMap<String, String>());
 }
     
 String FrameLoader::userAgent(const KURL& url) const
@@ -4032,21 +4033,25 @@ void FrameLoader::updateHistoryForInternalLoad()
     if (documentLoader())
         LOG(History, "WebCoreHistory - Updating History for internal load in frame %s", documentLoader()->title().utf8().data());
 #endif
-
-    // Add an item to the item tree for this frame
-    ASSERT(!documentLoader()->isClientRedirect());
-    Frame* parentFrame = m_frame->tree()->parent();
-    // The only case where parentItem is NULL should be when a parent frame loaded an
-    // empty URL, which doesn't set up a current item in that parent.
-    if (parentFrame) {
-        if (parentFrame->loader()->m_currentHistoryItem)
-            parentFrame->loader()->m_currentHistoryItem->addChildItem(createHistoryItem(true));
+    
+    if (documentLoader()->isClientRedirect()) {
+        m_currentHistoryItem->setURL(documentLoader()->URL());
+        m_currentHistoryItem->setFormInfoFromRequest(documentLoader()->request());
     } else {
-        // See 3556159. It's not clear if it's valid to be in FrameLoadTypeOnLoadEvent
-        // for a top-level frame, but that was a likely explanation for those crashes,
-        // so let's guard against it.
-        // ...and all FrameLoadTypeOnLoadEvent uses were folded to WebFrameLoadTypeInternal
-        LOG_ERROR("No parent frame in transitionToCommitted:, FrameLoadTypeInternal");
+        // Add an item to the item tree for this frame
+        Frame* parentFrame = m_frame->tree()->parent();
+        // The only case where parentItem is NULL should be when a parent frame loaded an
+        // empty URL, which doesn't set up a current item in that parent.
+        if (parentFrame) {
+            if (parentFrame->loader()->m_currentHistoryItem)
+                parentFrame->loader()->m_currentHistoryItem->addChildItem(createHistoryItem(true));
+        } else {
+            // See 3556159. It's not clear if it's valid to be in FrameLoadTypeOnLoadEvent
+            // for a top-level frame, but that was a likely explanation for those crashes,
+            // so let's guard against it.
+            // ...and all FrameLoadTypeOnLoadEvent uses were folded to WebFrameLoadTypeInternal
+            LOG_ERROR("No parent frame in transitionToCommitted:, FrameLoadTypeInternal");
+        }
     }
 }
 
index 4d70379..63a82cd 100644 (file)
@@ -264,8 +264,8 @@ namespace WebCore {
         void setDefersLoading(bool);
 
         void changeLocation(const String& URL, const String& referrer, bool lockHistory = true, bool userGesture = false);
-        void urlSelected(const ResourceRequest&, const String& target, Event*, bool lockHistory = false);
-        void urlSelected(const FrameLoadRequest&, Event*);
+        void urlSelected(const ResourceRequest&, const String& target, Event*, bool lockHistory, bool userGesture);
+        void urlSelected(const FrameLoadRequest&, Event*, bool userGesture);
       
         bool requestFrame(HTMLFrameOwnerElement*, const String& URL, const AtomicString& frameName);
         Frame* loadSubframe(HTMLFrameOwnerElement*, const KURL& URL, const String& name, const String& referrer);
@@ -287,7 +287,7 @@ namespace WebCore {
         String baseTarget() const;
         KURL dataURLBaseFromRequest(const ResourceRequest& request) const;
 
-        void scheduleRedirection(double delay, const String& URL, bool lockHistory = true);
+        void scheduleRedirection(double delay, const String& URL);
 
         void scheduleLocationChange(const String& URL, const String& referrer, bool lockHistory = true, bool userGesture = false);
         void scheduleRefresh(bool userGesture = false);