[webkit-reviews] review granted: [Bug 36201] hashchange event should be dispatched asynchronously : [Attachment 51114] Lay the groundwork for fixing popstate, pageshow, and hashchange in similar and simple manners.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 18 17:58:10 PDT 2010
Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 36201: hashchange event should be dispatched asynchronously
https://bugs.webkit.org/show_bug.cgi?id=36201
Attachment 51114: Lay the groundwork for fixing popstate, pageshow, and
hashchange in similar and simple manners.
https://bugs.webkit.org/attachment.cgi?id=51114&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> 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().pagesh
owEvent, 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.
More information about the webkit-reviews
mailing list