From 75301b3f6f0b69b972076b5e7d7912edc0751578 Mon Sep 17 00:00:00 2001 From: Justin Lebar Date: Thu, 26 Nov 2009 21:01:43 -0800 Subject: [PATCH] Bug 515190: Hashchange event should be dispatched synchronously. r=smaug --- docshell/base/nsDocShell.cpp | 2 +- docshell/base/nsDocShell.h | 6 ---- docshell/test/test_bug385434.html | 21 ++++++++---- dom/base/nsGlobalWindow.cpp | 17 +++------- dom/base/nsGlobalWindow.h | 5 ++- dom/base/nsPIDOMWindow.h | 5 ++- .../mochitest/general/test_bug504220.html | 32 +++++++------------ 7 files changed, 35 insertions(+), 53 deletions(-) diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index 075c29c2411..2466c9ad2a9 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -7964,7 +7964,7 @@ nsDocShell::InternalLoad(nsIURI * aURI, do_QueryInterface(mScriptGlobal); if (window) - window->DispatchAsyncHashchange(); + window->DispatchSyncHashchange(); } return NS_OK; diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h index 73fbdd4afcb..5b1d5927ea3 100644 --- a/docshell/base/nsDocShell.h +++ b/docshell/base/nsDocShell.h @@ -364,12 +364,6 @@ protected: PRUint32 aLoadType, nscoord *cx, nscoord *cy, PRBool * aDoHashchange); - // Dispatches the hashchange event to the current thread, if the document's - // readystate is "complete". - nsresult DispatchAsyncHashchange(); - - nsresult FireHashchange(); - // Returns PR_TRUE if would have called FireOnLocationChange, // but did not because aFireOnLocationChange was false on entry. // In this case it is the caller's responsibility to ensure diff --git a/docshell/test/test_bug385434.html b/docshell/test/test_bug385434.html index 108680a5b9b..9e5092953e5 100644 --- a/docshell/test/test_bug385434.html +++ b/docshell/test/test_bug385434.html @@ -25,6 +25,7 @@ SimpleTest.waitForExplicitFinish(); var gNumHashchanges = 0; var gCallbackOnIframeLoad = false; +var gCallbackOnHashchange = false; function statusMsg(msg) { var msgElem = document.createElement("p"); @@ -42,7 +43,14 @@ function longWait() { // event which was fired. function onIframeHashchange() { gNumHashchanges++; - gGen.next(); + if (gCallbackOnHashchange) { + gCallbackOnHashchange = false; + gGen.next(); + } +} + +function enableHashchangeCallback() { + gCallbackOnHashchange = true; } function onIframeLoad() { @@ -113,36 +121,35 @@ function run_test() { noEventExpected("No hashchange expected initially."); + enableHashchangeCallback(); sendMouseEvent({type: "click"}, "link1", frameCw); yield; eventExpected("Clicking link1 should trigger a hashchange."); + enableHashchangeCallback(); sendMouseEvent({type: "click"}, "link1", frameCw); longWait(); yield; // succeed if a hashchange event wasn't triggered while we were waiting noEventExpected("Clicking link1 again should not trigger a hashchange."); + enableHashchangeCallback(); sendMouseEvent({type: "click"}, "link2", frameCw); yield; eventExpected("Clicking link2 should trigger a hashchange."); frameCw.history.go(-1); - yield; eventExpected("Going back should trigger a hashchange."); frameCw.history.go(1); - yield; eventExpected("Going forward should trigger a hashchange."); frameCw.window.location.hash = ""; - yield; eventExpected("Changing window.location.hash should trigger a hashchange."); // window.location has a trailing '#' right now, so we append "link1", not // "#link1". frameCw.window.location = frameCw.window.location + "link1"; - yield; eventExpected("Assigning to window.location should trigger a hashchange."); // Set up history in the iframe which looks like: @@ -177,9 +184,8 @@ function run_test() { yield; frameCw.document.location = "file_bug385434_2.html#foo"; - yield; - eventExpected("frame onhashchange should fire events."); + // iframe should set gSampleEvent is(gSampleEvent.target, frameCw, "The hashchange event should be targeted to the window."); @@ -195,6 +201,7 @@ function run_test() { * hashchange is dispatched if the current document readyState is * not "complete" (bug 504837). */ + enableHashchangeCallback(); frameCw.document.location = "file_bug385434_3.html"; yield; eventExpected("Hashchange should fire even if the document " + diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp index 6ee9f14e697..d802d609889 100644 --- a/dom/base/nsGlobalWindow.cpp +++ b/dom/base/nsGlobalWindow.cpp @@ -6932,20 +6932,11 @@ nsGlobalWindow::PageHidden() } nsresult -nsGlobalWindow::DispatchAsyncHashchange() +nsGlobalWindow::DispatchSyncHashchange() { - FORWARD_TO_INNER(DispatchAsyncHashchange, (), NS_OK); - - nsCOMPtr event = - NS_NEW_RUNNABLE_METHOD(nsGlobalWindow, this, FireHashchange); - - return NS_DispatchToCurrentThread(event); -} - -nsresult -nsGlobalWindow::FireHashchange() -{ - NS_ENSURE_TRUE(IsInnerWindow(), NS_ERROR_FAILURE); + FORWARD_TO_INNER(DispatchSyncHashchange, (), NS_OK); + NS_ASSERTION(nsContentUtils::IsSafeToRunScript(), + "Must be safe to run script here."); // Don't do anything if the window is frozen. if (IsFrozen()) diff --git a/dom/base/nsGlobalWindow.h b/dom/base/nsGlobalWindow.h index cfafd3fcf3e..5c358588f61 100644 --- a/dom/base/nsGlobalWindow.h +++ b/dom/base/nsGlobalWindow.h @@ -443,7 +443,7 @@ public: virtual PRBool TakeFocus(PRBool aFocus, PRUint32 aFocusMethod); virtual void SetReadyForFocus(); virtual void PageHidden(); - virtual nsresult DispatchAsyncHashchange(); + virtual nsresult DispatchSyncHashchange(); virtual nsresult SetArguments(nsIArray *aArguments, nsIPrincipal *aOrigin); static PRBool DOMWindowDumpEnabled(); @@ -574,8 +574,7 @@ protected: const nsAString &aPopupWindowName, const nsAString &aPopupWindowFeatures); void FireOfflineStatusEvent(); - nsresult FireHashchange(); - + void FlushPendingNotifications(mozFlushType aType); void EnsureReflowFlushAndPaint(); nsresult CheckSecurityWidthAndHeight(PRInt32* width, PRInt32* height); diff --git a/dom/base/nsPIDOMWindow.h b/dom/base/nsPIDOMWindow.h index 59942b37480..4c90823e231 100644 --- a/dom/base/nsPIDOMWindow.h +++ b/dom/base/nsPIDOMWindow.h @@ -454,10 +454,9 @@ public: virtual void PageHidden() = 0; /** - * Instructs this window to asynchronously dispatch a hashchange event. This - * method must be called on an inner window. + * Instructs this window to synchronously dispatch a hashchange event. */ - virtual nsresult DispatchAsyncHashchange() = 0; + virtual nsresult DispatchSyncHashchange() = 0; /** diff --git a/dom/tests/mochitest/general/test_bug504220.html b/dom/tests/mochitest/general/test_bug504220.html index a0c1412ca52..b84c3e2e049 100644 --- a/dom/tests/mochitest/general/test_bug504220.html +++ b/dom/tests/mochitest/general/test_bug504220.html @@ -23,6 +23,8 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=504220 /** Test for Bug 504220 **/ function run_test() { + SimpleTest.waitForExplicitFinish(); + ok("onhashchange" in document.body, "document.body should contain 'onhashchange'."); @@ -35,30 +37,20 @@ function run_test() { // Likewise, document.body.hashchange should mirror window.onhashchange numEvents = 0; - var func2 = function() { numEvents++; gGen.next() }; + var func2 = function() { numEvents++; }; window.onhashchange = func2; is(document.body.onhashchange, func2); - SimpleTest.waitForExplicitFinish(); + // Change the document's hash. If we've been running this test manually, + // the hash might already be "#foo", so we need to check in order to be + // sure we trigger a hashchange. + if (location.hash != "#foo") + location.hash = "#foo"; + else + location.hash = "#bar"; - function waitForHashchange() { - // Change the document's hash. If we've been running this test manually, - // the hash might already be "#foo", so we need to check in order to be - // sure we trigger a hashchange. - if (location.hash != "#foo") - location.hash = "#foo"; - else - location.hash = "#bar"; - - yield; - - is(numEvents, 1, "Exactly one hashchange should have been fired."); - SimpleTest.finish(); - yield; - } - - var gGen = waitForHashchange(); - gGen.next(); + is(numEvents, 1, "Exactly one hashchange should have been fired."); + SimpleTest.finish(); }