Changeset 20813 in webkit

Timestamp:

Apr 9, 2007 5:00:21 PM (17 years ago)

Author:

beidson

Message:

LayoutTests:

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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/navigation/multiple-back-forward-entries-expected.txt (added)
• LayoutTests/http/tests/navigation/multiple-back-forward-entries.html (added)
• LayoutTests/http/tests/navigation/resources/slow-resource.pl (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/dom/Document.cpp (modified) (1 diff)
• WebCore/html/HTMLAnchorElement.cpp (modified) (1 diff)
• WebCore/ksvg2/svg/SVGAElement.cpp (modified) (1 diff)
• WebCore/loader/FrameLoader.cpp (modified) (11 diffs)
• WebCore/loader/FrameLoader.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/WebCore/dom/Document.cpp

@@ -1746 +1746 @@
         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")) {

trunk/WebCore/html/HTMLAnchorElement.cpp

@@ -216 +216 @@
 
         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();

trunk/WebCore/ksvg2/svg/SVGAElement.cpp

@@ -116 +116 @@
         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();

trunk/WebCore/loader/FrameLoader.cpp

@@ -124 +124 @@
     bool wasUserGesture;
 
-    ScheduledRedirection(double redirectDelay, const String& redirectURL, bool redirectLockHistory)
+    ScheduledRedirection(double redirectDelay, const String& redirectURL, bool redirectLockHistory, bool userGesture)
         : type(redirection)
         , delay(redirectDelay)
@@ -130 +130 @@
         , historySteps(0)
         , lockHistory(redirectLockHistory)
-        , wasUserGesture(false)
+        , wasUserGesture(userGesture)
     {
     }
@@ -330 +330 @@
     ResourceRequest request(completeURL(URL), referrer, policy);
     
-    urlSelected(request, "_self", 0, lockHistory);
-}
-
-void FrameLoader::urlSelected(const ResourceRequest& request, const String& _target, Event* triggeringEvent, bool lockHistory)
+    urlSelected(request, "_self", 0, lockHistory, userGesture);
+}
+
+void FrameLoader::urlSelected(const ResourceRequest& request, const String& _target, Event* triggeringEvent, bool lockHistory, bool userGesture)
 {
     String target = _target;
@@ -353 +353 @@
         frameRequest.resourceRequest().setHTTPReferrer(m_outgoingReferrer);
 
-    urlSelected(frameRequest, triggeringEvent);
+    urlSelected(frameRequest, triggeringEvent, userGesture);
 }
 
@@ -369 +369 @@
     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);
@@ -773 +773 @@
         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);
 }
 
@@ -1156 +1155 @@
 }
 
-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));
 }
 
@@ -1284 +1286 @@
             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;
             }
@@ -2829 +2831 @@
 }
 
-void FrameLoader::urlSelected(const FrameLoadRequest& request, Event* event)
+void FrameLoader::urlSelected(const FrameLoadRequest& request, Event* event, bool userGesture)
 {
     FrameLoadRequest copy = request;
@@ -2835 +2837 @@
         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>());
 }
     
@@ -4033 +4034 @@
         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");
+        }
     }
 }

trunk/WebCore/loader/FrameLoader.h

@@ -265 +265 @@
 
         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);
@@ -288 +288 @@
         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);