Bug 65613 – ScopedEventQueue should enqueue an EventDispatchMediator

• Component: UI Events
• Status: RESOLVED FIXED

[Hayato Ito]

This patch is the 2nd separated patch from bug 64249.

ScopedEventQueue should support enqueuing an EventDispatchMediator so that it supports coming FocusIn/FocusOutEventDispatchMediator.

[Hayato Ito]

Created attachment 102769 [details]
Supports EventDispatchMediator

[Dimitri Glazkov (Google)]

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?

[Hayato Ito]

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.

[Hayato Ito]

Created attachment 102886 [details]
no more parallel structure

[Hayato Ito]

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.

[Dimitri Glazkov (Google)]

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?

[Hayato Ito]

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

[Hayato Ito]

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.

[Dimitri Glazkov (Google)]

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?

[Hayato Ito]

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.

[Hayato Ito]

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

[Ryosuke Niwa]

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

[WebKit Review Bot]

Comment on attachment 103024 [details]
remove enqueueEvent and clients of that

Clearing flags on attachment: 103024

Committed r92450: <http://trac.webkit.org/changeset/92450>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.