Bug 122144 - Get rid of Node::preDispatchEventHandler and Node::postDispatchEventHandler
Summary: Get rid of Node::preDispatchEventHandler and Node::postDispatchEventHandler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 122143
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-01 01:05 PDT by Ryosuke Niwa
Modified: 2013-10-03 00:32 PDT (History)
6 users (show)

See Also:


Attachments
Cleanup (21.83 KB, patch)
2013-10-01 20:39 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
More cleanups (22.76 KB, patch)
2013-10-01 21:01 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (22.16 KB, patch)
2013-10-02 22:49 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-10-01 01:05:16 PDT
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.
Comment 1 Ryosuke Niwa 2013-10-01 20:39:15 PDT
Created attachment 213147 [details]
Cleanup
Comment 2 Andreas Kling 2013-10-01 20:50:49 PDT
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
Comment 3 Ryosuke Niwa 2013-10-01 21:01:16 PDT
Created attachment 213149 [details]
More cleanups
Comment 4 Darin Adler 2013-10-02 09:11:18 PDT
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.
Comment 5 Ryosuke Niwa 2013-10-02 22:43:17 PDT
(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.
Comment 6 Ryosuke Niwa 2013-10-02 22:49:19 PDT
Created attachment 213227 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2013-10-02 23:49:34 PDT
Comment on attachment 213227 [details]
Patch for landing

Clearing flags on attachment: 213227

Committed r156825: <http://trac.webkit.org/changeset/156825>
Comment 8 WebKit Commit Bot 2013-10-02 23:49:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Ryosuke Niwa 2013-10-03 00:32:49 PDT
Somehow removals in Node.h got lost. Committed it in http://trac.webkit.org/changeset/156826.