[webkit-reviews] review granted: [Bug 174616] WebDriver: implement advance user interactions : [Attachment 338723] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 174616: WebDriver: implement advance user interactions
https://bugs.webkit.org/show_bug.cgi?id=174616

Attachment 338723: Patch

https://bugs.webkit.org/attachment.cgi?id=338723&action=review




--- Comment #5 from Brian Burg <bburg at apple.com> ---
Comment on attachment 338723
  --> https://bugs.webkit.org/attachment.cgi?id=338723
Patch

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

r=me Looks good, but I want to move element-based pointer origin into the
protocol rather than doing it driver-side. Do you want to code that up now, or
land this as-is and refactor it later?

> Source/WebDriver/ChangeLog:21
> +	   (WebDriver::Session::virtualKeyForKeySequence): Add more kay codes
includes in the spec.

Nit: key

> Source/WebDriver/Session.cpp:2114
> +	   computeInViewCenterPointOfElements(WTFMove(elements),
WTFMove(elementOrigins), WTFMove(completionHandler));

Nice.

> Source/WebDriver/Session.cpp:2187
> +			   state->setDouble(ASCIILiteral("duration"),
action.duration.value());

Is duration guaranteed to have a value given these preconditions?

> Source/WebDriver/Session.cpp:2200
> +				   currentState.origin.x = action.x.value();

Per our discussion earlier, I think it is incorrect to resolve element
locations on the driver side since they may change in the course of a
multi-tick interaction. Instead, currentState.origin would have a symbolic x/y
if the type is Element. On the WebKit side it would convert that into a layout
computation.

> Source/WebDriver/Session.cpp:2203
> +			       case PointerOrigin::Type::Pointer:

This case can be merged with above case.

> Source/WebDriver/Session.cpp:2216
> +				   state->setDouble(ASCIILiteral("duration"),
action.duration.value());

This seems like the correct way to do what I called out above re:
Action::Type::None.

> Source/WebDriver/WebDriverService.cpp:1834
> +static std::optional<Vector<Action>> processInputActionSequence(Session&
session, JSON::Value& actionSequenceValue, std::optional<String>& errorMessage)

I would use return type of

Expected<Vector<Action>, String>

then you can get rid of the errorMessage out parameter.

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:206
> +	       m_client.simulateMouseInteraction(m_page,
MouseInteraction::Move, b.pressedMouseButton.value_or(MouseButton::NoButton),
b.location.value_or(WebCore::IntPoint()), WTFMove(eventDispatchFinished));

Let's land this separately since it affects safaridriver as well, and don't
want to hold it up on the above questions.