385434 - Add support for HTML5 onhashchange (event for named anchor changes)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Agustín Fernández]

There is currently no way to detect a change in the url of a page other than polling for changes in document.location.hash all the time (which is slow and potentially complex) or listening for click events on all links (which doesn't catch changes not started by clicking on links -such as clicking back and forward, or changing the hash by hand-).

Changing the hash in a page is useful to provide bookmarks and back and forward functionality in ajax driven web applications which never fetch a new page and is used extensively in the web.

I propose an urlchange (urlhashchange? hashchange? locationchange?) event which would be dispatched by the BODY element whenever the hash portion of the url changes.

Comment 1

[Hixie (not reading bugmail)]

Interesting idea. Could you send the proposal to the WHATWG list? 
([e-mail], if you're subscribed, or [e-mail], if you're not.)

Comment 2

[Hixie (not reading bugmail)]

FYI, this is now in the HTML5 spec.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So this event only fires on scripted session history traversals?  That's how I read the current spec language.

Comment 4

[Hixie (not reading bugmail)]

No, it applies whenever history traversal occurs in a way that results in a new history entry whose URI differs from the previous entry's only by its fragment identifier... why do you read it as being scripting-specific? History traversal happens for all kinds of reasons in the spec. e.g. typing in a new URI with a different frag ID triggers it; because it first invokes this:

   http://www.whatwg.org/specs/web-apps/current-work/#navigating

...whose step 2 goes to:

   http://www.whatwg.org/specs/web-apps/current-work/#scrolling0

...whose step 1 goes to:

   http://www.whatwg.org/specs/web-apps/current-work/#update

...whose step 4.3 goes to:

   http://www.whatwg.org/specs/web-apps/current-work/#traverse

...whose step 6 fires the event.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> why do you read it as being scripting-specific?

Because it's in the section that describes script-triggered history traversals, as far as I can tell?

That link chain just tells me that the spec text needs restructuring to make it clearer that we're not actually talking about history traversals but rather changes in what's loaded in the window.  It's possible for these to happen without changing the "current" history entry, no?  If nothing else replace loads (though the spec seems to be attempting to shoehorn them into the same model).

In any case, it seems to me that the spec severely restricts the possible history models browsers can use, which I find highly unfortunate: I think the current history setup is rather broken and we need something better; I'm just not sure what the something is.

Comment 6

[Hixie (not reading bugmail)]

The spec might well need restructuring. It's a work in progress.

The section in question doesn't define script-triggered history traversals, it just defines history traversals in general. It happens that this definition is currently right next to an invocation of that algorithm from a DOM API, but it's also used from the general navigation section.

As you say, right now history replacements are implemented in the spec as history traversals with certain flags set.

I'm not sure what is restricted exactly; if you have any proposals I'd love to hear about them (feel free to send them to the list(s) or to me). The idea is to allow basically any UI while still exposing the same underlying model to scripts. The spec goes out of its way to point out that any UI is conforming so long as it exposes a backwards-compatible model to the scripts. I agree that we don't want to constrain UI development.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I think the model is fundamentally broken.  And as I said, I don't have good suggestions for replacement models yet; otherwise I would be implementing one.

Comment 8

[Hixie (not reading bugmail)]

Even bad suggestions would be useful to see if the spec can handle different models. But I think we're always going to be a little constrained by what the underlying API has to expose for compatibility, probably.

Comment 10

[Bill Higgins]

FWIW, apparently IE 8 has implemented this feature, though I haven't had a chance to try it out yet:

http://ejohn.org/blog/javascript-in-internet-explorer-8/

Comment 11

[Varun Varada]

I don't know if this has been done here because I haven't looked through the complex code, but Mozilla itself seems to be using some kind of this event (because going to this page didn't kill my CPU) here:

http://en-us.www.mozilla.com/en-US/firefox/central/

I don't know, but it might be it. Try clicking on one of the tabs, and then changing the hash, and the tab instantly changes to the tab associated with that hash.

Comment 12

[kuvik3]

Is there any plan to implement this event in Firefox? Discussion about it seems to be old...

Comment 13

[Rob Retchless]

I can't believe this bug isn't higher on the priority list.  Until this is implemented, Ajax sites must continue to resort to polling loops to monitor the URL for changes.  This feature works great in IE8 and should be added to Firefox ASAP!

Also, the event is onhashchange not onhashchanged. Someone with power should rename the title of this bug :)

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Justin is working on this.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Proposed patch

Attaching a proposed patch for review.  A few notes:

* I don't have enough experience with the code to know for sure that when I dispatch to JS in nsDocShell::FireHashchange it's actually safe to do so, so that dispatch deserves particular scrutiny.

* nsEventListenerManager.cpp's sLoadEvents[] array appears to be dead code.  I'm not sure if I should remove it in this patch or leave it for another one.  (See http://mxr.mozilla.org/mozilla-central/search?string=sloadevents)

* This patch generated two warnings on the tryservers, but they both appear to be known issues: bug 498799, and bug 471647.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 385020 [details] [diff] [review]
Proposed patch

>diff -r 5f14dd5ec14a content/base/public/nsIContentUtilsExternal.idl
>+  boolean dispatchTrustedEvent(in nsIDocument aDoc,
>+                            in nsISupports aTarget,
>+                            in AString aEventName,
>+                            in boolean aCanBubble,
>+                            in boolean aCancelable);

Please fix indentation.

>diff -r 5f14dd5ec14a content/base/src/nsContentUtils.cpp
>+    { &nsGkAtoms::onhashchange,                  { NS_HASHCHANGE, EventNameType_HTMLXUL }},

This will make onhashchange appear on all elements. Not just body.

Technically that's true for a lot more events that we already implement, so if smaugs ok with it I'm fine with punting fixing that.

But it shouldn't be that hard to fix either.

>diff -r 5f14dd5ec14a docshell/test/file_bug385434_1.html
>diff -r 5f14dd5ec14a docshell/test/file_bug385434_2.html
>@@ -0,0 +1,20 @@
>+<!--
>+Inner frame for test of bug 385434.
>+https://bugzilla.mozilla.org/show_bug.cgi?id=385434
>+-->
>+<html>
>+<head>
>+  <script type="application/javascript">
>+    function hashchange(e) {
>+      parent.onHashchange();
>+
>+      // pass the event back to the parent so it can check its properties.
>+      parent.gSampleEvent = e;
>+    }

Why don't you need to set gSampleEvent first here? I suspect you're checking the the event before what you think you're checking :)

>+  </script>
>+</head>
>+
>+<frameset onhashchange="hashchange(event)">
>+  <frame src="http://lkjsdlfkjsdlfkjsdf.com" />
>+</frameset>

Use about:blank or simply don't define a src attribute at all.

>diff -r 5f14dd5ec14a docshell/test/test_bug385434.html
>+var gNumEvents = 0;
>+var gLastNumEvents = 0;

You can replace these two with a single 'gNumEventsExecuted' which you reset to 0 in noEventExpected/eventExpected.

>+// We sometimes have to wait for things which might take a while.  1000ms
>+// should be plenty of time.
>+var LONG_TIMEOUT = 1000;

use 'const' instead of 'var'. And declare this above where it's first used.

>+  // Set up history in the iframe which looks like:
>+  //   file_bug385434_1.html#link1
>+  //   www.(garbage).com
>+  //   file_bug385434_1.html#foo      <-- current page

make the middle frame navigate to "about:blank" instead.

>+  frameCw.window.location = "http://www.asdflkjsdlfkjsdfiusfsd.com";
>+  longWait();
>+  yield;

Don't do a "longWait", that can trigger random oranges if the navigation takes too long. Instead set frameCw.window.onload and wait for the load event to fire.

>+  frameCw.window.location = "file_bug385434_1.html#foo";
>+  longWait();
>+  yield;

Same here.

>+  // Now when we do history.go(-2) on the frame, it *shouldn't* fire a
>+  // hashchange.  Although the URIs differ only by their hashes, they belong to
>+  // two different Documents.
>+  frameCw.history.go(-2);
>+  noEventExpected("Moving between different Documents shouldn't " +
>+                  "trigger a hashchange.");
>+
>+  /*
>+   * TEST 2 tests that:
>+   *     <frame onhashchange = ... > works,
>+   *     the event's cancelable, bubbles settings are correct
>+   */

you mean frameset here, right?

>+  frameCw.document.location = "file_bug385434_2.html";
>+  longWait();
>+  yield;
>+  frameCw.document.location = "file_bug385434_2.html#foo";

wait for onload here too.

>+  /*
>+   * TEST 3 tests that:
>+   *     document.window.addEventListener() works for hashchange,
>+   *     hashchange is dispatched to window, not to body
>+   */
>+  var incNumEvents = function() { gNumEvents++; };
>+  window.addEventListener("hashchange", incNumEvents, false);
>+  window.location.hash = "#foo";
>+  eventExpected("addEventListener() should work for hashchange.");
>+  window.removeEventListener("hashchange", incNumEvents, false);
>+
>+  // Can't re-use |incNumEvents| after it's removed.
>+  var newIncNumEvents = function() { gNumEvents++; };
>+  document.body.addEventListener("hashchange", newIncNumEvents, false);
>+  window.location.hash = "#bar";
>+  noEventExpected("hashchange shouldn't be dispatched to <body>.");

Why can't you reuse incNumEvents?


>diff -r 5f14dd5ec14a layout/build/nsLayoutModule.cpp

>+   { "Focus Manager",
>+     NS_FOCUSMANAGER_CID,
>+     "@mozilla.org/focus-manager;1",
>+     CreateFocusManager },
> 
>-
>-  { "Focus Manager",
>-    NS_FOCUSMANAGER_CID,
>-    "@mozilla.org/focus-manager;1",
>-    CreateFocusManager },

If you're fixing the indentation, please fix it to be correct :)

Still need to check that we're firing the event when it's safe.

Comment 17

[Boris Zbarsky [:bzbarsky]]

>+var LONG_TIMEOUT = 1000;

Please don't have any in-test timeouts.  If you need to do X after Y, then listen for an event that tells you Y happened, and if that never fires the test harness will time you out after an appropriate amount of time.  1000ms may well be too short on a loaded N810, for example.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Parts of the tests do that, and I asked to get them fixed above.

However in some cases we want to test that an asynchronously firing event does *not* fire.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> we want to test that an asynchronously firing event does *not* fire.

And you can't hook into something that would happen either right before or right after that event?  Like onLocationChange, say?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Talked it over with bz on irc. Given that the timeout makes it possible that a test will accidentally *pass* rather than accidentally *fail*, he was ok with the current setup.

Alternatively, it would definitely be great if you could hook nsIWebProgress and listen to onLocationChange. Lets talk tomorrow.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, forgot to say, a condition for keeping things the way they are is that it's very clearly documented in the test.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Please use -p when creating patches. Add for example the following to your
.hgrc
[defaults]
diff = -U 8 -p


(In reply to comment #15)
> * I don't have enough experience with the code to know for sure that when I
> dispatch to JS in nsDocShell::FireHashchange it's actually safe to do so, so
> that dispatch deserves particular scrutiny.

If you aren't sure and don't have to dispatch event synchronously, use a script
runner or post a runnable to current thread.
We have even a helper for that; nsPLDOMEvent and its RunDOMEventWhenSafe and
PostDOMEvent methods.
But in this case things are a bit tricky, if we want to follow HTML 5
exactly. "6.11.9 History traversal" says the first dispatch and then scroll.
I wonder what should happen if the listener causes a new hashchange while
we're still processing the previous one. Per the spec we first fire one event,
then another, then scroll to the latter position and then to the first position.
Pretty strange. I sent a mail to what-wg list about this.

Note, HTML 5 says that the event should be dispatched only 
"if the Document's current document readiness is the string "complete".
I'm not sure why.
What behavior does the patch have?

> * nsEventListenerManager.cpp's sLoadEvents[] array appears to be dead code. 
> I'm not sure if I should remove it in this patch or leave it for another one. 
> (See http://mxr.mozilla.org/mozilla-central/search?string=sloadevents)
sLoadEvents is still used. See the macros below it. And for example
nsDOMParser is a nsIDOMLoadListener. However, nsIDOMLoadListener is deprecated and I'd like to get rid of it soon. Anyway, nothing to do with this bug.

(In reply to comment #16)
> This will make onhashchange appear on all elements. Not just body.
onhashchange attribute. Do we not want that?

> Technically that's true for a lot more events that we already implement, so if
> smaugs ok with it I'm fine with punting fixing that.
I'm ok with that, and clean up it later, when doing the same for other events.

Do we want to have .onhashchange property on (body) elements?

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > Technically that's true for a lot more events that we already implement, so if
> > smaugs ok with it I'm fine with punting fixing that.
> I'm ok with that, and clean up it later, when doing the same for other events.
> 
> Do we want to have .onhashchange property on (body) elements?

We do. Does the patch not implement that properly (or at least as properly as for other events)? Seems to work in the mochitest.

However Justin did run into some weirdness. Apparently setting body.onhashchange to null doesn't actually remove the listener from the window.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #23)
> We do. Does the patch not implement that properly (or at least as properly as
> for other events)? Seems to work in the mochitest.
Oops, silly me. I had pressed ESC or something when loading the patch :p

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

IE8 seems to dispatch hashchange after scrolling
http://mozilla.pettay.fi/moztests/hashchange.html

If we take that behavior, then FireHashchange() should be called probably
after ScrollIfAnchor() call, but before returning from
7814         if (wasAnchor) {
in nsDocShell.cpp

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

> If we take that behavior, then FireHashchange() should be called probably
> after ScrollIfAnchor() call, but before returning from
> 7814         if (wasAnchor) {
> in nsDocShell.cpp
So around
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?mark=7906#7906

Comment 27

[Boris Zbarsky [:bzbarsky]]

Is there a reason it needs to be synchronous, in that case?  I think the synchronous firing of events from docshell code we have now is a huge problem, and would prefer to not add more to it...

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Well, it depends on what kind of behavior we want when 
window.location is used.
I'll test what IE8 does in this case.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

IE8 fires the hashchange event asynchronously,
after setting window.location has returned.
Scroll event is fired before returning.
(Gecko and Opera fire scroll after returning from window.location, Webkit
apparently randomly before or after returning.)

http://mozilla.pettay.fi/moztests/hashchange_window_location.html

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

(Not related to this, but apparently also IE8 has the random scroll event firing.
very strange.)

Comment 31

[Justin Lebar (not reading bugmail)]

Thanks for the comments, everyone.

I'm working on a new version of the patch which corrects the issues sicking pointed out.  I'm also going to change the behavior to hew more closely to what IE8 already does, by dispatching the hashchange event asynchronously, after the scroll event.

One issue I haven't addressed yet is from #22:
> Note, HTML 5 says that the event should be dispatched only 
> "if the Document's current document readiness is the string "complete".
> I'm not sure why.
> What behavior does the patch have?
My patch ignores the document rediness right now.  I need to check and see what IE8 does in this case; my guess is that IE8 ignores this property, so we'll end up ignoring it too.

Comment 32

[Justin Lebar (not reading bugmail)]

It appears that IE does respect the document rediness part of the spec, so I guess we'll have to as well.  See http://stanford.edu/~jlebar/moz/hashchange-ie.html

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

The spec has been changed. Now the event should be dispatched asynchronously.
http://www.whatwg.org/specs/web-apps/current-work/#history-traversal
So we should try to do whatever IE8 does.

Comment 34

[Hixie (not reading bugmail)]

Please let me know if IE8 doesn't match the spec in some way.

Comment 35

[Justin Lebar (not reading bugmail)]

Attachment: Second version of patch

Fixes problems sicking pointed out in comment 16.  Also changes behavior so it matches IE8 (and the modified HTML5 spec): hashchange is dispatched asynchronously, and after scrolling.

Setting |body.onhashchange = foo| still doesn't work.  This is a more general problem, bug 500253.

On the tryserver, I get only one warning, on OSX.  It's been observed before, so it appears to be unrelated to this patch (see bug 501221).

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 385862 [details] [diff] [review]
Second version of patch

> NS_IMETHODIMP
> nsDocShell::ScrollIfAnchor(nsIURI * aURI, PRBool * aWasAnchor,
>-                           PRUint32 aLoadType, nscoord *cx, nscoord *cy)
>+                           PRUint32 aLoadType, nscoord *cx, nscoord *cy,
>+                           PRBool * aDoHashchange)

While you're here, could you make this method to return nsresult.
No need for NS_IMETHOD in .h either, as far as I see.
>+    // Get a handle on the docshell's outer window
>+    nsCOMPtr<nsPIDOMWindow> outerWindow = do_QueryInterface(mScriptGlobal, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // We'd like to use nsContentUtils to dispatch the event, but we can't
>+    // reference it here because it's declared in a different library.
>+    // Instead, we get a handle on nsIContentUtilsExternal, which wraps
>+    // nsContentUtils.
>+    nsCOMPtr<nsIContentUtilsExternal> contentUtils =
>+        do_GetService("@mozilla.org/content-utils-external;1", &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Get a handle on the document, which we need to specify when we dispatch
>+    // the event.
>+    nsCOMPtr<nsIDocumentViewer> docViewer
>+        (do_QueryInterface(mContentViewer, &rv));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIDocument> document;
>+    docViewer->GetDocument(getter_AddRefs(document));
>+    NS_ENSURE_TRUE(document, NS_ERROR_FAILURE);
You can get document from DocShell using do_QueryInterface

>+
>+    if (! readyState.Equals(NS_LITERAL_STRING("complete")))
Extra space after !

What happens, if before the event has fired, the docshell starts to load
a new page? I guess if the new page hasn't loaded yet, window.location points still to the 
right place and firing the event is ok.
But, if a new document is loaded, the event will use wrong window/document.
Not sure how to test this. Perhaps some iframe could be set to "about:blank" and then
hashchange listener could be added to its window.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

So even if there wasn't a testcase, I think the event should be dispatched only
if docshell's current document is the same as what it was when the runnable was
dispatched to the current thread.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> You can get document from DocShell using do_QueryInterface

As a matter of fact, you cannot.  You can get an nsIDOMDocument using do_GetInterface...  It might be good to have a private helper that returns an nsIDocument, and use it for both this code and the existing code in GetInterface that looks for an application cache container.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Well, in this case that doesn't matter. It is nsIDOMNSDocument which is actually needed, so the document will be QI'ed.

Comment 40

[Justin Lebar (not reading bugmail)]

re comment 36:
> Not sure how to test this. Perhaps some iframe could be set to "about:blank"
> and then hashchange listener could be added to its window.

Could you elaborate on this?  I haven't been able to develop a testcase which triggers this failing behavior.  Here's what I have right now:

<script>
    window.addEventListener("load", function() {
      println("load");
      document.location.hash = "foo";
      document.location = "a_different_page.html";
    }, false);

    window.addEventListener("hashchange", function() {
      println("hashchange");
    }, false);
</script>
...

This test prints "load", then "hashchange", *then* indicates that it's loaded "a_different_page.html".  I'm not sure how I might be able to get the load to fire before the hashchange.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

a_different_page.html loads probably asynchronously, but try about:blank, or if that doesn't work use some page which can be bfcached, and load it from cache
history.go(1);

Comment 42

[Boris Zbarsky [:bzbarsky]]

All loads are asynchronous.  That includes about:blank and history.go().

A more interesting test might be document.open() wiping out the previous document.  That happens synchronously.

Another interesting test might be to start the about:blank load _before_ doing the location.hash set.

Comment 43

[Justin Lebar (not reading bugmail)]

To solve the problem in comment 36, that hashchange events might be dispatched to the wrong document, sicking suggested that we do the following:
 * Keep a nsTArray<nsComPtr<nsIRunnable> > of all outstanding hashchange events
 * When the docshell's document changes, iterate over this list, removing all events which have fired, and revoking all events which haven't yet fired.

Alternatively, the hashchange event could keep a weak reference to the document it was fired on.  Upon running, it could check to see that the docshell's document is the same as the reference it has stored.  If it's not, it doesn't dispatch the event.

We preferred the first approach because there's a race condition with the second: We might trigger a hashchange on page A, immediately navigate to page B, then navigate back to page A.  If we do this quickly enough, the hashchange might still fire on A.  With the first approach, our behavior is consistent.  (Granted, in either case, we're still dealing with a race condition on the initial hashchange dispatch versus the navigation to B.)

I wanted to get some comments before implementing this.  Does the first approach seem reasonable?

Comment 44

[Justin Lebar (not reading bugmail)]

Re comment 42:
> A more interesting test might be document.open() wiping out the previous
> document.  That happens synchronously.
I think I got the test to fail this way.  I'll verify that it's failing for the right reason once I implement the fix.  :)

Comment 45

[Boris Zbarsky [:bzbarsky]]

Note that neither of the two approaches in comment 43 would affect the hashchange event firing on document.open.  What is the exact behavior we want in that case?

Should we just cancel pending hashchange events if the docshell's stop() is called, perhaps?

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

document.open() doesn't change the Document object though (only changes what's inside it), so i'm not sure it should prevent hashchange from firing.

Comment 47

[Boris Zbarsky [:bzbarsky]]

In gecko it changes the document URI (not sure what it does per current HTML5 draft), so it's not clear that the event should in fact fire...

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

Perhaps inner window could fire the event. DocShell would just call some new
method in nsPIDOMWindow and then the inner window would fire the event asynchronously. If the window gets deleted before the event fires, 
the event is canceled. And the events should fire if the window becomes frozen.
This approach should simplify the code and there wouldn't be need for nsIContentUtilsExternal.

Comment 49

[Justin Lebar (not reading bugmail)]

I just modified the code per comment 48, and it works great.  It even keeps hashchange from firing after a document.open(), although I'm now not sure if that's the right behavior.  :)

Should I be testing other edge cases to make sure hashchange is fired only on the correct document?

Comment 50

[Justin Lebar (not reading bugmail)]

Attachment: Third version of hashchange patch

Third version of hashchange patch.  Moved event dispatching code into inner window, got rid of ContentUtilsExternal, added test with document.open().

Comment 51

[Justin Lebar (not reading bugmail)]

Attachment: Fourth version of hashchange patch

Minor cleanup of previous patch.  Sorry for the double-submission.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

>diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h
>--- a/content/base/public/nsIDocument.h
>+++ b/content/base/public/nsIDocument.h
>@@ -630,16 +630,17 @@ public:
You're changing the interface so update the IID

>+  virtual nsresult GetReadyStateEnum(ReadyState *aResult) = 0;
This could be 
virtual ReadyState GetReadyStateEnum() = 0

>+            if (doHashchange) {
>+                nsCOMPtr<nsPIDOMWindow> outerWindow =
>+                    do_QueryInterface(mScriptGlobal, &rv);
>+                NS_ENSURE_SUCCESS(rv, rv);
>+                
>+                nsCOMPtr<nsPIDOMWindow> innerWindow =
>+                    outerWindow->GetCurrentInnerWindow();
>+
>+                rv = innerWindow->DispatchAsyncHashchange();
>+                NS_ENSURE_SUCCESS(rv, rv);
>+            }
>+
I this could be just:
nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mScriptGlobal);
if (window) {
  window->DispatchAsyncHashchange();
}
return NS_OK;


+nsresult
+nsGlobalWindow::DispatchAsyncHashchange()
+{
+  NS_ENSURE_TRUE(IsInnerWindow(), NS_ERROR_FAILURE);
Replace NS_ENSURE_TRUE(...) with FORWARD_TO_INNER(FireHashchange, (), NS_OK);

>+nsresult
>+nsGlobalWindow::FireHashchange()
{
You should check here if the window is frozen and not dispatch in that case.

>+  nsCOMPtr<nsPIDOMWindow> outerWindow = GetOuterWindow();
>+
>+  // Dispatch the hashchange event, which doesn't bubble and isn't cancelable,
>+  // to the outer window.
>+  return nsContentUtils::DispatchTrustedEvent(mDoc, outerWindow,
>+                                              NS_LITERAL_STRING("hashchange"),
>+                                              PR_FALSE, PR_FALSE);
This should work too:
  return nsContentUtils::DispatchTrustedEvent(mDoc, GetOuterWindow(),
                                              NS_LITERAL_STRING("hashchange"),
                                              PR_FALSE, PR_FALSE);

Update the IID of nsPIDOMWindow

With those, r=me

Comment 53

[Justin Lebar (not reading bugmail)]

Attachment: ContentUtilsExternal code

In case it's needed in the future, here's a patch which adds ns(I)ContentUtilsExternal.

Comment 54

[Justin Lebar (not reading bugmail)]

Attachment: Fifth version of hashchange patch

Addressing smaug's comments.  Forwarding review from last patch.

Comment 55

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 386398 [details] [diff] [review]
Fifth version of hashchange patch

>+nsresult
> nsDocShell::ScrollIfAnchor(nsIURI * aURI, PRBool * aWasAnchor,
>-                           PRUint32 aLoadType, nscoord *cx, nscoord *cy)
>+                           PRUint32 aLoadType, nscoord *cx, nscoord *cy,
>+                           PRBool * aDoHashchange)
> {
>     NS_ASSERTION(aURI, "null uri arg");
>     NS_ASSERTION(aWasAnchor, "null anchor arg");
>-
>-    if (aURI == nsnull || aWasAnchor == nsnull) {
>+    NS_ASSERTION(aDoHashchange, "null hashchange arg");
>+
>+    if (!aURI || !aWasAnchor || !aDoHashchange) {
>         return NS_ERROR_FAILURE;
>     }

Don't actually deal with aDoHashchange being null. Just crash instead. Patterns like the above will just lead to people passing in null for no good reason.

In general it's discouraged to assert as well as dealing. Either something is ok, in which case we shouldn't assert, or it's not ok in which case we shouldn't bother with dealing.

Also, assertions at the start of a function like that should use NS_PRECONDITION instead of NS_ASSERT. Doesn't make a big difference, but it's the RightThing to do :)

sr=me with that fixed!!

Comment 56

[Justin Lebar (not reading bugmail)]

Attachment: Sixth version of hashchange patch

Forwarding smaug's r+ and sicking's sr+ to this patch.

Comment 57

[Justin Lebar (not reading bugmail)]

Attachment: Seventh version of patch

Patch for checkin.

Comment 58

[Justin Lebar (not reading bugmail)]

Fixed in changeset da57f50c3be8: http://hg.mozilla.org/mozilla-central/rev/da57f50c3be8

Comment 59

[Fabian (Crash) Grutschus]

https://developer.mozilla.org/en/DOM/window.onhashchange

Comment 60

[Justin Lebar (not reading bugmail)]

Unsetting "wanted 1.9.2 ?" because this is already in 1.9.2.