[webkit-reviews] review canceled: [Bug 184462] Web Automation: simulated mouse interactions should not be done until associated DOM events have been dispatched : [Attachment 337755] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 16 12:44:58 PDT 2018
Brian Burg <bburg at apple.com> has canceled Brian Burg <bburg at apple.com>'s
request for review:
Bug 184462: Web Automation: simulated mouse interactions should not be done
until associated DOM events have been dispatched
https://bugs.webkit.org/show_bug.cgi?id=184462
Attachment 337755: Patch
https://bugs.webkit.org/attachment.cgi?id=337755&action=review
--- Comment #4 from Brian Burg <bburg at apple.com> ---
Comment on attachment 337755
--> https://bugs.webkit.org/attachment.cgi?id=337755
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=337755&action=review
>> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:82
>> + std::optional<String> message { std::nullopt };
>
> I don't think we need to initialize this, it's already nullopt. It's a bit
weird to have here some members, then the constructors and then another member,
could we move this below the constructors?
I'll clean this up. I just kept appending stuff as I went.
>> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:84
>> + AutomationCommandError(Inspector::Protocol::Automation::ErrorMessage
type)
>
> explicit
Good catch.
>> Source/WebKit/UIProcess/WebPageProxy.cpp:1882
>> + m_mouseEventQueue.append(event);
>
> I don't think we want to append events if the page was closed, I would early
return if !isValid()
OK.
>> Source/WebKit/UIProcess/WebPageProxy.cpp:1894
>> + NativeWebMouseEvent event = m_mouseEventQueue.first();
>
> Do we want to copy the event here? I guess it could be const
NativeWebMouseEvent&
I'll make it take a reference.
>> Source/WebKit/UIProcess/WebPageProxy.cpp:4785
>> + return nullptr;
>
> What happens if there's a mouse move after the mouse down? This will return
nullptr, but current code only resets the currently processed mouse down on
mouse Up. Or is it guaranteed that mouse down is the only event in the queue?
I tested this and nothing seemed broken. I looked into this more after reading
your question. The chain is as follows:
- Enqueue mousedown event
- UIProcess sends mousedown to WebProcess
- EventDispatcher tries to dispatch
-> HTMLSelectElement::menuListDefaultEventHandler
-> RenderMenuList::showPopup
-> WebPopupMenu::show
-> IPC to UIProcess
-> WebPopupMenuProxy(Mac)::showPopupMenu
At this point WebProcess is still processing the mousedown, but now execution
is back in UIProcess. It will spin a nested run loop to handle the NSEvents
being sent to the dropdown. When one is selected, the native control eats the
subsequent mouseup that fires over the opened dialog, so no mouseup is enqueued
into m_mouseEventQueue once it is dismissed. When this returns to WebProcess it
makes a fake event and dispatches it within the DOM only (i.e., not a native
event).
So I don't think there is anything to change here.
>> Source/WebKit/UIProcess/WebPageProxy.cpp:5209
>> + if (auto* automationSession =
process().processPool().automationSession())
>
> else if
OK
More information about the webkit-reviews
mailing list