[webkit-reviews] review granted: [Bug 174616] WebDriver: implement advance user interactions : [Attachment 339961] Updated 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 339961: Updated patch

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




--- Comment #9 from Brian Burg <bburg at apple.com> ---
Comment on attachment 339961
  --> https://bugs.webkit.org/attachment.cgi?id=339961
Updated patch

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

r=me, great work!

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp:273
> +void SimulatedInputDispatcher::run(uint64_t frameID,
Vector<SimulatedInputKeyFrame>&& keyFrames, HashSet<Ref<SimulatedInputSource>>&
inputSources, AutomationCompletionHandler&& completionHandler)

I am not sure whether the spec allows interacting with elements from different
frames in the same interaction; it's a bit vague. The actions commands will
fail if there is not a current browsing context.

EDIT: I think this is correct, because the "get a known element" algorithm only
looks up elements in the current browsing context. I think this might trip up
some users, but let's go with this for now.

> Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:119
> +	   virtual void viewportInViewCenterPointOfElement(WebPageProxy&,
uint64_t frameID, const String& nodeHandle, Function<void
(std::optional<WebCore::IntPoint>, std::optional<AutomationCommandError>)>&&) =
0;

This name is a bit awkward, but I can live with it, assuming it computes the
in-view centre point of the element in viewport coordinate space. If it does
something else then please clarify :)

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:981
> +    // Start at 2 and use only even numbers to not conflict with
m_nextViewportInViewCenterPointOfElementCallbackID.

I'm not a fan of tricks like this. Would it be possible to just have the
existing Automation command use the Client API?