Bug 36201 - hashchange event should be dispatched asynchronously
: hashchange event should be dispatched asynchronously
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Brady Eidson
: InRadar
Depends on:
Blocks: 38754
  Show dependency treegraph
 
Reported: 2010-03-16 16:49 PDT by Darin Fisher (:fishd, Google)
Modified: 2010-05-10 19:20 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2010-03-16 16:49:28 PDT
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 Darin Fisher (:fishd, Google) 2010-03-16 16:50:02 PDT
Related to this, the HashChangeEvent now has additional fields:
http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#hashchangeevent
Comment 2 Brady Eidson 2010-03-16 16:53:08 PDT
<rdar://problem/7761278>
Comment 3 Brady Eidson 2010-03-18 16:52:54 PDT
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 Brady Eidson 2010-03-18 17:27:03 PDT
Created attachment 51114 [details]
Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners.
Comment 5 Darin Adler 2010-03-18 17:58:10 PDT
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.
Comment 6 Brady Eidson 2010-03-19 09:00:04 PDT
(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.
Comment 7 Brady Eidson 2010-03-19 09:05:30 PDT
> > 
> > > -    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 Brady Eidson 2010-03-19 11:46:08 PDT
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 Alexey Proskuryakov 2010-03-19 11:52:05 PDT
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 Eric Seidel 2010-03-19 11:57:55 PDT
Looks like this broke the chromium compile?
Comment 11 Eric Seidel 2010-03-19 11:58:31 PDT
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 Darin Adler 2010-03-19 12:01:35 PDT
Just need to change callers to call enqueueEvent instead of enqueueStorageEvent.
Comment 13 Dimitri Glazkov (Google) 2010-03-19 13:10:25 PDT
Chromium build fixed in http://trac.webkit.org/changeset/56258.
Comment 14 Eric Seidel 2010-03-24 14:32:44 PDT
Attachment 51114 [details] was posted by a committer and has review+, assigning to Brady Eidson for commit.
Comment 15 Eric Seidel 2010-03-24 20:24:35 PDT
Looks like this can be closed, no?
Comment 16 Darin Fisher (:fishd, Google) 2010-03-24 21:14:57 PDT
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 Eric Seidel 2010-03-24 21:51:36 PDT
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.
Comment 18 Steven Lai 2010-04-28 15:28:20 PDT
Created attachment 54630 [details]
Patch

Revert hashchange to fire asynchronously
Comment 19 Brady Eidson 2010-04-28 16:01:02 PDT
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.
Comment 20 Steven Lai 2010-04-28 17:45:24 PDT
Just re-ran layout tests on r58441
The change didn't affect any existing layout tests.
Comment 21 Brady Eidson 2010-04-28 17:49:38 PDT
Okay, I stand by my r+ then!
Comment 22 Brady Eidson 2010-04-28 17:49:39 PDT
Okay, I stand by my r+ then!
Comment 23 Darin Fisher (:fishd, Google) 2010-04-28 21:15:15 PDT
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 :)
Comment 24 Steven Lai 2010-04-30 14:16:23 PDT
It used to be async on before r51644

Please see:
http://trac.webkit.org/changeset/51644
Comment 25 Darin Fisher (:fishd, Google) 2010-04-30 23:34:06 PDT
(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 Steven Lai 2010-05-03 15:39:31 PDT
Hi,
Just want to ask why it's cq-?
Comment 27 WebKit Commit Bot 2010-05-03 23:52:42 PDT
Comment on attachment 54630 [details]
Patch

Clearing flags on attachment: 54630

Committed r58736: <http://trac.webkit.org/changeset/58736>
Comment 28 WebKit Commit Bot 2010-05-03 23:52:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 WebKit Review Bot 2010-05-04 00:15:02 PDT
http://trac.webkit.org/changeset/58736 might have broken Qt Linux Release
Comment 30 Steven Lai 2010-05-04 02:20:14 PDT
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 Brady Eidson 2010-05-04 09:05:08 PDT
(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 Brady Eidson 2010-05-04 09:07:55 PDT
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 Steven Lai 2010-05-04 10:16:02 PDT
oops
i see!
Comment 34 Eric Seidel 2010-05-07 09:12:55 PDT
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 Eric Seidel 2010-05-07 09:16:38 PDT
Filed bug 38754 about the new test failure.
Comment 36 Steven Lai 2010-05-10 19:20:00 PDT
submitted patch to https://bugs.webkit.org/show_bug.cgi?id=36335 to fix the failure without disabling the test case.