Bug 36201 - hashchange event should be dispatched asynchronously
: hashchange event should be dispatched asynchronously
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
: 38754
  Show dependency treegraph
 
Reported: 2010-03-16 16:49 PST by
Modified: 2010-05-10 19:20 PST (History)


Attachments
Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners. (8.51 KB, patch)
2010-03-18 17:27 PST, Brady Eidson
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.37 KB, patch)
2010-04-28 15:28 PST, Steven Lai
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-03-16 16:49:28 PST
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
------- Comment #1 From 2010-03-16 16:50:02 PST -------
Related to this, the HashChangeEvent now has additional fields:
http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#hashchangeevent
------- Comment #2 From 2010-03-16 16:53:08 PST -------
<rdar://problem/7761278>
------- Comment #3 From 2010-03-18 16:52:54 PST -------
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...
------- Comment #4 From 2010-03-18 17:27:03 PST -------
Created an attachment (id=51114) [details]
Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners.
------- Comment #5 From 2010-03-18 17:58:10 PST -------
(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?

> +    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.
------- Comment #6 From 2010-03-19 09:00:04 PST -------
(In reply to comment #5)
> (From update of attachment 51114 [details] [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.
------- Comment #7 From 2010-03-19 09:05:30 PST -------
> > 
> > > -    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.
------- Comment #8 From 2010-03-19 11:46:08 PST -------
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.
------- Comment #9 From 2010-03-19 11:52:05 PST -------
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.
------- Comment #10 From 2010-03-19 11:57:55 PST -------
Looks like this broke the chromium compile?
------- Comment #11 From 2010-03-19 11:58:31 PST -------
sheriffbot> bradee-oh, darin: r56249 appears to have broken Chromium Mac Release, Chromium Linux Release

Not sure why the EWS didn't catch it.
------- Comment #12 From 2010-03-19 12:01:35 PST -------
Just need to change callers to call enqueueEvent instead of enqueueStorageEvent.
------- Comment #13 From 2010-03-19 13:10:25 PST -------
Chromium build fixed in http://trac.webkit.org/changeset/56258.
------- Comment #14 From 2010-03-24 14:32:44 PST -------
Attachment 51114 [details] was posted by a committer and has review+, assigning to Brady Eidson for commit.
------- Comment #15 From 2010-03-24 20:24:35 PST -------
Looks like this can be closed, no?
------- Comment #16 From 2010-03-24 21:14:57 PST -------
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 #17 From 2010-03-24 21:51:36 PST -------
(From update of attachment 51114 [details])
Ok.  Clearing the flags and obsoleting this landed patch.
------- Comment #18 From 2010-04-28 15:28:20 PST -------
Created an attachment (id=54630) [details]
Patch

Revert hashchange to fire asynchronously
------- Comment #19 From 2010-04-28 16:01:02 PST -------
(From update of attachment 54630 [details])
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.
------- Comment #20 From 2010-04-28 17:45:24 PST -------
Just re-ran layout tests on r58441
The change didn't affect any existing layout tests.
------- Comment #21 From 2010-04-28 17:49:38 PST -------
Okay, I stand by my r+ then!
------- Comment #22 From 2010-04-28 17:49:39 PST -------
Okay, I stand by my r+ then!
------- Comment #23 From 2010-04-28 21:15:15 PST -------
(From update of attachment 54630 [details])
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 :)
------- Comment #24 From 2010-04-30 14:16:23 PST -------
It used to be async on before r51644

Please see:
http://trac.webkit.org/changeset/51644
------- Comment #25 From 2010-04-30 23:34:06 PST -------
(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!
------- Comment #26 From 2010-05-03 15:39:31 PST -------
Hi,
Just want to ask why it's cq-?
------- Comment #27 From 2010-05-03 23:52:42 PST -------
(From update of attachment 54630 [details])
Clearing flags on attachment: 54630

Committed r58736: <http://trac.webkit.org/changeset/58736>
------- Comment #28 From 2010-05-03 23:52:51 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #29 From 2010-05-04 00:15:02 PST -------
http://trac.webkit.org/changeset/58736 might have broken Qt Linux Release
------- Comment #30 From 2010-05-04 02:20:14 PST -------
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).
------- Comment #31 From 2010-05-04 09:05:08 PST -------
(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.
------- Comment #32 From 2010-05-04 09:07:55 PST -------
Also note that the FIXME in the code already references the bug for that  :)

https://bugs.webkit.org/show_bug.cgi?id=36335
------- Comment #33 From 2010-05-04 10:16:02 PST -------
oops
i see!
------- Comment #34 From 2010-05-07 09:12:55 PST -------
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?
------- Comment #35 From 2010-05-07 09:16:38 PST -------
Filed bug 38754 about the new test failure.
------- Comment #36 From 2010-05-10 19:20:00 PST -------
submitted patch to https://bugs.webkit.org/show_bug.cgi?id=36335 to fix the failure without disabling the test case.