Bug 122144 – Get rid of Node::preDispatchEventHandler and Node::postDispatchEventHandler

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Node::preDispatchEventHandler and Node::postDispatchEventHandler are only implemented by HTMLInputElement and HTMLMediaElement for a couple of events.
There is no reason every single event dispatch has to pay the cost of calling these virtual functions.

Furthermore, preDispatchEventHandlerResult that's passed around from preDispatchEventHandler and postDispatchEventHandler is only used by HTMLInputElement's preDispatchEventHandler and it's always of type ClickHandlingState. Since ClickHandlingState is never subclassed, we don't even need to heap-allocate this object.

[Ryosuke Niwa]

Created attachment 213147 [details]
Cleanup

[Andreas Kling]

Comment on attachment 213147 [details]
Cleanup

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

> Source/WebCore/dom/Document.cpp:5392
> +    dispatchFullScreenChangeOrErrorEvent(changeQueue, eventNames().webkitfullscreenchangeEvent, true /* shouldNotifyMediaElement */);
> +    dispatchFullScreenChangeOrErrorEvent(errorQueue, eventNames().webkitfullscreenerrorEvent, false /* shouldNotifyMediaElement */);

I think typically we do "/* variableName */ true" rather than "true /* variableName */"
Of course it'd be nicest to use enums instead of magical boolean arguments.

> Source/WebCore/dom/Document.cpp:5411
> +        if (shouldNotifyMediaElement && isElement(*node.get()) && toElement(node.get())->isMediaElement())
> +            toHTMLMediaElement(node.get())->willDispatchFullScreenChangeEvent();

if (shouldNotifyMediaElement && isMediaElement(node.get()))

> Source/WebCore/dom/EventDispatcher.cpp:137
> +    if (isHTMLInputElement(m_node.get())) {

I would use early return style here.

> Source/WebCore/dom/EventDispatcher.cpp:194
> +inline void EventDispatcher::dispatchEventPostProcess(const ClickHandlingState* inputElmenetClickHandlingState)

inputElmenetClickHandlingState
Typo, Elmenet

[Ryosuke Niwa]

Created attachment 213149 [details]
More cleanups

[Darin Adler]

Comment on attachment 213149 [details]
More cleanups

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

While I am OK with this, I think it can be improved.

> Source/WebCore/dom/Document.cpp:5411
> +        if (shouldNotifyMediaElement && isMediaElement(node.get()))
> +            toHTMLMediaElement(node.get())->willDispatchFullScreenChangeEvent();

This seems awkward. Is there really not a better way of doing this? I’m not even sure that the work to configure the media element has much to do with DOM events. Maybe we can do this in Document::fullScreenChangeDelayTimerFired rather than inside the dispatching function.

> Source/WebCore/dom/EventDispatcher.cpp:115
> +    InputElementClickHandlingState inputElementClickHandlingState;

I don’t think the local variable needs such a long name. How about just "clickHandlingState" or even just "state"? The type name will still be there.

> Source/WebCore/dom/EventDispatcher.cpp:116
> +    if (callWillDispatchEventOnInputElement(inputElementClickHandlingState) == ContinueDispatching && !m_eventPath.isEmpty()) {

The m_eventPath.isEmpty() check here is non-obvious. I don’t know why you chose to move this out of callWillDispatchEventOnInputElement but not the m_event.propagationStopped part. The EventDispatchContinuation enum seems quite awkward.

> Source/WebCore/dom/EventDispatcher.cpp:141
> -inline EventDispatchContinuation EventDispatcher::dispatchEventPreProcess(void*& preDispatchEventHandlerResult)
> +inline EventDispatchContinuation EventDispatcher::callWillDispatchEventOnInputElement(InputElementClickHandlingState& state)
>  {
> -    // Give the target node a chance to do some work before DOM event handlers get a crack.
> -    preDispatchEventHandlerResult = m_node->preDispatchEventHandler(m_event.get());
> -    return (m_eventPath.isEmpty() || m_event->propagationStopped()) ? DoneDispatching : ContinueDispatching;
> +    if (isHTMLInputElement(m_node.get())) {
> +        toHTMLInputElement(m_node.get())->willDispatchEvent(m_event.get(), state);
> +        if (m_event->propagationStopped())
> +            return DoneDispatching;
> +    }
> +    return ContinueDispatching;

I think it’s not all that helpful to have this as a separate function. Can we just put this inline in EventDispatcher::dispatch? Or maybe find some other way to factor the code into a separate function. This particular function just seems like a random snippet of code broken out into a separate function, not something with its own separate mission.

And given we still have a dispatchEventPostProcess function, maybe we should try to keep this named dispatchEventPreProcess and have it do a bit more? I’m not really sure.

> Source/WebCore/dom/EventDispatcher.cpp:193
> -inline void EventDispatcher::dispatchEventPostProcess(void* preDispatchEventHandlerResult)
> +inline void EventDispatcher::dispatchEventPostProcess(const InputElementClickHandlingState& inputElementClickHandlingState)

I think the argument name here is too long. I think we can just call it clickHandlingState or maybe event just state since the type name will still be there.

> Source/WebCore/html/HTMLInputElement.h:47
> +class InputElementClickHandlingState {
> +    WTF_MAKE_FAST_ALLOCATED;

This seems more like a struct with a constructor than a class. I suggest making it a struct.

Why WTF_MAKE_FAST_ALLOCATED? I don’t think we are allocating these with new/delete any more.

Would be nice to have a slightly more streamlined name. Maybe InputElementClickState or InputElementClickEventState or CheckboxClickState?

> Source/WebCore/html/HTMLInputElement.h:53
> +    InputElementClickHandlingState()
> +    : stateful(false)
> +    , checked(false)
> +    , indeterminate(false)
> +    { }

Formatting here isn’t right. I suggest one of these two forms:

    InputElementClickHandlingState()
        : stateful(false)
        , checked(false)
        , indeterminate(false)
    {
    }

Or:

    InputElementClickHandlingState() : stateful(false), checked(false), indeterminate(false) { }

To me, either of these seems consistent with our style guidelines, but the formatting you chose does not seem to be.

> Source/WebCore/html/HTMLMediaElement.cpp:-4830
> -    if (event && event->type() == eventNames().webkitfullscreenchangeEvent)
> -        configureMediaControls();

The patch describes this as a refactoring.

But this is not just a refactoring, it’s a behavior change. We now do this work when *sending* a full screen change event, rather than when receiving it. In the old code, sending the event from a webpage to the media element would trigger this. In the new code, it’s only the engine that can trigger it.

Was the old behavior incorrect? Is the new behavior correct?

> Source/WebCore/html/HTMLMediaElement.h:386
> +    void willDispatchFullScreenChangeEvent() { configureMediaControls(); }

I’m not too fond of this name. If my guess is correct, this media control configuration is really not about dispatching the event. It’s about being properly configured for full screen display. The event is to let the content know that happened, not a way to tell the media element to configure itself.

[Ryosuke Niwa]

(In reply to comment #4)
> > Source/WebCore/dom/Document.cpp:5411
> > +        if (shouldNotifyMediaElement && isMediaElement(node.get()))
> > +            toHTMLMediaElement(node.get())->willDispatchFullScreenChangeEvent();
> 
> This seems awkward. Is there really not a better way of doing this? I’m not even sure that the work to configure the media element has much to do with DOM events. Maybe we can do this in Document::fullScreenChangeDelayTimerFired rather than inside the dispatching function.

Renamed to enteredOrExitedFullscreen.

> > Source/WebCore/dom/EventDispatcher.cpp:115
> > +    InputElementClickHandlingState inputElementClickHandlingState;
> 
> I don’t think the local variable needs such a long name. How about just "clickHandlingState" or even just "state"? The type name will still be there.

Renamed to clickHandlingState.

> > Source/WebCore/dom/EventDispatcher.cpp:116
> > +    if (callWillDispatchEventOnInputElement(inputElementClickHandlingState) == ContinueDispatching && !m_eventPath.isEmpty()) {
> 
> The m_eventPath.isEmpty() check here is non-obvious. I don’t know why you chose to move this out of callWillDispatchEventOnInputElement but not the m_event.propagationStopped part. The EventDispatchContinuation enum seems quite awkward.
> 
> > Source/WebCore/dom/EventDispatcher.cpp:141
> > -inline EventDispatchContinuation EventDispatcher::dispatchEventPreProcess(void*& preDispatchEventHandlerResult)
> > +inline EventDispatchContinuation EventDispatcher::callWillDispatchEventOnInputElement(InputElementClickHandlingState& state)
> >  {
> > -    // Give the target node a chance to do some work before DOM event handlers get a crack.
> > -    preDispatchEventHandlerResult = m_node->preDispatchEventHandler(m_event.get());
> > -    return (m_eventPath.isEmpty() || m_event->propagationStopped()) ? DoneDispatching : ContinueDispatching;
> > +    if (isHTMLInputElement(m_node.get())) {
> > +        toHTMLInputElement(m_node.get())->willDispatchEvent(m_event.get(), state);
> > +        if (m_event->propagationStopped())
> > +            return DoneDispatching;
> > +    }
> > +    return ContinueDispatching;
> 
> I think it’s not all that helpful to have this as a separate function. Can we just put this inline in EventDispatcher::dispatch? Or maybe find some other way to factor the code into a separate function. This particular function just seems like a random snippet of code broken out into a separate function, not something with its own separate mission.

Done.

> > Source/WebCore/dom/EventDispatcher.cpp:193
> > -inline void EventDispatcher::dispatchEventPostProcess(void* preDispatchEventHandlerResult)
> > +inline void EventDispatcher::dispatchEventPostProcess(const InputElementClickHandlingState& inputElementClickHandlingState)
> 
> I think the argument name here is too long. I think we can just call it clickHandlingState or maybe event just state since the type name will still be there.
> 
> > Source/WebCore/html/HTMLInputElement.h:47
> > +class InputElementClickHandlingState {
> > +    WTF_MAKE_FAST_ALLOCATED;
> 
> This seems more like a struct with a constructor than a class. I suggest making it a struct.
> Why WTF_MAKE_FAST_ALLOCATED? I don’t think we are allocating these with new/delete any more.

Fixed.

> Would be nice to have a slightly more streamlined name. Maybe InputElementClickState or InputElementClickEventState or CheckboxClickState?

Renamed to InputElementClickState.

> > Source/WebCore/html/HTMLInputElement.h:53
> > +    InputElementClickHandlingState()
> > +    : stateful(false)
> > +    , checked(false)
> > +    , indeterminate(false)
> > +    { }
> 
> Formatting here isn’t right. I suggest one of these two forms:
> 
>     InputElementClickHandlingState()
>         : stateful(false)
>         , checked(false)
>         , indeterminate(false)
>     {
>     }
> 
> Or:
> 
>     InputElementClickHandlingState() : stateful(false), checked(false), indeterminate(false) { }
> 
> To me, either of these seems consistent with our style guidelines, but the formatting you chose does not seem to be.

Fixed.

> > Source/WebCore/html/HTMLMediaElement.cpp:-4830
> > -    if (event && event->type() == eventNames().webkitfullscreenchangeEvent)
> > -        configureMediaControls();
> 
> The patch describes this as a refactoring.
> 
> But this is not just a refactoring, it’s a behavior change. We now do this work when *sending* a full screen change event, rather than when receiving it. In the old code, sending the event from a webpage to the media element would trigger this. In the new code, it’s only the engine that can trigger it.
> Was the old behavior incorrect? Is the new behavior correct?

The old code was wrong since configureMediaControls is called to show the media controls after entering or existing fullscreen.  Unfortunately, I couldn't create a test case after a couple hours of attempt.

> > Source/WebCore/html/HTMLMediaElement.h:386
> > +    void willDispatchFullScreenChangeEvent() { configureMediaControls(); }
> 
> I’m not too fond of this name. If my guess is correct, this media control configuration is really not about dispatching the event. It’s about being properly configured for full screen display. The event is to let the content know that happened, not a way to tell the media element to configure itself.

Renamed to enteredOrExitedFullscreen.

[Ryosuke Niwa]

Created attachment 213227 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 213227 [details]
Patch for landing

Clearing flags on attachment: 213227

Committed r156825: <http://trac.webkit.org/changeset/156825>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Somehow removals in Node.h got lost. Committed it in http://trac.webkit.org/changeset/156826.