Changeset 24353 in webkit


Ignore:
Timestamp:
Jul 16, 2007 11:17:50 PM (17 years ago)
Author:
ggaren
Message:

LayoutTests:

Reviewed by Sam Weinig.


Tests for <rdar://problem/5334483> REGRESSION: JavaScript-induced loads
not added to back/forward list

  • fast/history/location-assign-adds-history-item-expected.txt: Added.
  • fast/history/location-assign-adds-history-item.html: Added.
  • fast/history/location-href-set-adds-history-item-expected.txt: Added.
  • fast/history/location-href-set-adds-history-item.html: Added.
  • fast/history/location-replace-adds-history-item-expected.txt: Added.
  • fast/history/location-replace-adds-history-item.html: Added.
  • fast/history/location-set-adds-history-item-expected.txt: Added.
  • fast/history/location-set-adds-history-item.html: Added.
  • fast/history/window-open-adds-history-item-expected.txt: Added.
  • fast/history/window-open-adds-history-item.html: Added.
  • fast/history/window-open-adds-history-item2-expected.txt: Added.
  • fast/history/window-open-adds-history-item2.html: Added.
  • http/tests/navigation/redirect-load-no-form-restoration-expected.txt: Updated results. Adding a history entry here is correct behavior.

WebCore:

Reviewed by Sam Weinig.


Fixed <rdar://problem/5334483> REGRESSION: JavaScript-induced
window.open loads not added to back/forward list


I did an audit of our history rules in loading and tried to establish
some sane uniformity.


The uniform rule is:

  • HTTP redirects and HTTP redirects simulated by <meta http-equiv> add a history item if and only if the redirect takes > 1 second.
  • Other navigations, including JavaScript navigations, always add a history item, except for location.replace navigations.

In the future, we'll want to refine the second case to be more like the
first. I've filed <rdar://problem/5339292> about that.

  • bindings/js/JSHTMLDocumentCustom.cpp: (WebCore::JSHTMLDocument::setLocation): Don't pass 'true' for userGesture unconditionally. userGesture is used to determine popup blocking, not history item creation.
  • bindings/js/kjs_window.cpp: Pass 'false' for lockHistory in all loads except location.replace, which intends to lock history.
  • loader/FrameLoader.cpp: Distinguish between lockHistory and userGesture. The former determines whether a new history item gets created. The latter determines whether JavaScript can open popup windows. Start passing these variables in functions that used to swallow or conflate them.


(WebCore::FrameLoader::requestFrame): Pass 'true' for lockHistory here
because that's usually correct when setting the 'src' attribute of a
child frame, and we want to avoid regressing <rdar://problem/4921797>.

(WebCore::FrameLoader::load): Use the lockHistory variable to determine
whether to start a history-creating load. Using userGesture for this
purpose is wrong, as explained above.

  • loader/FrameLoader.h: Renamed one variant of scheduleRedirection to scheduleHTTPRedirection because the behavior there of measuring elapsed time is specific to the HTTP redirection case.
  • page/ContextMenuController.cpp: (WebCore::ContextMenuController::contextMenuItemSelected): lockHistory can always be false here because this navigation is never the result of a redirection.
Location:
trunk
Files:
12 added
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r24346 r24353  
     12007-07-16  Geoffrey Garen  <ggaren@apple.com>
     2
     3        Reviewed by Sam Weinig.
     4       
     5        Tests for <rdar://problem/5334483> REGRESSION: JavaScript-induced loads
     6        not added to back/forward list
     7
     8        * fast/history/location-assign-adds-history-item-expected.txt: Added.
     9        * fast/history/location-assign-adds-history-item.html: Added.
     10        * fast/history/location-href-set-adds-history-item-expected.txt: Added.
     11        * fast/history/location-href-set-adds-history-item.html: Added.
     12        * fast/history/location-replace-adds-history-item-expected.txt: Added.
     13        * fast/history/location-replace-adds-history-item.html: Added.
     14        * fast/history/location-set-adds-history-item-expected.txt: Added.
     15        * fast/history/location-set-adds-history-item.html: Added.
     16        * fast/history/window-open-adds-history-item-expected.txt: Added.
     17        * fast/history/window-open-adds-history-item.html: Added.
     18        * fast/history/window-open-adds-history-item2-expected.txt: Added.
     19        * fast/history/window-open-adds-history-item2.html: Added.
     20        * http/tests/navigation/redirect-load-no-form-restoration-expected.txt:
     21        Updated results. Adding a history entry here is correct behavior.
     22
    1232007-07-16  Sam Weinig  <sam@webkit.org>
    224

  • trunk/WebCore/ChangeLog

    r24351 r24353  
     12007-07-16  Geoffrey Garen  <ggaren@apple.com>
     2
     3        Reviewed by Sam Weinig.
     4       
     5        Fixed <rdar://problem/5334483> REGRESSION: JavaScript-induced
     6        window.open loads not added to back/forward list
     7       
     8        I did an audit of our history rules in loading and tried to establish
     9        some sane uniformity.
     10       
     11        The uniform rule is:
     12            - HTTP redirects and HTTP redirects simulated by <meta http-equiv>
     13            add a history item if and only if the redirect takes > 1 second.
     14            - Other navigations, including JavaScript navigations, always
     15            add a history item, except for location.replace navigations.
     16
     17        In the future, we'll want to refine the second case to be more like the
     18        first. I've filed <rdar://problem/5339292> about that.
     19
     20        * bindings/js/JSHTMLDocumentCustom.cpp:
     21        (WebCore::JSHTMLDocument::setLocation): Don't pass 'true' for
     22        userGesture unconditionally. userGesture is used to determine popup
     23        blocking, not history item creation.
     24
     25        * bindings/js/kjs_window.cpp: Pass 'false' for lockHistory in all loads
     26        except location.replace, which intends to lock history.
     27
     28        * loader/FrameLoader.cpp: Distinguish between lockHistory and
     29        userGesture. The former determines whether a new history item gets
     30        created. The latter determines whether JavaScript can open popup
     31        windows. Start passing these variables in functions that used to
     32        swallow or conflate them.
     33       
     34        (WebCore::FrameLoader::requestFrame): Pass 'true' for lockHistory here
     35        because that's usually correct when setting the 'src' attribute of a
     36        child frame, and we want to avoid regressing <rdar://problem/4921797>.
     37
     38        (WebCore::FrameLoader::load): Use the lockHistory variable to determine
     39        whether to start a history-creating load. Using userGesture for this
     40        purpose is wrong, as explained above.
     41
     42        * loader/FrameLoader.h: Renamed one variant of scheduleRedirection to
     43        scheduleHTTPRedirection because the behavior there of measuring elapsed
     44        time is specific to the HTTP redirection case.
     45
     46        * page/ContextMenuController.cpp:
     47        (WebCore::ContextMenuController::contextMenuItemSelected): lockHistory
     48        can always be false here because this navigation is never the result of
     49        a redirection.
     50
    1512007-07-16  Sam Weinig  <sam@webkit.org>
    252

  • trunk/WebCore/bindings/js/JSHTMLDocumentCustom.cpp

    r24004 r24353  
    116116        str = activeFrame->document()->completeURL(str);
    117117
    118     // We always want a new history item when assigning to document.location.
    119     frame->loader()->scheduleLocationChange(str, activeFrame->loader()->outgoingReferrer(), false, true);
     118    bool userGesture = static_cast<ScriptInterpreter*>(exec->dynamicInterpreter())->wasRunByUserGesture();
     119    frame->loader()->scheduleLocationChange(str, activeFrame->loader()->outgoingReferrer(), false, userGesture);
    120120}
    121121

  • trunk/WebCore/bindings/js/kjs_window.cpp

    r24351 r24353  
    747747          bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
    748748          // We want a new history item if this JS was called via a user gesture
    749           impl()->frame()->loader()->scheduleLocationChange(dstUrl, p->loader()->outgoingReferrer(), !userGesture, userGesture);
     749          impl()->frame()->loader()->scheduleLocationChange(dstUrl, p->loader()->outgoingReferrer(), false, userGesture);
    750750        }
    751751      }
    … …  
    13081308          if (!completedURL.isEmpty() && (!completedURL.startsWith("javascript:", false) || (window && window->isSafeScript(exec)))) {
    13091309              bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
    1310               frame->loader()->scheduleLocationChange(completedURL, activeFrame->loader()->outgoingReferrer(), false/*don't lock history*/, userGesture);
     1310              frame->loader()->scheduleLocationChange(completedURL, activeFrame->loader()->outgoingReferrer(), false, userGesture);
    13111311          }
    13121312          return Window::retrieve(frame);
    … …  
    18021802  if (!url.url().startsWith("javascript:", false) || (window && window->isSafeScript(exec))) {
    18031803    bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
    1804     // We want a new history item if this JS was called via a user gesture
    1805     m_frame->loader()->scheduleLocationChange(url.url(), activeFrame->loader()->outgoingReferrer(), !userGesture, userGesture);
     1804    m_frame->loader()->scheduleLocationChange(url.url(), activeFrame->loader()->outgoingReferrer(), false, userGesture);
    18061805  }
    18071806}
    … …  
    18281827        if (!str.startsWith("javascript:", false) || (window && window->isSafeScript(exec))) {
    18291828          bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
    1830           frame->loader()->scheduleLocationChange(p->loader()->completeURL(str).url(), p->loader()->outgoingReferrer(), true /*lock history*/, userGesture);
     1829          frame->loader()->scheduleLocationChange(p->loader()->completeURL(str).url(), p->loader()->outgoingReferrer(), true, userGesture);
    18311830        }
    18321831      }
    … …  
    18511850                bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
    18521851                // We want a new history item if this JS was called via a user gesture
    1853                 frame->loader()->scheduleLocationChange(dstUrl, p->loader()->outgoingReferrer(), !userGesture, userGesture);
     1852                frame->loader()->scheduleLocationChange(dstUrl, p->loader()->outgoingReferrer(), false, userGesture);
    18541853            }
    18551854        }

  • trunk/WebCore/dom/Document.cpp

    r24238 r24353  
    17461746            else
    17471747                url = completeURL(url);
    1748             frame->loader()->scheduleRedirection(delay, url);
     1748            frame->loader()->scheduleHTTPRedirection(delay, url);
    17491749        }
    17501750    } else if (equalIgnoringCase(equiv, "set-cookie")) {

  • trunk/WebCore/loader/FrameLoader.cpp

    r24299 r24353  
    291291        if (Frame* frame = m_frame->tree()->find(request.frameName())) {
    292292            if (!request.resourceRequest().url().isEmpty())
    293                 frame->loader()->load(request, true, 0, 0, HashMap<String, String>());
     293                frame->loader()->load(request, false, true, 0, 0, HashMap<String, String>());
    294294            if (Page* page = frame->page())
    295295                page->chrome()->focus();
    … …  
    397397        frameRequest.resourceRequest().setHTTPReferrer(m_outgoingReferrer);
    398398
    399     urlSelected(frameRequest, triggeringEvent, userGesture);
     399    urlSelected(frameRequest, triggeringEvent, lockHistory, userGesture);
    400400}
    401401
    … …  
    421421    Frame* frame = m_frame->tree()->child(frameName);
    422422    if (frame)
    423         frame->loader()->scheduleLocationChange(url.url(), m_outgoingReferrer, false, userGestureHint());
     423        frame->loader()->scheduleLocationChange(url.url(), m_outgoingReferrer, true, userGestureHint());
    424424    else
    425425        frame = loadSubframe(ownerElement, url, frameName, m_outgoingReferrer);
    … …  
    833833        URL = m_frame->document()->completeURL(URL);
    834834
    835     scheduleRedirection(delay, URL);
     835    scheduleHTTPRedirection(delay, URL);
    836836}
    837837
    … …  
    12441244}
    12451245
    1246 void FrameLoader::scheduleRedirection(double delay, const String& url)
     1246void FrameLoader::scheduleHTTPRedirection(double delay, const String& url)
    12471247{
    12481248    if (delay < 0 || delay > INT_MAX / 1000)
    12491249        return;
    12501250       
    1251     // We want a new history item if the refresh timeout > 1 second.  We accomplish this
    1252     // by pretending a slow redirect is a user gesture and passing false for lockHistory
     1251    // We want a new history item if the refresh timeout is > 1 second.
    12531252    if (!m_scheduledRedirection || delay <= m_scheduledRedirection->delay)
    1254         scheduleRedirection(new ScheduledRedirection(delay, url, delay <= 1, delay > 1));
     1253        scheduleRedirection(new ScheduledRedirection(delay, url, delay <= 1, false));
    12551254}
    12561255
    … …  
    17861785void FrameLoader::load(const KURL& URL, Event* event)
    17871786{
    1788     load(ResourceRequest(URL), true, event, 0, HashMap<String, String>());
    1789 }
    1790 
    1791 void FrameLoader::load(const FrameLoadRequest& request, bool userGesture, Event* event,
     1787    load(ResourceRequest(URL), false, true, event, 0, HashMap<String, String>());
     1788}
     1789
     1790void FrameLoader::load(const FrameLoadRequest& request, bool lockHistory, bool userGesture, Event* event,
    17921791    HTMLFormElement* submitForm, const HashMap<String, String>& formValues)
    17931792{
    … …  
    18191818        if (request.resourceRequest().cachePolicy() == ReloadIgnoringCacheData)
    18201819            loadType = FrameLoadTypeReload;
    1821         else if (!userGesture)
     1820        else if (lockHistory)
    18221821            loadType = FrameLoadTypeInternal;
    18231822        else
    … …  
    29602959    }
    29612960
    2962     // FIXME: Why do we always pass true for userGesture?
    2963     load(request, true, event, m_formAboutToBeSubmitted.get(), m_formValuesAboutToBeSubmitted);
     2961    // FIXME: We should probably call userGestureHint() to tell whether this form submission was the result of a user gesture.
     2962    load(request, false, true, event, m_formAboutToBeSubmitted.get(), m_formValuesAboutToBeSubmitted);
    29642963
    29652964    clearRecordedFormValues();
    29662965}
    29672966
    2968 void FrameLoader::urlSelected(const FrameLoadRequest& request, Event* event, bool userGesture)
     2967void FrameLoader::urlSelected(const FrameLoadRequest& request, Event* event, bool lockHistory, bool userGesture)
    29692968{
    29702969    FrameLoadRequest copy = request;
    … …  
    29722971        copy.resourceRequest().setHTTPReferrer(m_outgoingReferrer);
    29732972
    2974     load(copy, userGesture, event, 0, HashMap<String, String>());
     2973    load(copy, lockHistory, userGesture, event, 0, HashMap<String, String>());
    29752974}
    29762975   

  • trunk/WebCore/loader/FrameLoader.h

    r24276 r24353  
    142142        void finalSetupForReplace(DocumentLoader*);
    143143        void load(const KURL&, Event*);
    144         void load(const FrameLoadRequest&, bool userGesture,
     144        void load(const FrameLoadRequest&, bool lockHistory, bool userGesture,
    145145            Event*, HTMLFormElement*, const HashMap<String, String>& formValues);
    146146        void load(const KURL&, const String& referrer, FrameLoadType, const String& target,
    … …  
    276276        void changeLocation(const KURL& URL, const String& referrer, bool lockHistory = true, bool userGesture = false);
    277277        void urlSelected(const ResourceRequest&, const String& target, Event*, bool lockHistory, bool userGesture);
    278         void urlSelected(const FrameLoadRequest&, Event*, bool userGesture);
     278        void urlSelected(const FrameLoadRequest&, Event*, bool lockHistory, bool userGesture);
    279279     
    280280        bool requestFrame(HTMLFrameOwnerElement*, const String& URL, const AtomicString& frameName);
    … …  
    298298        KURL dataURLBaseFromRequest(const ResourceRequest& request) const;
    299299
    300         void scheduleRedirection(double delay, const String& URL);
    301 
     300        bool isScheduledLocationChangePending() const;
     301        void scheduleHTTPRedirection(double delay, const String& URL);
    302302        void scheduleLocationChange(const String& URL, const String& referrer, bool lockHistory = true, bool userGesture = false);
    303303        void scheduleRefresh(bool userGesture = false);
    304         bool isScheduledLocationChangePending() const;
    305 
    306304        void scheduleHistoryNavigation(int steps);
    307305

  • trunk/WebCore/page/ContextMenuController.cpp

    r24156 r24353  
    203203            if (Frame* targetFrame = result.targetFrame())
    204204                targetFrame->loader()->load(FrameLoadRequest(ResourceRequest(result.absoluteLinkURL(),
    205                     frame->loader()->outgoingReferrer())), true, 0, 0, HashMap<String, String>());
     205                    frame->loader()->outgoingReferrer())), false, true, 0, 0, HashMap<String, String>());
    206206            else
    207207                openNewWindow(result.absoluteLinkURL(), frame);
Note: See TracChangeset for help on using the changeset viewer.