Bug 184603 – Web Automation: add support for mouse/keyboard interaction sequences

• Component: WebDriver
• Status: RESOLVED FIXED

[Blaze Burg]

This is the main WebKit-side functionality to support Actions endpoint.

[Radar WebKit Bug Importer]

<rdar://problem/39421839>

[Blaze Burg]

Created attachment 337922 [details]
WIP - depends on 184462

[Blaze Burg]

Created attachment 338082 [details]
Proposed Fix

[Blaze Burg]

This patch does not pass all tests, but it gets us most of the way towards having a real implementation. Some bugs & radars have already been filed with known issues to be addressed in followup patches.

[Blaze Burg]

Created attachment 338083 [details]
Proposed Fix - depends on 184462

[Carlos Garcia Campos]

Created attachment 338097 [details]
Fix the GTK build

I haven't had time to review the patch in detail yet, I only fixed the build in GTK+.

[Blaze Burg]

Created attachment 338172 [details]
Rebased Patch

[Blaze Burg]

Created attachment 338194 [details]
Fix some builds

[Carlos Garcia Campos]

Comment on attachment 338194 [details]
Fix some builds

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

I haven't had time to read the spec to review this, so I only have a few comments for now

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:44
> +    Seconds result;
> +    for (auto& entry : states)
> +        result = std::max(result, entry.second.duration.value_or(Seconds(0)));

I think we could use std::max_element here, it would be something like:

std::max_element(states.begin(), states.end(), [](const StateEntry& a, const StateEntry& b) {
    return a.second.duration.value_or(0_s) < b.second.duration.value_or(0_s);
})->second.duration.value_or(0_s);

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:69
> +        entries.uncheckedAppend(std::pair<SimulatedInputSource&, SimulatedInputSourceState> { inputSource.get(), SimulatedInputSourceState::emptyState() });

I think we can use { } instead of SimulatedInputSourceState::emptyState()

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:77
> +    , m_keyFrameTransitionDurationTimer(RunLoop::current(), this, &SimulatedInputDispatcher::keyFrameTransitionDurationTimerFired)

I guess this is expected to run always in the main thread, no? I would use RunLoop::main() instead of current.

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:96
> +    m_keyFrameTransitionDurationTimer.stop();

I think the timer is guaranteed to be stopped at this point.

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:137
> +        auto finish = WTFMove(m_keyFrameTransitionCompletionHandler);
> +        finish(std::nullopt);

I'm not sure if move behavior is defined for functions, so I normally use std::exchange with nullptr instead of move for callbacks to be sure.

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:152
> +            auto finish = WTFMove(m_keyFrameTransitionCompletionHandler);
> +            finish(error);

Should we stop m_keyFrameTransitionDurationTimer here?

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:231
> +    for (const Ref<SimulatedInputSource>& inputSource : inputSources)
> +        m_inputSources.add(inputSource.copyRef());

I think the assignment operator will do the right thing copying the HashSet, but I wonder if we really need to copy it. The original set in WebAutomationSession is guaranteed to be alive during the run operation, no?

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:235
> +    m_keyframes.reserveCapacity(keyFrames.size() + 1);
> +    m_keyframes.append(SimulatedInputKeyFrame::keyFrameFromStateOfInputSources(m_inputSources));
> +    m_keyframes.appendVector(WTFMove(keyFrames));

I think it would be simpler to first move the vector and then prepend the keyFrameFromStateOfInputSources

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:253
> +    if (m_keyFrameTransitionDurationTimer.isActive())
> +        m_keyFrameTransitionDurationTimer.stop();

stop does nothing when stopped so we don't need to check isActive before calling stop

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:65
> +    static SimulatedInputSourceState emptyState() { return SimulatedInputSourceState(); }

I don't think we need this, we could simply use { } to create and empty SimulatedInputSourceState when needed.

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:68
> +struct SimulatedInputSource : public RefCounted<SimulatedInputSource> {

Do we really need this to be refcounted? Input sources are only owned by m_inputSources in WebAutomationSession, no? I think we could use std::unique_ptr instead. Any reason why this is a struct instead of a class?

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:85
> +    static Ref<SimulatedInputSource> create(Type type)
> +    {
> +        return adoptRef(*new SimulatedInputSource(type, SimulatedInputSourceState::emptyState()));
> +    }

There's no point in passing an empty SimulatedInputSourceState, the object is already created with an empty one.

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:88
> +    SimulatedInputSource(Type type, SimulatedInputSourceState state)

We don't need to receive a state that is always empty

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1392
> +    if (!m_inputDispatchersByPage.contains(page.pageID()))
> +        m_inputDispatchersByPage.set(page.pageID(), SimulatedInputDispatcher::create(page, *this));
> +
> +    return *m_inputDispatchersByPage.get(page.pageID());

This would be simpler and more efficient using ensure:

return m_inputDispatchersByPage.ensure(page.pageID(), [&] { return SimulatedInputDispatcher::create(page, *this); }).iterator->value;

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1489
> +    }
> +}

GCC complains if we don't add a return here.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1493
> +#if !USE(APPKIT) &&! PLATFORM(GTK)

Why?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1659
> +    }
> +}

We need a return here too to make GCC happy.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1768
> +    SimulatedInputDispatcher& inputDispatcher = inputDispatcherForPage(*page);
> +    if (inputDispatcher.isActive()) {

We don't need the local variable since it's only used for this.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1790
> +    SimulatedInputDispatcher& inputDispatcher = inputDispatcherForPage(*page);
> +    if (inputDispatcher.isActive())
> +        inputDispatcher.cancel();

cancel does nothing when not active so we can simply call cancel here:

inputDispatcherForPage(*page).cancel();

[Alex Christensen]

Comment on attachment 338194 [details]
Fix some builds

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

>> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:137
>> +        finish(std::nullopt);
> 
> I'm not sure if move behavior is defined for functions, so I normally use std::exchange with nullptr instead of move for callbacks to be sure.

Carlos, you're right that the state of m_keyFrameTransitionCompletionHandler is undefined after a WTFMove.  In this case, it's a WTF::CompletionHandler that is moved then called.  I'm pretty sure it'll end up becoming nullptr because of the property of CompletionHandlers that they null themselves out after being called once.  I wouldn't leave it like this, though.  I'd do something like this:
std::exchange(m_keyFrameTransitionCompletionHandler, nullptr)(std::nullopt);
or even safer:
if (auto completionHandler = std::exchange(m_keyFrameTransitionCompletionHandler, nullptr))
    completionHandler(std::nullopt);
else
    ASSERT_NOT_REACHED();

[Blaze Burg]

(In reply to Carlos Garcia Campos from comment #9)
> Comment on attachment 338194 [details]
> Fix some builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338194&action=review
> 
> I haven't had time to read the spec to review this, so I only have a few
> comments for now
> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:44
> > +    Seconds result;
> > +    for (auto& entry : states)
> > +        result = std::max(result, entry.second.duration.value_or(Seconds(0)));
> 
> I think we could use std::max_element here, it would be something like:
> 
> std::max_element(states.begin(), states.end(), [](const StateEntry& a, const
> StateEntry& b) {
>     return a.second.duration.value_or(0_s) < b.second.duration.value_or(0_s);
> })->second.duration.value_or(0_s);

This is gross and doesn't seem to have an upside.

> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:69
> > +        entries.uncheckedAppend(std::pair<SimulatedInputSource&, SimulatedInputSourceState> { inputSource.get(), SimulatedInputSourceState::emptyState() });
> 
> I think we can use { } instead of SimulatedInputSourceState::emptyState()

I kept it as emptyState() to make it obvious what it's trying to do. We may have to initialize emptyState in the future.

> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:77
> > +    , m_keyFrameTransitionDurationTimer(RunLoop::current(), this, &SimulatedInputDispatcher::keyFrameTransitionDurationTimerFired)
> 
> I guess this is expected to run always in the main thread, no? I would use
> RunLoop::main() instead of current.
> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:96
> > +    m_keyFrameTransitionDurationTimer.stop();
> 
> I think the timer is guaranteed to be stopped at this point.
> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:137
> > +        auto finish = WTFMove(m_keyFrameTransitionCompletionHandler);
> > +        finish(std::nullopt);
> 
> I'm not sure if move behavior is defined for functions, so I normally use
> std::exchange with nullptr instead of move for callbacks to be sure.

Good catch. I'm kind of new to this part of C++. I think std::exchange is only needed for members, which we null check in various places. It's overkill for a function parameter that is never used after being moved.

> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:152
> > +            auto finish = WTFMove(m_keyFrameTransitionCompletionHandler);
> > +            finish(error);
> 
> Should we stop m_keyFrameTransitionDurationTimer here?

We could, but it's already done at top-level by finishDispatching() in the error case.

> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:231
> > +    for (const Ref<SimulatedInputSource>& inputSource : inputSources)
> > +        m_inputSources.add(inputSource.copyRef());
> 
> I think the assignment operator will do the right thing copying the HashSet,
> but I wonder if we really need to copy it. The original set in
> WebAutomationSession is guaranteed to be alive during the run operation, no?

I'd prefer a copy, so it isn't overly coupled to what the caller does with the HashSet.

> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:235
> > +    m_keyframes.reserveCapacity(keyFrames.size() + 1);
> > +    m_keyframes.append(SimulatedInputKeyFrame::keyFrameFromStateOfInputSources(m_inputSources));
> > +    m_keyframes.appendVector(WTFMove(keyFrames));
> 
> I think it would be simpler to first move the vector and then prepend the
> keyFrameFromStateOfInputSources

I don't feel strongly either way.

> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:253
> > +    if (m_keyFrameTransitionDurationTimer.isActive())
> > +        m_keyFrameTransitionDurationTimer.stop();
> 
> stop does nothing when stopped so we don't need to check isActive before
> calling stop

ok

> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:65
> > +    static SimulatedInputSourceState emptyState() { return SimulatedInputSourceState(); }
> 
> I don't think we need this, we could simply use { } to create and empty
> SimulatedInputSourceState when needed.
> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:68
> > +struct SimulatedInputSource : public RefCounted<SimulatedInputSource> {
> 
> Do we really need this to be refcounted? Input sources are only owned by
> m_inputSources in WebAutomationSession, no? I think we could use
> std::unique_ptr instead.

In theory, no. I tried this and gave up. It caused headaches trying to use references in SimulatedInputDispatcher, especially in hash sets/maps far away from the owner / unique_ptr.

> Any reason why this is a struct instead of a class?

I usually default to a struct unless its members are modified by instance methods. No other reason. Is there a counterargument?

> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:85
> > +    static Ref<SimulatedInputSource> create(Type type)
> > +    {
> > +        return adoptRef(*new SimulatedInputSource(type, SimulatedInputSourceState::emptyState()));
> > +    }
> 
> There's no point in passing an empty SimulatedInputSourceState, the object
> is already created with an empty one.

See above comment about readability and future initialization.

> 
> > Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:88
> > +    SimulatedInputSource(Type type, SimulatedInputSourceState state)
> 
> We don't need to receive a state that is always empty

Oops.

> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1392
> > +    if (!m_inputDispatchersByPage.contains(page.pageID()))
> > +        m_inputDispatchersByPage.set(page.pageID(), SimulatedInputDispatcher::create(page, *this));
> > +
> > +    return *m_inputDispatchersByPage.get(page.pageID());
> 
> This would be simpler and more efficient using ensure:
> 
> return m_inputDispatchersByPage.ensure(page.pageID(), [&] { return
> SimulatedInputDispatcher::create(page, *this); }).iterator->value;

OK

> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1489
> > +    }
> > +}
> 
> GCC complains if we don't add a return here.

I think a RELEASE_ASSERT_NOT_REACHED also works, because it boils down to __builtin_unreachable().


> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1493
> > +#if !USE(APPKIT) &&! PLATFORM(GTK)
> 
> Why?

The intent of this (when written 2 years ago) was to make the command simply fail with NotImplemented if the platform methods are not implemented. This is better than the scripts running but mysteriously failing because mouse/keyboard interactions aren't actually doing anything.

The actions command isn't guarded this way, so it would be consistent to make it return that error as well. I think it would be preferable to offer platformSupportsMouseActions() and so on and bail out early that way. I'm happy to clean this up once the feature is in the tree.

> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1659
> > +    }
> > +}
> 
> We need a return here too to make GCC happy.

Same as above I guess.

> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1768
> > +    SimulatedInputDispatcher& inputDispatcher = inputDispatcherForPage(*page);
> > +    if (inputDispatcher.isActive()) {
> 
> We don't need the local variable since it's only used for this.

It's used for .isActive() and .run(...)

> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1790
> > +    SimulatedInputDispatcher& inputDispatcher = inputDispatcherForPage(*page);
> > +    if (inputDispatcher.isActive())
> > +        inputDispatcher.cancel();
> 
> cancel does nothing when not active so we can simply call cancel here:
> 
> inputDispatcherForPage(*page).cancel();

[Blaze Burg]

Created attachment 338286 [details]
Address Carlos' comments

This will no longer build against TOT because the patch under it got rolled out. I'll investigate that tonight and resubmit this patch once it's back in tree.

[Carlos Garcia Campos]

Comment on attachment 338194 [details]
Fix some builds

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

>>> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:137
>>> +        finish(std::nullopt);
>> 
>> I'm not sure if move behavior is defined for functions, so I normally use std::exchange with nullptr instead of move for callbacks to be sure.
> 
> Carlos, you're right that the state of m_keyFrameTransitionCompletionHandler is undefined after a WTFMove.  In this case, it's a WTF::CompletionHandler that is moved then called.  I'm pretty sure it'll end up becoming nullptr because of the property of CompletionHandlers that they null themselves out after being called once.  I wouldn't leave it like this, though.  I'd do something like this:
> std::exchange(m_keyFrameTransitionCompletionHandler, nullptr)(std::nullopt);
> or even safer:
> if (auto completionHandler = std::exchange(m_keyFrameTransitionCompletionHandler, nullptr))
>     completionHandler(std::nullopt);
> else
>     ASSERT_NOT_REACHED();

This is what I always do to be sure.

>>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1489
>>> +}
>> 
>> GCC complains if we don't add a return here.
> 
> I think a RELEASE_ASSERT_NOT_REACHED also works, because it boils down to __builtin_unreachable().

Right, either ASSERT + return or RELEASE_ASSERT.

>>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1493
>>> +#if !USE(APPKIT) &&! PLATFORM(GTK)
>> 
>> Why?
> 
> The intent of this (when written 2 years ago) was to make the command simply fail with NotImplemented if the platform methods are not implemented. This is better than the scripts running but mysteriously failing because mouse/keyboard interactions aren't actually doing anything.
> 
> The actions command isn't guarded this way, so it would be consistent to make it return that error as well. I think it would be preferable to offer platformSupportsMouseActions() and so on and bail out early that way. I'm happy to clean this up once the feature is in the tree.

I mean why this particular change, not the platform ifdefs

>>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1768
>>> +    if (inputDispatcher.isActive()) {
>> 
>> We don't need the local variable since it's only used for this.
> 
> It's used for .isActive() and .run(...)

Oh!, indeed.

[Carlos Garcia Campos]

I've been reading the spec, and I'm a bit confused. I don't know exactly what parts are supposed to be done by the driver and what by the automation session. References to the spec in the implementation would help I guess. At first I thought Step 4 (Dispatch actions) is what automation is doing, but I see it expects a pressed state, so the driver has to process something as well, right? Could you clarify what parts of the spec are done by each part (driver, automation)? Do the driver keep the input sources list and input source states table?

[Blaze Burg]

(In reply to Carlos Garcia Campos from comment #14)
> I've been reading the spec, and I'm a bit confused. I don't know exactly
> what parts are supposed to be done by the driver and what by the automation
> session. References to the spec in the implementation would help I guess. At
> first I thought Step 4 (Dispatch actions) is what automation is doing, but I
> see it expects a pressed state, so the driver has to process something as
> well, right? Could you clarify what parts of the spec are done by each part
> (driver, automation)? Do the driver keep the input sources list and input
> source states table?

The driver side is responsible for parsing the JSON, validating source type vs action properties, and transposing the per-input source action list into a per-tick action list. Rather than an action list, it's a list of states and the events/actions to go from one state to the next are inferred by SimulatedInputDispatcher. This abstracts away from the Pointer Events terminology the spec uses, and allows WebKit to dispatch whatever events it needs to (i.e., touch events will differ from what pointer events would do).

Most of the "process a foo action" stuff happens in the driver. I'll try to add relevant spec sections in the WebKit side; for some reason I didn't think to add them but I did put them in the Safari patch.

For practical reasons, there are only a fixed set of input sources right now. At least on Mac, you can't independently address >1 mouse or >1 keyboard from web content. So, there are singleton input sources. In the protocol these still have symbolic names provided by the REST API client, and the WebKit side unswizzles them into Ref<SimulatedInputSource>.

One thing I would point out is that the driver side keeps track of the last used states for each input source, so that it can compute the initial state correctly. Each state is computed as a copy of the previous state for the input source, plus the effect of the requested action (i.e., mousedown/mouseup -> pressedMouseButtons state changes, mouse move -> location changes). It is kind of annoying to track the state in the driver and in the browser, but it's necessary given the state transition-based design.

[Blaze Burg]

(In reply to Brian Burg from comment #15)
> (In reply to Carlos Garcia Campos from comment #14)
> > I've been reading the spec, and I'm a bit confused. I don't know exactly
> > what parts are supposed to be done by the driver and what by the automation
> > session. References to the spec in the implementation would help I guess. At
> > first I thought Step 4 (Dispatch actions) is what automation is doing, but I
> > see it expects a pressed state, so the driver has to process something as
> > well, right? Could you clarify what parts of the spec are done by each part
> > (driver, automation)? Do the driver keep the input sources list and input
> > source states table?
> 
> The driver side is responsible for parsing the JSON, validating source type
> vs action properties, and transposing the per-input source action list into
> a per-tick action list. Rather than an action list, it's a list of states
> and the events/actions to go from one state to the next are inferred by
> SimulatedInputDispatcher. This abstracts away from the Pointer Events
> terminology the spec uses, and allows WebKit to dispatch whatever events it
> needs to (i.e., touch events will differ from what pointer events would do).
> 
> Most of the "process a foo action" stuff happens in the driver. I'll try to
> add relevant spec sections in the WebKit side; for some reason I didn't
> think to add them but I did put them in the Safari patch.
> 
> For practical reasons, there are only a fixed set of input sources right
> now. At least on Mac, you can't independently address >1 mouse or >1
> keyboard from web content. So, there are singleton input sources. In the
> protocol these still have symbolic names provided by the REST API client,
> and the WebKit side unswizzles them into Ref<SimulatedInputSource>.
> 
> One thing I would point out is that the driver side keeps track of the last
> used states for each input source, so that it can compute the initial state
> correctly. Each state is computed as a copy of the previous state for the
> input source, plus the effect of the requested action (i.e.,
> mousedown/mouseup -> pressedMouseButtons state changes, mouse move ->
> location changes). It is kind of annoying to track the state in the driver
> and in the browser, but it's necessary given the state transition-based
> design.

Also note that some of the smaller points, such as element-relative coordinates, are not yet implemented. For that, the location field will be an optional 'InputSourceLocation' with element handle, x, y, etc. I can add some placeholder bugs for those tasks if I remember them.

[Blaze Burg]

Created attachment 338363 [details]
For EWS

[Blaze Burg]

Created attachment 338371 [details]
For EWS

[Blaze Burg]

Created attachment 338379 [details]
For EWS

[Blaze Burg]

Created attachment 338383 [details]
For EWS

[Blaze Burg]

Committed r230830: <https://trac.webkit.org/changeset/230830>