Created attachment 102769 [details] Supports EventDispatchMediator

Comment on attachment 102769 [details] Supports EventDispatchMediator I am not sure I like this patch. Now that we can pass around EventDispatchMediator, we should be able to convert all scoped event dispatches to use it, instead of building a parallel structure. Right?

Thank you for the review. (In reply to comment #2) > (From update of attachment 102769 [details]) > I am not sure I like this patch. Now that we can pass around EventDispatchMediator, we should be able to convert all scoped event dispatches to use it, instead of building a parallel structure. Right? That makes sense and actually I tried that at first. AFAIR, I gave up that for some reasons, which I can't recall now. Let me retry that tomorrow again to recall the reason.

Created attachment 102886 [details] no more parallel structure

Hi Dimitri, I updated a patch so that only a EventDispatchMediator, instead of a parallel structure, is enqueued into a internal queue of ScopedEventQueue. To do so, I had to make EventDispatchMediator::event() public, not private. I guess making it public is not desirable, but that's much better than maintaing parallel data structure in ScopedEventQueue. (In reply to comment #3) > Thank you for the review. > > (In reply to comment #2) > > (From update of attachment 102769 [details] [details]) > > I am not sure I like this patch. Now that we can pass around EventDispatchMediator, we should be able to convert all scoped event dispatches to use it, instead of building a parallel structure. Right? > > That makes sense and actually I tried that at first. AFAIR, I gave up that for some reasons, which I can't recall now. > Let me retry that tomorrow again to recall the reason.

Comment on attachment 102886 [details] no more parallel structure View in context: https://bugs.webkit.org/attachment.cgi?id=102886&action=review I like it, but it doesn't go far enough. Be brave, Ito-san! :) > Source/WebCore/dom/ScopedEventQueue.cpp:70 > void ScopedEventQueue::enqueueEvent(PassRefPtr<Event> event) > { > if (m_scopingLevel) > - m_queuedEvents.append(event); > + m_queuedEventDispatchMediators.append(EventDispatchMediator::create(event)); > + else { > + RefPtr<EventTarget> eventTarget = event->target(); > + eventTarget->dispatchEvent(event); > + } > +} Can we just kill all callsites for this and remove it?

Created attachment 103024 [details] remove enqueueEvent and clients of that

Hi Dimitri, Thank you for the review. (In reply to comment #6) > (From update of attachment 102886 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102886&action=review > > I like it, but it doesn't go far enough. Be brave, Ito-san! :) > > > Source/WebCore/dom/ScopedEventQueue.cpp:70 > > void ScopedEventQueue::enqueueEvent(PassRefPtr<Event> event) > > { > > if (m_scopingLevel) > > - m_queuedEvents.append(event); > > + m_queuedEventDispatchMediators.append(EventDispatchMediator::create(event)); > > + else { > > + RefPtr<EventTarget> eventTarget = event->target(); > > + eventTarget->dispatchEvent(event); > > + } > > +} > > Can we just kill all callsites for this and remove it? Sure. We can remove them to simplify code. Done.

Comment on attachment 103024 [details] remove enqueueEvent and clients of that View in context: https://bugs.webkit.org/attachment.cgi?id=103024&action=review > Source/WebCore/dom/Node.cpp:2710 > void Node::dispatchScopedEvent(PassRefPtr<Event> event) Can we remove this too?

Thank you for the review again. (In reply to comment #9) > (From update of attachment 103024 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103024&action=review > > > Source/WebCore/dom/Node.cpp:2710 > > void Node::dispatchScopedEvent(PassRefPtr<Event> event) > > Can we remove this too? We can. But we have to touch a lot of files to remove that because there are a lot of clients using that function. If we remove that, all clientes will be forced to use node->dispatchEvent(EventDispatchMediater::create(Event:create(....,)); instead of node->dispatchEvent(Event:create(....,));. Is it okay? Anyway, that requires touching a lot of files. So it might be done in another patch. I am going to this patch.

s/ I am going to this patch / I am going to land this patch./

Please cc me for these scopedEventQueue-related bugs in the future.

Comment on attachment 103024 [details] remove enqueueEvent and clients of that Clearing flags on attachment: 103024 Committed r92450: <http://trac.webkit.org/changeset/92450>

All reviewed patches have been landed. Closing bug.