[webkit-reviews] review canceled: [Bug 57642] Move WheelEvent-dispatching logic into WheelEventDispatchMediator. : [Attachment 88274] Ready to bake on EWSs.

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has canceled Dimitri Glazkov
(Google) <dglazkov at chromium.org>'s request for review:
Bug 57642: Move WheelEvent-dispatching logic into WheelEventDispatchMediator.
https://bugs.webkit.org/show_bug.cgi?id=57642

Attachment 88274: Ready to bake on EWSs.
https://bugs.webkit.org/attachment.cgi?id=88274&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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.