Bug 102610 - Extend EventDispatcher::dispatchSimulatedClick to allow sending a mouseover event
Summary: Extend EventDispatcher::dispatchSimulatedClick to allow sending a mouseover e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.8
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks: 98318
  Show dependency treegraph
 
Reported: 2012-11-18 00:12 PST by Jon Lee
Modified: 2012-11-26 01:13 PST (History)
8 users (show)

See Also:


Attachments
Patch (6.93 KB, patch)
2012-11-18 01:59 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (13.35 KB, patch)
2012-11-18 17:23 PST, Jon Lee
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>