Commit bcd93e64 authored by ggaren's avatar ggaren

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.



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@24353 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 8e9c2713
2007-07-16 Geoffrey Garen <ggaren@apple.com>
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.
2007-07-16 Sam Weinig <sam@webkit.org>
Reviewed by Oliver.
This page dumps the back/forward list that results from calling location.assign
============== Back Forward List ==============
file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/location-assign-adds-history-item.html **nav target**
curr-> file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/location-assign-adds-history-item.html?1 **nav target**
===============================================
<p>This page dumps the back/forward list that results from calling location.assign</p>
<script src="../../http/tests/navigation/resources/document-location.js"></script>
<script>
window.onload = start;
function runTest()
{
location.assign(location + "?1");
}
</script>
This page dumps the back/forward list that results from setting window.location.href
============== Back Forward List ==============
file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/location-href-set-adds-history-item.html **nav target**
curr-> file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/location-href-set-adds-history-item.html?1 **nav target**
===============================================
<p>This page dumps the back/forward list that results from setting window.location.href</p>
<script src="../../http/tests/navigation/resources/document-location.js"></script>
<script>
window.onload = start;
function runTest()
{
location.href = location + "?1";
}
</script>
This page dumps the back/forward list that results from calling location.replace
============== Back Forward List ==============
curr-> file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/location-replace-adds-history-item.html?1 **nav target**
===============================================
<p>This page dumps the back/forward list that results from calling location.replace</p>
<script src="../../http/tests/navigation/resources/document-location.js"></script>
<script>
window.onload = start;
function runTest()
{
location.replace(location + "?1");
}
</script>
This page dumps the back/forward list that results from setting window.location
============== Back Forward List ==============
file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/location-set-adds-history-item.html **nav target**
curr-> file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/location-set-adds-history-item.html?1 **nav target**
===============================================
<p>This page dumps the back/forward list that results from setting window.location</p>
<script src="../../http/tests/navigation/resources/document-location.js"></script>
<script>
window.onload = start;
function runTest()
{
window.location += "?1";
}
</script>
This page dumps the back/forward list that results from calling window.open targeted at the current window
============== Back Forward List ==============
file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/window-open-adds-history-item.html **nav target**
curr-> file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/window-open-adds-history-item.html?1 **nav target**
===============================================
<p>This page dumps the back/forward list that results from calling window.open targeted at the current window</p>
<script src="../../http/tests/navigation/resources/document-location.js"></script>
<script>
window.onload = start;
function runTest()
{
var tmp = window.name;
window.name = "myWindow";
window.open(window.location + "?1", "myWindow");
window.name = tmp;
}
</script>
This page dumps the back/forward list that results from calling window.open targeted at _top
============== Back Forward List ==============
file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/window-open-adds-history-item2.html **nav target**
curr-> file:///Volumes/Big/ggaren/Labyrinth/OpenSource/LayoutTests/fast/history/window-open-adds-history-item2.html?1 **nav target**
===============================================
<p>This page dumps the back/forward list that results from calling window.open targeted at _top</p>
<script src="../../http/tests/navigation/resources/document-location.js"></script>
<script>
window.onload = start;
function runTest()
{
window.open(window.location + "?1", "_top");
}
</script>
2007-07-16 Geoffrey Garen <ggaren@apple.com>
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.
2007-07-16 Sam Weinig <sam@webkit.org>
Reviewed by Adam and Maciej.
......@@ -115,8 +115,8 @@ void JSHTMLDocument::setLocation(ExecState* exec, JSValue* value)
if (activeFrame)
str = activeFrame->document()->completeURL(str);
// We always want a new history item when assigning to document.location.
frame->loader()->scheduleLocationChange(str, activeFrame->loader()->outgoingReferrer(), false, true);
bool userGesture = static_cast<ScriptInterpreter*>(exec->dynamicInterpreter())->wasRunByUserGesture();
frame->loader()->scheduleLocationChange(str, activeFrame->loader()->outgoingReferrer(), false, userGesture);
}
// Custom functions
......
......@@ -746,7 +746,7 @@ void Window::put(ExecState* exec, const Identifier& propertyName, JSValue* value
if (!dstUrl.startsWith("javascript:", false) || isSafeScript(exec)) {
bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
// We want a new history item if this JS was called via a user gesture
impl()->frame()->loader()->scheduleLocationChange(dstUrl, p->loader()->outgoingReferrer(), !userGesture, userGesture);
impl()->frame()->loader()->scheduleLocationChange(dstUrl, p->loader()->outgoingReferrer(), false, userGesture);
}
}
return;
......@@ -1307,7 +1307,7 @@ JSValue *WindowFunc::callAsFunction(ExecState *exec, JSObject *thisObj, const Li
const Window* window = Window::retrieveWindow(frame);
if (!completedURL.isEmpty() && (!completedURL.startsWith("javascript:", false) || (window && window->isSafeScript(exec)))) {
bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
frame->loader()->scheduleLocationChange(completedURL, activeFrame->loader()->outgoingReferrer(), false/*don't lock history*/, userGesture);
frame->loader()->scheduleLocationChange(completedURL, activeFrame->loader()->outgoingReferrer(), false, userGesture);
}
return Window::retrieve(frame);
}
......@@ -1801,8 +1801,7 @@ void Location::put(ExecState *exec, const Identifier &p, JSValue *v, int attr)
Frame* activeFrame = Window::retrieveActive(exec)->impl()->frame();
if (!url.url().startsWith("javascript:", false) || (window && window->isSafeScript(exec))) {
bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
// We want a new history item if this JS was called via a user gesture
m_frame->loader()->scheduleLocationChange(url.url(), activeFrame->loader()->outgoingReferrer(), !userGesture, userGesture);
m_frame->loader()->scheduleLocationChange(url.url(), activeFrame->loader()->outgoingReferrer(), false, userGesture);
}
}
......@@ -1827,7 +1826,7 @@ JSValue *LocationFunc::callAsFunction(ExecState *exec, JSObject *thisObj, const
const Window* window = Window::retrieveWindow(frame);
if (!str.startsWith("javascript:", false) || (window && window->isSafeScript(exec))) {
bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
frame->loader()->scheduleLocationChange(p->loader()->completeURL(str).url(), p->loader()->outgoingReferrer(), true /*lock history*/, userGesture);
frame->loader()->scheduleLocationChange(p->loader()->completeURL(str).url(), p->loader()->outgoingReferrer(), true, userGesture);
}
}
break;
......@@ -1850,7 +1849,7 @@ JSValue *LocationFunc::callAsFunction(ExecState *exec, JSObject *thisObj, const
if (!dstUrl.startsWith("javascript:", false) || (window && window->isSafeScript(exec))) {
bool userGesture = static_cast<ScriptInterpreter *>(exec->dynamicInterpreter())->wasRunByUserGesture();
// We want a new history item if this JS was called via a user gesture
frame->loader()->scheduleLocationChange(dstUrl, p->loader()->outgoingReferrer(), !userGesture, userGesture);
frame->loader()->scheduleLocationChange(dstUrl, p->loader()->outgoingReferrer(), false, userGesture);
}
}
break;
......
......@@ -1745,7 +1745,7 @@ void Document::processHttpEquiv(const String &equiv, const String &content)
url = frame->loader()->url().url();
else
url = completeURL(url);
frame->loader()->scheduleRedirection(delay, url);
frame->loader()->scheduleHTTPRedirection(delay, url);
}
} else if (equalIgnoringCase(equiv, "set-cookie")) {
// FIXME: make setCookie work on XML documents too; e.g. in case of <html:meta .....>
......
......@@ -290,7 +290,7 @@ Frame* FrameLoader::createWindow(const FrameLoadRequest& request, const WindowFe
if (!request.frameName().isEmpty() && request.frameName() != "_blank")
if (Frame* frame = m_frame->tree()->find(request.frameName())) {
if (!request.resourceRequest().url().isEmpty())
frame->loader()->load(request, true, 0, 0, HashMap<String, String>());
frame->loader()->load(request, false, true, 0, 0, HashMap<String, String>());
if (Page* page = frame->page())
page->chrome()->focus();
created = false;
......@@ -396,7 +396,7 @@ void FrameLoader::urlSelected(const ResourceRequest& request, const String& _tar
if (frameRequest.resourceRequest().httpReferrer().isEmpty())
frameRequest.resourceRequest().setHTTPReferrer(m_outgoingReferrer);
urlSelected(frameRequest, triggeringEvent, userGesture);
urlSelected(frameRequest, triggeringEvent, lockHistory, userGesture);
}
bool FrameLoader::requestFrame(HTMLFrameOwnerElement* ownerElement, const String& urlString, const AtomicString& frameName)
......@@ -420,7 +420,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, userGestureHint());
frame->loader()->scheduleLocationChange(url.url(), m_outgoingReferrer, true, userGestureHint());
else
frame = loadSubframe(ownerElement, url, frameName, m_outgoingReferrer);
......@@ -832,7 +832,7 @@ void FrameLoader::receivedFirstData()
else
URL = m_frame->document()->completeURL(URL);
scheduleRedirection(delay, URL);
scheduleHTTPRedirection(delay, URL);
}
const String& FrameLoader::responseMIMEType() const
......@@ -1243,15 +1243,14 @@ KURL FrameLoader::completeURL(const String& url)
return m_frame->document()->completeURL(url).deprecatedString();
}
void FrameLoader::scheduleRedirection(double delay, const String& url)
void FrameLoader::scheduleHTTPRedirection(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
// We want a new history item if the refresh timeout is > 1 second.
if (!m_scheduledRedirection || delay <= m_scheduledRedirection->delay)
scheduleRedirection(new ScheduledRedirection(delay, url, delay <= 1, delay > 1));
scheduleRedirection(new ScheduledRedirection(delay, url, delay <= 1, false));
}
void FrameLoader::scheduleLocationChange(const String& url, const String& referrer, bool lockHistory, bool wasUserGesture)
......@@ -1785,10 +1784,10 @@ void FrameLoader::finalSetupForReplace(DocumentLoader* loader)
void FrameLoader::load(const KURL& URL, Event* event)
{
load(ResourceRequest(URL), true, event, 0, HashMap<String, String>());
load(ResourceRequest(URL), false, true, event, 0, HashMap<String, String>());
}
void FrameLoader::load(const FrameLoadRequest& request, bool userGesture, Event* event,
void FrameLoader::load(const FrameLoadRequest& request, bool lockHistory, bool userGesture, Event* event,
HTMLFormElement* submitForm, const HashMap<String, String>& formValues)
{
KURL url = request.resourceRequest().url();
......@@ -1818,7 +1817,7 @@ void FrameLoader::load(const FrameLoadRequest& request, bool userGesture, Event*
FrameLoadType loadType;
if (request.resourceRequest().cachePolicy() == ReloadIgnoringCacheData)
loadType = FrameLoadTypeReload;
else if (!userGesture)
else if (lockHistory)
loadType = FrameLoadTypeInternal;
else
loadType = FrameLoadTypeStandard;
......@@ -2959,19 +2958,19 @@ void FrameLoader::submitForm(const FrameLoadRequest& request, Event* event)
m_submittedFormURL = request.resourceRequest().url();
}
// FIXME: Why do we always pass true for userGesture?
load(request, true, event, m_formAboutToBeSubmitted.get(), m_formValuesAboutToBeSubmitted);
// FIXME: We should probably call userGestureHint() to tell whether this form submission was the result of a user gesture.
load(request, false, true, event, m_formAboutToBeSubmitted.get(), m_formValuesAboutToBeSubmitted);
clearRecordedFormValues();
}
void FrameLoader::urlSelected(const FrameLoadRequest& request, Event* event, bool userGesture)
void FrameLoader::urlSelected(const FrameLoadRequest& request, Event* event, bool lockHistory, bool userGesture)
{
FrameLoadRequest copy = request;
if (copy.resourceRequest().httpReferrer().isEmpty())
copy.resourceRequest().setHTTPReferrer(m_outgoingReferrer);
load(copy, userGesture, event, 0, HashMap<String, String>());
load(copy, lockHistory, userGesture, event, 0, HashMap<String, String>());
}
String FrameLoader::userAgent(const KURL& url) const
......
......@@ -141,7 +141,7 @@ namespace WebCore {
void setupForReplaceByMIMEType(const String& newMIMEType);
void finalSetupForReplace(DocumentLoader*);
void load(const KURL&, Event*);
void load(const FrameLoadRequest&, bool userGesture,
void load(const FrameLoadRequest&, bool lockHistory, bool userGesture,
Event*, HTMLFormElement*, const HashMap<String, String>& formValues);
void load(const KURL&, const String& referrer, FrameLoadType, const String& target,
Event*, PassRefPtr<FormState>);
......@@ -275,7 +275,7 @@ namespace WebCore {
void changeLocation(const String& URL, const String& referrer, bool lockHistory = true, bool userGesture = false);
void changeLocation(const KURL& URL, const String& referrer, bool lockHistory = true, bool userGesture = false);
void urlSelected(const ResourceRequest&, const String& target, Event*, bool lockHistory, bool userGesture);
void urlSelected(const FrameLoadRequest&, Event*, bool userGesture);
void urlSelected(const FrameLoadRequest&, Event*, bool lockHistory, bool userGesture);
bool requestFrame(HTMLFrameOwnerElement*, const String& URL, const AtomicString& frameName);
Frame* loadSubframe(HTMLFrameOwnerElement*, const KURL& URL, const String& name, const String& referrer);
......@@ -297,12 +297,10 @@ namespace WebCore {
String baseTarget() const;
KURL dataURLBaseFromRequest(const ResourceRequest& request) const;
void scheduleRedirection(double delay, const String& URL);
bool isScheduledLocationChangePending() const;
void scheduleHTTPRedirection(double delay, const String& URL);
void scheduleLocationChange(const String& URL, const String& referrer, bool lockHistory = true, bool userGesture = false);
void scheduleRefresh(bool userGesture = false);
bool isScheduledLocationChangePending() const;
void scheduleHistoryNavigation(int steps);
bool canGoBackOrForward(int distance) const;
......
......@@ -202,7 +202,7 @@ void ContextMenuController::contextMenuItemSelected(ContextMenuItem* item)
case ContextMenuItemTagOpenLink:
if (Frame* targetFrame = result.targetFrame())
targetFrame->loader()->load(FrameLoadRequest(ResourceRequest(result.absoluteLinkURL(),
frame->loader()->outgoingReferrer())), true, 0, 0, HashMap<String, String>());
frame->loader()->outgoingReferrer())), false, true, 0, 0, HashMap<String, String>());
else
openNewWindow(result.absoluteLinkURL(), frame);
break;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment