Bug 154133

<rdar://problem/24616246>

Created attachment 271132 [details] Patch

Created attachment 271133 [details] Patch

Comment on attachment 271133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271133&action=review This patch is OK, but I have a better idea of how to do this. You can land it as-is, but then I see a way to make this cleaner. > Source/WebCore/dom/EventDispatcher.cpp:171 > + if (creationOptions == SimulatedClickCreationOptions::FromUserAgent) { > + if (mouseEventOptions == SendMouseOverUpDownEvents) > + dispatchEvent(element, SimulatedMouseEvent::create(eventNames().mouseoverEvent, element->document().defaultView(), underlyingEvent, element)); > + > + if (mouseEventOptions != SendNoEvents) > + dispatchEvent(element, SimulatedMouseEvent::create(eventNames().mousedownEvent, element->document().defaultView(), underlyingEvent, element)); > + element->setActive(true, visualOptions == ShowPressedLook); > + if (mouseEventOptions != SendNoEvents) > + dispatchEvent(element, SimulatedMouseEvent::create(eventNames().mouseupEvent, element->document().defaultView(), underlyingEvent, element)); > + element->setActive(false); > + > + // always send click > + dispatchEvent(element, SimulatedMouseEvent::create(eventNames().clickEvent, element->document().defaultView(), underlyingEvent, element)); > + } else if (creationOptions == SimulatedClickCreationOptions::FromBindings) { > + SimulatedMouseEventInit init; > + init.bubbles = true; > + init.cancelable = true; > + init.view = element->document().defaultView(); > + init.isSimulated = true; > + init.underlyingEvent = underlyingEvent; > + init.target = element; > + > + if (mouseEventOptions == SendMouseOverUpDownEvents) > + dispatchEvent(element, SimulatedMouseEvent::createForBindings(eventNames().mouseoverEvent, init)); > + > + if (mouseEventOptions != SendNoEvents) > + dispatchEvent(element, SimulatedMouseEvent::createForBindings(eventNames().mousedownEvent, init)); > + element->setActive(true, visualOptions == ShowPressedLook); > + if (mouseEventOptions != SendNoEvents) > + dispatchEvent(element, SimulatedMouseEvent::createForBindings(eventNames().mouseupEvent, init)); > + element->setActive(false); > + > + // always send click > + dispatchEvent(element, SimulatedMouseEvent::createForBindings(eventNames().clickEvent, init)); > + } This seems excessive. I don’t think we need to make any changes to SimulatedMouseEvent itself (although changing PassRefPtr to RefPtr&& is nice!). Instead, I think the code should just use a private helper function, dispatchSimulatedMouseEvent that would say: auto event = SimulatedMouseEvent::create(<arguments>); if (creationOptions == SimulatedClickCreationOptions::FromBindings) event.get().setUntrusted(); dispatchEvent(element, event.get()); I know that’s not the pattern we use elsewhere, but I think it’s fine here. No need to create the events in an entirely different way and pretend they were created directly by JavaScript when that’s not the real way the code flows. I also think that SimulatedMouseEvent should be moved out of the public headers so no one is tempted to use it in a different way. It’s should be used only by this function, dispatchSimulatedClick. So what I would suggest is that we move this function and the SimulatedMouseEvent class into a new file, SimulatedClick.h/cpp. The header file would have only one function, dispatchSimulatedClick, and the implementation file would contain the entire SimulatedMouseEvent class, which would be removed from MouseEvent.h and MouseEvent.cpp. Note that this header would also be included only by Element.cpp! I would also put SimulatedClickCreationOptions into SimulatedClick.h instead of SimulatedClickOptions.h. The options are for the functions that code uses directly, the ones in the Element class. Whereas the SimulatedClickCreationOptions is only for use inside Element.cpp. We could also consider renaming this function. I’m not sure that “dispatch” is a great name for a function that creates new events and dispatches them. The other “dispatch” functions take an existing event and dispatch it. I think a better name for the function would be “simulateClick”. > Source/WebCore/dom/EventDispatcher.h:42 > +void dispatchSimulatedClick(Element*, Event* underlyingEvent, SimulatedClickMouseEventOptions, SimulatedClickVisualOptions, SimulatedClickCreationOptions); The element argument should be Element&, not Element*.

Created attachment 271248 [details] Patch for landing

Attachment 271248 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SimulatedClick.cpp:51: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/SimulatedClick.cpp:53: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/SimulatedClick.cpp:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 271269 [details] Patch

Attachment 271269 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SimulatedClick.cpp:51: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/SimulatedClick.cpp:53: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/SimulatedClick.cpp:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 271269 [details] Patch Rejecting attachment 271269 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 271269, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: m/SimulatedClick.cpp patching file Source/WebCore/dom/SimulatedClick.h patching file Source/WebCore/html/HTMLElement.cpp patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/imported/blink/fast/events/event-trusted-expected.txt patching file LayoutTests/imported/blink/fast/events/event-trusted.html Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/832061

Comment on attachment 271269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=271269&action=review > Source/WebCore/dom/Element.h:462 > + void dispatchSimulatedClickForBindings(Event* underlyingEvent, SimulatedClickMouseEventOptions = SendNoEvents, SimulatedClickVisualOptions = ShowPressedLook); I suggest we remove the options arguments from this function and instead have it always use the options "SendNoEvents" and "DoNotShowPressedLook", since the only caller wants those options. We can always add the options later if needed. > Source/WebCore/dom/SimulatedClick.cpp:47 > + virtual ~SimulatedMouseEvent() > + { > + } This is not needed and should just be removed. > Source/WebCore/dom/SimulatedClick.cpp:77 > +static void simulateClickInternal(const AtomicString& eventType, Element& element, Event* underlyingEvent, SimulatedClickCreationOptions creationOptions) I’d name this simulateMouseEvent. > Source/WebCore/dom/SimulatedClick.cpp:104 > + // always send click We should remove this comment, unless we replace it with a longer one that says *why* we always send a click event. I think there’s no need for a comment. > Source/WebCore/dom/SimulatedClick.h:36 > +enum SimulatedClickCreationOptions { Should change this to an enum class.

Comment on attachment 271269 [details] Patch Rejecting attachment 271269 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 271269, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: m/SimulatedClick.cpp patching file Source/WebCore/dom/SimulatedClick.h patching file Source/WebCore/html/HTMLElement.cpp patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/imported/blink/fast/events/event-trusted-expected.txt patching file LayoutTests/imported/blink/fast/events/event-trusted.html Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/832086

Created attachment 271352 [details] Patch for landing

Created attachment 271359 [details] Patch for landing

Attachment 271359 [details] did not pass style-queue: ERROR: Source/WebCore/dom/SimulatedClick.cpp:47: Comma should be at the beginning of the line in a member initialization list. [whitespace/init] [4] ERROR: Source/WebCore/dom/SimulatedClick.cpp:49: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/dom/SimulatedClick.cpp:51: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 3 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r196598: <http://trac.webkit.org/changeset/196598>