Bug 102610 – Extend EventDispatcher::dispatchSimulatedClick to allow sending a mouseover event

• Component: UI Events
• Status: RESOLVED FIXED

[Jon Lee]

Allow dispatched clicks to also send a mouse over event. This is needed to pass click events for plugin snapshotting.

[Jon Lee]

<rdar://problem/12725663>

[Jon Lee]

Created attachment 174844 [details]
Patch

[Darin Adler]

Comment on attachment 174844 [details]
Patch

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

> Source/WebCore/dom/Node.h:643
> +    void dispatchSimulatedClick(Event* underlyingEvent, bool sendMouseUpDownEvents = false, bool sendMouseOverEvent = false, bool showPressedLook = true);

This is taking a bad API and making it worse. These boolean arguments are completely unreadable at call sites. If we were creating a function like this today in WebKit we would use enums instead of booleans because this function is often called with constants, which are completely unreadable at the call site.

So if you are touching this function in this way, you should change these booleans into enums, or find some other way of refactoring this function so it’s readable at call sites.

> Source/WebCore/html/HTMLElement.cpp:693
> -    dispatchSimulatedClick(0, false, false);
> +    dispatchSimulatedClick(0, false, false, false);

This is a perfect example, what does 0, false, false, false mean here? Totally mysterious!

[Jon Lee]

(In reply to comment #3)
> (From update of attachment 174844 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174844&action=review
> 
> > Source/WebCore/dom/Node.h:643
> > +    void dispatchSimulatedClick(Event* underlyingEvent, bool sendMouseUpDownEvents = false, bool sendMouseOverEvent = false, bool showPressedLook = true);
> 
> This is taking a bad API and making it worse. These boolean arguments are completely unreadable at call sites. If we were creating a function like this today in WebKit we would use enums instead of booleans because this function is often called with constants, which are completely unreadable at the call site.
> 
> So if you are touching this function in this way, you should change these booleans into enums, or find some other way of refactoring this function so it’s readable at call sites.
> 
> > Source/WebCore/html/HTMLElement.cpp:693
> > -    dispatchSimulatedClick(0, false, false);
> > +    dispatchSimulatedClick(0, false, false, false);
> 
> This is a perfect example, what does 0, false, false, false mean here? Totally mysterious!

True. I had a previous version that used the Event::EventType enum as a mask, but thought that might be too esoteric. I'll switch to a dedicated enum for this function. It'll be nice to be able to condense all three of the bool parameters into one.

[Jon Lee]

Created attachment 174865 [details]
Patch

[Darin Adler]

Comment on attachment 174865 [details]
Patch

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

> Source/WebCore/dom/EventDispatcher.cpp:226
> +    if (mouseEventOptions > SendNoEvents)

This seems fragile. If someone adds a new value to the enum they won’t realize that the order of enum values matters. I suggest writing this differently.

> Source/WebCore/dom/EventDispatcher.cpp:229
> +    if (mouseEventOptions > SendNoEvents)

Same comment as above.

> Source/WebCore/dom/EventDispatcher.h:29
> +#include "Node.h"

I don’t think we want to pull in the whole Node.h header to every file that includes EventDispatcher.h. We should find some other way to get the enum in the right files.

[Jon Lee]

(In reply to comment #6)
> (From update of attachment 174865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174865&action=review
> 
> > Source/WebCore/dom/EventDispatcher.cpp:226
> > +    if (mouseEventOptions > SendNoEvents)
> 
> This seems fragile. If someone adds a new value to the enum they won’t realize that the order of enum values matters. I suggest writing this differently.

Switched to using !=.

> > Source/WebCore/dom/EventDispatcher.h:29
> > +#include "Node.h"
> 
> I don’t think we want to pull in the whole Node.h header to every file that includes EventDispatcher.h. We should find some other way to get the enum in the right files.

I'll add a new SimulatedClickOptions.h file.

[Jon Lee]

Committed r135690: <http://trac.webkit.org/changeset/135690>