hashchange event should be dispatched asynchronously see step #11 of the history traversal algorithm (section 6.5.9): http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#traverse-the-history this is a recent change to the spec
Related to this, the HashChangeEvent now has additional fields: http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#hashchangeevent
<rdar://problem/7761278>
There's 3 interesting new loading events in the spec where we don't match: hashchange, popstate, and pageshow. In exploring these 3 closely related events, there's 4 quite relevant bugzillas, including this one: https://bugs.webkit.org/show_bug.cgi?id=36201 https://bugs.webkit.org/show_bug.cgi?id=36202 https://bugs.webkit.org/show_bug.cgi?id=36334 https://bugs.webkit.org/show_bug.cgi?id=36335 While I plan to resolve each of these with its own patch, I'll to lay the groundwork for all 4 to easily be resolved with a single zero-change-in-behavior patch here. It will be touching Document.h, so its better to get it out of the way once. ;) Stay tuned here...
Created attachment 51114 [details] Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners.
Comment on attachment 51114 [details] Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners. > void Document::enqueueStorageEvent(PassRefPtr<Event> storageEvent) It seems that you could rename this function to be non-storage-event-specific as part of this "no behavior change" step. Is there a reason you didn't? > + void schedulePageshowEvent(bool persisted); > + void scheduleHashchangeEvent(const String& oldURL, const String& newURL); > + void schedulePopstateEvent(PassRefPtr<SerializedScriptValue> stateObject); It's a little strange to use schedule for all these new functions, but enqueue for the old function. > - m_document->dispatchWindowEvent(PageTransitionEvent::create(eventNames().pageshowEvent, true), m_document); > + m_document->schedulePageshowEvent(true); Boolean arguments where we pass constnat valuesstink. And what in the world is "persisted"? > + KURL oldURL; > bool hashChange = equalIgnoringFragmentIdentifier(url, m_URL) && url.fragmentIdentifier() != m_URL.fragmentIdentifier(); > + if (hashChange) > + oldURL = m_URL; If all we need for the old URL is a string, it's more efficient to save it in a String local variable instead of KURL. I don't think we should bother with the "if" statement here -- it's OK to unconditionally save the old URL since it will just be a bump to a reference count. Can you end up getting a lot of hashchange events all in a row? Is it OK to get a hashchange event, later, after the URL is no longer the same at all? I’ll say r=me but there is room for improvement here.
(In reply to comment #5) > (From update of attachment 51114 [details]) > > void Document::enqueueStorageEvent(PassRefPtr<Event> storageEvent) > > It seems that you could rename this function to be non-storage-event-specific > as part of this "no behavior change" step. Is there a reason you didn't? Good call - I'll just change it to "enqueueEvent" > > > + void schedulePageshowEvent(bool persisted); > > + void scheduleHashchangeEvent(const String& oldURL, const String& newURL); > > > + void schedulePopstateEvent(PassRefPtr<SerializedScriptValue> stateObject); > > It's a little strange to use schedule for all these new functions, but enqueue > for the old function. You're right. > > > - m_document->dispatchWindowEvent(PageTransitionEvent::create(eventNames().pageshowEvent, true), m_document); > > + m_document->schedulePageshowEvent(true); > > Boolean arguments where we pass constnat valuesstink. And what in the world is > "persisted"? "persisted" is the official name of the field in the PageTransitionEvent that tells you whether this is a fresh page load or a page cache load, and it is defined to be a bool. In this same patch I also call schedulePageShowEvent(false). I'll make an enum for this for clarities sake, anyways. > > > + KURL oldURL; > > bool hashChange = equalIgnoringFragmentIdentifier(url, m_URL) && url.fragmentIdentifier() != m_URL.fragmentIdentifier(); > > + if (hashChange) > > + oldURL = m_URL; > > If all we need for the old URL is a string, it's more efficient to save it in a > String local variable instead of KURL. I don't think we should bother with the > "if" statement here -- it's OK to unconditionally save the old URL since it > will just be a bump to a reference count. Good point, will change. > > Can you end up getting a lot of hashchange events all in a row? Currently, no. Once they're asynchronous, yes. >Is it OK to get a hashchange event, later, after the URL is no longer the same at all? That's what the new fields in the hashchange event are about - to make sure the event captures the before and after url so script knows what the change was even though it happened "in the past" > > I’ll say r=me but there is room for improvement here. I'll address your feedback first! Thanks for the review.
> > > > > - m_document->dispatchWindowEvent(PageTransitionEvent::create(eventNames().pageshowEvent, true), m_document); > > > + m_document->schedulePageshowEvent(true); > > > > Boolean arguments where we pass constnat valuesstink. And what in the world is > > "persisted"? > > "persisted" is the official name of the field in the PageTransitionEvent that > tells you whether this is a fresh page load or a page cache load, and it is > defined to be a bool. In this same patch I also call > schedulePageShowEvent(false). > > I'll make an enum for this for clarities sake, anyways. > After digging in to this one, the PageTransitionEvent itself is too hard-wired into using this bool, and has to remain as such for the purposes of the DOM/IDL/etc. I'm adding an enum to Document.h specifically for the Pageshow event helper that will keep clarity there.
Landed the groundwork in http://trac.webkit.org/changeset/56249 Now resolving this bug, bug 36202, and bug 36334 should all be very simple, but I won't have time to tackle it for a few days.
We use enqueueEvent sometimes, and postTask other times. I talked to Brady about standardizing on one mechanism, and also about the fact that we needn't be using it whenever a specification says so. In most cases, events to be posted are triggered by network loading, which is itself asynchronous, so the behavior is indistinguishable. Since these particular events can be dispatched during synchronous operations, we do need to post them in order to match the spec.
Looks like this broke the chromium compile?
sheriffbot> bradee-oh, darin: r56249 appears to have broken Chromium Mac Release, Chromium Linux Release Not sure why the EWS didn't catch it.
Just need to change callers to call enqueueEvent instead of enqueueStorageEvent.
Chromium build fixed in http://trac.webkit.org/changeset/56258.
Attachment 51114 [details] was posted by a committer and has review+, assigning to Brady Eidson for commit.
Looks like this can be closed, no?
No, this bug is not fixed. Please see comment #8. Brady only landed the groundwork for fixing this bug. There is still a follow-up patch that is required.
Comment on attachment 51114 [details] Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners. Ok. Clearing the flags and obsoleting this landed patch.
Created attachment 54630 [details] Patch Revert hashchange to fire asynchronously
Comment on attachment 54630 [details] Patch Thanks for doing this! One question though - I'm kind of surprised that no existing layouttest fails with this change. Did we really not need to update any previous results? r+ if that's the case.
Just re-ran layout tests on r58441 The change didn't affect any existing layout tests.
Okay, I stand by my r+ then!
Comment on attachment 54630 [details] Patch WebCore/ChangeLog:5 + Reverted hashchange() event back to async. Is this description of the change really correct? I didn't think hashchange was ever async before. I would have expected this to read: "Change hashchange() event to be async." or something like that :)
It used to be async on before r51644 Please see: http://trac.webkit.org/changeset/51644
(In reply to comment #24) > It used to be async on before r51644 > > Please see: > http://trac.webkit.org/changeset/51644 Ah, I see. Sorry for the spam then!
Hi, Just want to ask why it's cq-?
Comment on attachment 54630 [details] Patch Clearing flags on attachment: 54630 Committed r58736: <http://trac.webkit.org/changeset/58736>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/58736 might have broken Qt Linux Release
Should reopen this bug because the patch doesn't update HashChangeEvent to its new proposed interface (i.e the arguments for old hash and the new hash in the event callback).
(In reply to comment #30) > Should reopen this bug because the patch doesn't update HashChangeEvent to its > new proposed interface (i.e the arguments for old hash and the new hash in the > event callback). This bug is about the hash change event being dispatched asynchronously or not. If other changes need to be made to bring the event up to spec, please file separate bugzillas for those changes, as they are logically separate tasks.
Also note that the FIXME in the code already references the bug for that :) https://bugs.webkit.org/show_bug.cgi?id=36335
oops i see!
I believe this caused: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r58943%20(5683)/fast/loader/stateobjects/document-destroyed-navigate-back-with-fragment-scroll-pretty-diff.html to start being flaky on Gtk, and probably all bots. The test probably doesn't want to print the hashChanged event? Or certainly not depend on its firing order in relation to other events?
Filed bug 38754 about the new test failure.
submitted patch to https://bugs.webkit.org/show_bug.cgi?id=36335 to fix the failure without disabling the test case.