[webkit-reviews] review granted: [Bug 122144] Get rid of Node::preDispatchEventHandler and Node::postDispatchEventHandler : [Attachment 213149] More cleanups

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 122144: Get rid of Node::preDispatchEventHandler and
Node::postDispatchEventHandler
https://bugs.webkit.org/show_bug.cgi?id=122144

Attachment 213149: More cleanups
https://bugs.webkit.org/attachment.cgi?id=213149&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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(InputElementClickHandlingS
tate& 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.