[Webkit-unassigned] [Bug 174670] WebDriver: wait until navigation is complete before running new commands and after a click

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=174670

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

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

>>> Source/WebDriver/ChangeLog:15
>>> +        6.3 Processing Model. Step 7. Wait for navigation to complete. If this returns an error return its value and
>> 
>> Nit: step 6
>> 
>> This suggests that we should be waiting for navigations to complete after any command. Should this be extracted out of particular Session methods, then?
> 
> hmm, are you using an old version of the spec or something? This is Section 6.3 and the step is indeed 7. And it suggests we should be waiting for navigations *before* any command, step 8 is "Let response result be the return value obtained by running the remote end steps for command with url variables as arguments." I added this for some of the commands only, though, I don't think we want to wait for any navigation to get the window handles, for example, but we want for sure before getting the page title.

I asked the spec editors, and they haven't updated the W3C copy of the CR in months. You should refer to the latest editor draft here. https://w3c.github.io/webdriver/webdriver-spec.html

They said they'll be updating the one at w3.org next week, and that section numbers are stabilized now (in the editor draft).

Sorry for the confusion! The w3.org copy used to sync on a regular basis.

>>> Source/WebDriver/Session.cpp:954
>>> +    if (m_browsingContext)
>> 
>> Can we rename this local to m_selectedFrameBrowsingContext or something similar? Otherwise it's a bit obscure why there are two browsing context members.
> 
> I think the spec uses top level browsing context and current browsing context, so maybe m_currentBrowsingContext? or maybe using frame explicitly makes the thing easier. I'll do the rename in a follow up patch in any case.

Ok. Either way is better than current.

>>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:400
>>> +    if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(page.pageID()))
>> 
>> Note: per §6.3, substeps 4.3.1–4.3.4, the spec assumes commands are serialized into a queue per session, otherwise one command could cause another one to timeout if they both need to wait for navigation to complete.
>> 
>> I don't think it's the case right now that we queue commands explicitly or implicitly in WebKitDriver, nor in safaridriver. So this could hypothetically be a problem right now, but it seems unlikely in practice.
> 
> Here step 4 doesn't have substeps. 4. Let session id be the corresponding variable from url variables.

See above comment about spec version.

>>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:467
>>> +        if (auto callback = m_pendingNavigationInBrowsingContextCallbacksPerPage.take(frame.page()->pageID()))
>> 
>> When asked to wait on a page navigation, couldn't we use the main frame as the callback key and remove m_pendingNavigationInBrowsingContextCallbacksPerPage? Or does the frame somehow not survive long enough?
> 
> We might not have a main frame yet right after a navigation command, it's created in WebPageProxy::didCreateMainFrame, so we wouldn't have an id to use as the key when adding it to the table.

Gotcha, I must have forgotten fixing that.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170721/0e87bcd8/attachment-0001.html>