Bug 102610

Summary: Extend EventDispatcher::dispatchSimulatedClick to allow sending a mouseover event
Product: WebKit Reporter: Jon Lee <jonlee>
Component: UI EventsAssignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, beidson, mifenton, mitz, sam, tkent, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.8   
Bug Depends on:    
Bug Blocks: 98318    
Attachments:
Description Flags
Patch
none
Patch darin: review+

Description Jon Lee 2012-11-18 00:12:07 PST
Allow dispatched clicks to also send a mouse over event. This is needed to pass click events for plugin snapshotting.
Comment 1 Jon Lee 2012-11-18 00:12:28 PST
<rdar://problem/12725663>
Comment 2 Jon Lee 2012-11-18 01:59:53 PST
Created attachment 174844 [details]
Patch
Comment 3 Darin Adler 2012-11-18 07:39:08 PST
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!
Comment 4 Jon Lee 2012-11-18 13:40:30 PST
(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.
Comment 5 Jon Lee 2012-11-18 17:23:07 PST
Created attachment 174865 [details]
Patch
Comment 6 Darin Adler 2012-11-25 21:34:18 PST
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.
Comment 7 Jon Lee 2012-11-26 00:23:55 PST
(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.
Comment 8 Jon Lee 2012-11-26 01:13:09 PST
Committed r135690: <http://trac.webkit.org/changeset/135690>