Summary: | popstate event should be dispatched asynchronously | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Fisher (:fishd, Google) <fishd> | ||||||
Component: | Page Loading | Assignee: | Mihai Parparita <mihaip> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aestes, ap, beidson, commit-queue, eric, esprehn+autocc, jhoneycutt, kangil.han, mihaip | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=150202 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 153686 | ||||||||
Attachments: |
|
Description
Darin Fisher (:fishd, Google)
2010-03-16 16:51:24 PDT
Created attachment 81647 [details]
Patch
Darin, let me know if the change to FrameLoaderClientImpl::pluginLoadObserver can be tested. Comment on attachment 81647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81647&action=review > Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:-1548 > - // We can arrive here if a popstate event handler detaches this frame. To test this, I think you'd need to modify the NPAPI test plugin. This patch actually fixes the bug where we would fail to notify a plugin of when a frame finishes navigating. You could have a test, where a plugin uses NPN_GetURLNotify w/ target equal to the name of a frame. It expects to receive a NPP_URLNotify call once the frame navigation completes. I'm not sure if TestNetscapePlugin has any tests like this already. Attachment 81647 [details] was posted by a committer and has review+, assigning to Mihai Parparita for commit.
Created attachment 265347 [details]
Patch
Comment on attachment 265347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265347&action=review Looks great! R=me. > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=36202 I think there might be a radar for this. Can you add it, please? > Source/WebCore/ChangeLog:6 > + Based on an original patch by Mihai Parparita <mihaip@chromium.org>. Please reference the CRBugs entry, if available. (In reply to comment #7) > Comment on attachment 265347 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265347&action=review > > Looks great! R=me. > > > Source/WebCore/ChangeLog:4 > > + https://bugs.webkit.org/show_bug.cgi?id=36202 > > I think there might be a radar for this. Can you add it, please? Fixed. > > > Source/WebCore/ChangeLog:6 > > + Based on an original patch by Mihai Parparita <mihaip@chromium.org>. > > Please reference the CRBugs entry, if available. The original patch came from this bug, not a CR. Thanks for the review! I've pinged Brady to take a look at this as well before I land it. Committed r192369: <http://trac.webkit.org/changeset/192369> I don't think this fix is right. The spec says to dispatch the event asynchronously only when the "asynchronous events" flag is set. This flag is only set is during navigation to a fragment identifier, afaict. For cases like history.back(), we still want to dispatch the event synchronously (which makes sense, because the history navigation itself happens asynchronously). (In reply to comment #10) > I don't think this fix is right. The spec says to dispatch the event > asynchronously only when the "asynchronous events" flag is set. This flag is > only set is during navigation to a fragment identifier, afaict. For cases > like history.back(), we still want to dispatch the event synchronously > (which makes sense, because the history navigation itself happens > asynchronously). Reading the spec, yup you're right. (In reply to comment #11) > (In reply to comment #10) > > I don't think this fix is right. The spec says to dispatch the event > > asynchronously only when the "asynchronous events" flag is set. This flag is > > only set is during navigation to a fragment identifier, afaict. For cases > > like history.back(), we still want to dispatch the event synchronously > > (which makes sense, because the history navigation itself happens > > asynchronously). > > Reading the spec, yup you're right. I filed https://bugs.webkit.org/show_bug.cgi?id=153686 |