[Webkit-unassigned] [Bug 36201] hashchange event should be dispatched asynchronously
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 19 09:00:04 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=36201
--- Comment #6 from Brady Eidson <beidson at apple.com> 2010-03-19 09:00:04 PST ---
(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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list