<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body><span class="vcard"><a class="email" href="mailto:bburg@apple.com" title="Brian Burg <bburg@apple.com>"> <span class="fn">Brian Burg</span></a>
</span> changed
          <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - WebDriver: implement advance user interactions"
   href="https://bugs.webkit.org/show_bug.cgi?id=174616">bug 174616</a>
          <br>
             <table border="1" cellspacing="0" cellpadding="8">
          <tr>
            <th>What</th>
            <th>Removed</th>
            <th>Added</th>
          </tr>

         <tr>
           <td style="text-align:right;">Attachment #338723 Flags</td>
           <td>review?
           </td>
           <td>review+
           </td>
         </tr></table>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - WebDriver: implement advance user interactions"
   href="https://bugs.webkit.org/show_bug.cgi?id=174616#c5">Comment # 5</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - WebDriver: implement advance user interactions"
   href="https://bugs.webkit.org/show_bug.cgi?id=174616">bug 174616</a>
              from <span class="vcard"><a class="email" href="mailto:bburg@apple.com" title="Brian Burg <bburg@apple.com>"> <span class="fn">Brian Burg</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=338723&action=diff" name="attach_338723" title="Patch">attachment 338723</a> <a href="attachment.cgi?id=338723&action=edit" title="Patch">[details]</a></span>
Patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=338723&action=review">https://bugs.webkit.org/attachment.cgi?id=338723&action=review</a>

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?

<span class="quote">> Source/WebDriver/ChangeLog:21
> +        (WebDriver::Session::virtualKeyForKeySequence): Add more kay codes includes in the spec.</span >

Nit: key

<span class="quote">> Source/WebDriver/Session.cpp:2114
> +        computeInViewCenterPointOfElements(WTFMove(elements), WTFMove(elementOrigins), WTFMove(completionHandler));</span >

Nice.

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

Is duration guaranteed to have a value given these preconditions?

<span class="quote">> Source/WebDriver/Session.cpp:2200
> +                                currentState.origin.x = action.x.value();</span >

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.

<span class="quote">> Source/WebDriver/Session.cpp:2203
> +                            case PointerOrigin::Type::Pointer:</span >

This case can be merged with above case.

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

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

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

I would use return type of

Expected<Vector<Action>, String>

then you can get rid of the errorMessage out parameter.

<span class="quote">> 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));</span >

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


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>