Bug 57642 – Move WheelEvent-dispatching logic into WheelEventDispatchMediator.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Move WheelEvent-dispatching logic into WheelEventDispatchMediator.

[Dimitri Glazkov (Google)]

Created attachment 87901 [details]
Patch

[Dimitri Glazkov (Google)]

Created attachment 88274 [details]
Ready to bake on EWSs.

[Eric Seidel (no email)]

Comment on attachment 88274 [details]
Ready to bake on EWSs.

View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

I guess you've not written the ChangelOg yet?

> Source/WebCore/dom/Event.h:204
> +    EventDispatchMediator();

What does this class mediate between?

[Dimitri Glazkov (Google)]

(In reply to comment #3)
> (From update of attachment 88274 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> I guess you've not written the ChangelOg yet?

No, I just forgot to replace this line :)

> 
> > Source/WebCore/dom/Event.h:204
> > +    EventDispatchMediator();
> 
> What does this class mediate between?

It mediates event dispatch. This class knows specifics of firing events, helping EventDispatcher do the right thing without having to know how.

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 88274 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        No new tests. (OOPS!)
> > 
> > I guess you've not written the ChangelOg yet?
> 
> No, I just forgot to replace this line :)
> 
> > 
> > > Source/WebCore/dom/Event.h:204
> > > +    EventDispatchMediator();
> > 
> > What does this class mediate between?
> 
> It mediates event dispatch. This class knows specifics of firing events, helping EventDispatcher do the right thing without having to know how.

Believe it or not, the whole EventDispatchMediator was born out of your comment: you said the EventDispatcher knows too much :)

[Darin Adler]

Comment on attachment 88274 [details]
Ready to bake on EWSs.

View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review

> Source/WebCore/dom/Node.cpp:2665
> -void Node::dispatchWheelEvent(PlatformWheelEvent& e)
> +bool Node::dispatchWheelEvent(PlatformWheelEvent& e)
>  {
> -    EventDispatcher::dispatchWheelEvent(this, e);
> +    return EventDispatcher::dispatchEvent(this, WheelEventDispatchMediator(e, document()->defaultView()));
>  }

As long as you are touching this, I would prefer the name “event” to “e”.

> Source/WebCore/dom/Node.h:536
> -    void dispatchWheelEvent(PlatformWheelEvent&);
> +    bool dispatchWheelEvent(PlatformWheelEvent&);

Is this really a good change?

The DOM establishes a pattern where events themselves know if they have been handled. I think this is clearer than a boolean return value. It’s hard to name a function so you can understand what its boolean return value means.

Worse, we have functions that tell whether an event is handled both with a return value and with state in the event too! Although this is not one of them.

If you really ant to change this then I think it should take a const&. The reason this took a non-const before was so it could record that the event was handled.

> Source/WebCore/dom/WheelEvent.cpp:104
> +    : EventDispatchMediator()

Do you need to say this explicitly? I am surprised if so.

[Dimitri Glazkov (Google)]

Comment on attachment 88274 [details]
Ready to bake on EWSs.

View in context: https://bugs.webkit.org/attachment.cgi?id=88274&action=review

Darin, thank you for review. Will upload new patch shortly.

>> Source/WebCore/dom/Node.cpp:2665
>>  }
> 
> As long as you are touching this, I would prefer the name “event” to “e”.

Will change.

>> Source/WebCore/dom/Node.h:536
>> +    bool dispatchWheelEvent(PlatformWheelEvent&);
> 
> Is this really a good change?
> 
> The DOM establishes a pattern where events themselves know if they have been handled. I think this is clearer than a boolean return value. It’s hard to name a function so you can understand what its boolean return value means.
> 
> Worse, we have functions that tell whether an event is handled both with a return value and with state in the event too! Although this is not one of them.
> 
> If you really ant to change this then I think it should take a const&. The reason this took a non-const before was so it could record that the event was handled.

This just brings it in line with other similar events. Both dispatchEvent and dispatchKeyEvent return false when they weren't handled. I definitely overlooked the non-const ref. Will change that.

>> Source/WebCore/dom/WheelEvent.cpp:104
>> +    : EventDispatchMediator()
> 
> Do you need to say this explicitly? I am surprised if so.

No. I just got too mechanical in writing the code. I'll remove.

[Dimitri Glazkov (Google)]

Created attachment 88566 [details]
Patch

[Dimitri Glazkov (Google)]

<3

[Dimitri Glazkov (Google)]

Committed r83298: <http://trac.webkit.org/changeset/83298>