Move asynchronous event dispatching out of Document
Created attachment 74355 [details] Patch
This breaks out the EventQueue class out of bug 45631, so that Ryosuke can use it.
Comment on attachment 74355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74355&action=review Thanks for doing this! > WebCore/WebCore.xcodeproj/project.pbxproj:2656 > + 8F67561B1288B17B0047ACA3 /* EventQueue.h in Headers */ = {isa = PBXBuildFile; fileRef = 8F6756191288B17B0047ACA3 /* EventQueue.h */; }; You probably want to make this private. i.e. settings = {ATTRIBUTES = (Private, ); }; > WebCore/WebCore.xcodeproj/project.pbxproj:18435 > + 8F6756191288B17B0047ACA3 /* EventQueue.h */, > + 8F67561A1288B17B0047ACA3 /* EventQueue.cpp */, Please put this in the lexicologically correct location. > WebCore/WebCore.xcodeproj/project.pbxproj:21372 > + 8F67561B1288B17B0047ACA3 /* EventQueue.h in Headers */, Ditto. > WebCore/WebCore.xcodeproj/project.pbxproj:23942 > + 8F67561C1288B17B0047ACA3 /* EventQueue.cpp in Sources */, Ditto. > WebCore/dom/EventQueue.cpp:67 > + if (!eventTarget->toNode()->inDocument()) > + return; I'm not sure why you're doing this. Why shouldn't we fire events when the target node is not in the document? > WebCore/dom/EventQueue.h:39 > +class EventQueue { Should this inherit from Noncopyable?
(In reply to comment #3) > You probably want to make this private. i.e. settings = {ATTRIBUTES = (Private, ); }; Why? Despite the name, "private" actually exposes the header outside of the project. The default visibility ("project") is better unless there's a need for EventQueue to be used outside of WebCore. Other headers nearby (e.g. EventContext.h) use "project" visibility too. > > WebCore/WebCore.xcodeproj/project.pbxproj:18435 > > + 8F6756191288B17B0047ACA3 /* EventQueue.h */, > > + 8F67561A1288B17B0047ACA3 /* EventQueue.cpp */, > > Please put this in the lexicologically correct location. Done. > > WebCore/WebCore.xcodeproj/project.pbxproj:21372 > > + 8F67561B1288B17B0047ACA3 /* EventQueue.h in Headers */, > > Ditto. Done. > > WebCore/WebCore.xcodeproj/project.pbxproj:23942 > > + 8F67561C1288B17B0047ACA3 /* EventQueue.cpp in Sources */, > > Ditto. Done. > > > WebCore/dom/EventQueue.cpp:67 > > + if (!eventTarget->toNode()->inDocument()) > > + return; > > I'm not sure why you're doing this. Why shouldn't we fire events when the target node is not in the document? This was left over from the scrolling-related change, and isn't necessary for this refactoring. Removed. > > > WebCore/dom/EventQueue.h:39 > > +class EventQueue { > > Should this inherit from Noncopyable? Based on http://trac.webkit.org/changeset/68414, WTF_MAKE_NONCOPYABLE is actually the preferred way of doing this now.
Created attachment 74399 [details] Patch
Ojan may be a good reviewer too.
Comment on attachment 74399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74399&action=review > WebCore/dom/EventQueue.cpp:70 > + else if (eventTarget->toDOMWindow()) > + eventTarget->toDOMWindow()->dispatchEvent(event, 0); > + else > + ASSERT_NOT_REACHED(); This "else if" condition is unnecessary since you already asserted ASSERT(event->target()->toNode() || event->target()->toDOMWindow()); in enqueueEvent. You can just do: else { ASSERT(event->target()->toDOMWindow()); eventTarget->toDOMWindow()->dispatchEvent(event, 0); } It's not possible for the node or DOMWindow to go away since the target is stored as RefPtr in Event, right?
(In reply to comment #4) > (In reply to comment #3) > > You probably want to make this private. i.e. settings = {ATTRIBUTES = (Private, ); }; > > Why? Despite the name, "private" actually exposes the header outside of the project. The default visibility ("project") is better unless there's a need for EventQueue to be used outside of WebCore. Other headers nearby (e.g. EventContext.h) use "project" visibility too. Ok, that makes sense.
(In reply to comment #7) > It's not possible for the node or DOMWindow to go away since the target is stored as RefPtr in Event, right? It all depends what you mean by “go away”. The memory for the object won’t be reclaimed and the object won’t be deleted as long as we hold a reference. But the object can be in a state where it’s no longer reasonable to dispatch events to it. For example, we don’t want to dispatch events to elements in webpages in the back/forward cache, to name the trickiest example I can think of. Similarly, if we already sent an unload event to a webpage we don’t want to send other events to the elements on the page that are left over from earlier.
(In reply to comment #9) > It all depends what you mean by “go away”. The memory for the object won’t be reclaimed and the object won’t be deleted as long as we hold a reference. But the object can be in a state where it’s no longer reasonable to dispatch events to it. Right but calling toDOMWindow() doesn't help, does it? > For example, we don’t want to dispatch events to elements in webpages in the back/forward cache, to name the trickiest example I can think of. Similarly, if we already sent an unload event to a webpage we don’t want to send other events to the elements on the page that are left over from earlier. Wait, I thought this won't happen because of ActiveDOMObject? +ap
(In reply to comment #10) > (In reply to comment #9) > > It all depends what you mean by “go away”. The memory for the object won’t be reclaimed and the object won’t be deleted as long as we hold a reference. But the object can be in a state where it’s no longer reasonable to dispatch events to it. > > Right but calling toDOMWindow() doesn't help, does it? I don’t understand this remark. The function toDOMWindow is a good way to convert from an EventTarget* to a DOMWindow*. It has nothing to do with object lifetime; did someone say it does? > > For example, we don’t want to dispatch events to elements in webpages in the back/forward cache, to name the trickiest example I can think of. Similarly, if we already sent an unload event to a webpage we don’t want to send other events to the elements on the page that are left over from earlier. > > Wait, I thought this won't happen because of ActiveDOMObject? The ActiveDOMObject mechanism prevents some sorts of activity from occurring while a page is in the back/forward cache. But how does ActiveDOMObject prevent us from dispatching one of these queued events? Clearly it doesn’t.
(In reply to comment #11) > (In reply to comment #10) > > Right but calling toDOMWindow() doesn't help, does it? > > I don’t understand this remark. The function toDOMWindow is a good way to convert from an EventTarget* to a DOMWindow*. It has nothing to do with object lifetime; did someone say it does? Sorry, my point was to say this refactoring wouldn't worsen the problem. Am I right? > > > For example, we don’t want to dispatch events to elements in webpages in the back/forward cache, to name the trickiest example I can think of. Similarly, if we already sent an unload event to a webpage we don’t want to send other events to the elements on the page that are left over from earlier. > > > > Wait, I thought this won't happen because of ActiveDOMObject? > > The ActiveDOMObject mechanism prevents some sorts of activity from occurring while a page is in the back/forward cache. But how does ActiveDOMObject prevent us from dispatching one of these queued events? Clearly it doesn’t. I see. Maybe we should check that the document is still attached to a frame? (i.e. frame() != 0). But I'm not sure how that helps for DOMWindow.
(In reply to comment #11) > The ActiveDOMObject mechanism prevents some sorts of activity from occurring while a page is in the back/forward cache. But how does ActiveDOMObject prevent us from dispatching one of these queued events? Clearly it doesn’t. One more thing. I was going to suggest that we can add clear() to EventQueue so that we can remove all the pending events when we detach the document and or frame. What do you think of this idea?
> For example, we don’t want to dispatch events to elements in webpages in the back/forward cache This depends on event - for example, async DOM mutation events almost certainly should keep the document object alive and work regardless of whether the document is in frame, or ever was.
(In reply to comment #14) > > For example, we don’t want to dispatch events to elements in webpages in the back/forward cache > > This depends on event - for example, async DOM mutation events almost certainly should keep the document object alive and work regardless of whether the document is in frame, or ever was. Keeping a document alive is fine, but in the back/forward cache case the document is in suspended animation. I think it would be bad to run any JavaScript at all when the document is in that state. But I could be wrong.
I agree that running JS in a context of a document that's in b/f cache is unacceptable. But event dispatch is different - a handler can be in another context. E.g. var doc = frames[0].document; doc.documentElement.addEventListener("AsyncDOMMutation", function() { alert("ha-ha!") }, true); frames[0].navigateAway(); doc.documentElement.appendElement(...); There is no reason for the alert to not be displayed. JSEventListener::handleEvent() takes care of not running scripts in documents that are in b/f cache or frameless.
Could someone review this patch? There are two bugs depending on this change.
Given Alexey's clarifications in comment 16 about event dispatch and the b/f cache, are there any other concerns with this patch?
Comment on attachment 74399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74399&action=review Looks ok overall, except for the win build fix mentioned. > WebCore/WebCore.vcproj/WebCore.vcproj:42430 > + RelativePath="..\dom\EventQueue.cpp" I think you also need to add this file to DOMAllInOne for it to build on Win.
Created attachment 76568 [details] Patch
(In reply to comment #20) > I think you also need to add this file to DOMAllInOne for it to build on Win. Fixed.
Attachment 76568 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6948114
Comment on attachment 76568 [details] Patch Looking at Chromium build issue.
Comment on attachment 76568 [details] Patch Rejecting attachment 76568 [details] from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'apply-attachment', '--non-interactive', 76568]" exit_code: 2 Last 500 characters of output: project.pbxproj patching file WebCore/dom/DOMAllInOne.cpp patching file WebCore/dom/Document.cpp patching file WebCore/dom/Document.h patching file WebCore/dom/EventQueue.cpp patching file WebCore/dom/EventQueue.h patching file WebCore/storage/StorageEventDispatcher.cpp patching file WebKit/chromium/ChangeLog patching file WebKit/chromium/src/StorageAreaProxy.cpp Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Dimitri Glazkov', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/6992126
Attachment 76568 [details] did not build on qt: Build output: http://queues.webkit.org/results/7023131
Attachment 76568 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7073001
Created attachment 76574 [details] Patch for landing
Comment on attachment 76574 [details] Patch for landing Rejecting attachment 76574 [details] from commit-queue. Failed to run "['/Projects/WebKit/WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--non-interactive', 76574]" exit_code: 2 Last 500 characters of output: ing file WebCore/WebCore.xcodeproj/project.pbxproj patching file WebCore/dom/DOMAllInOne.cpp patching file WebCore/dom/Document.cpp patching file WebCore/dom/Document.h patching file WebCore/dom/EventQueue.cpp patching file WebCore/dom/EventQueue.h patching file WebCore/storage/StorageEventDispatcher.cpp patching file WebKit/chromium/ChangeLog patching file WebKit/chromium/src/StorageAreaProxy.cpp Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/7007113
(In reply to comment #24) > (From update of attachment 76568 [details]) > Looking at Chromium build issue. The EventQueue forward declaration in Document.h got dropped due a merge conflict, hence the build issues (with all platforms). New patch should build correctly. Will land manually due to the cq complaining about merge issues.
Committed r74062: <http://trac.webkit.org/changeset/74062>
Attachment 76568 [details] did not build on win: Build output: http://queues.webkit.org/results/6979140