[Webkit-unassigned] [Bug 174670] WebDriver: wait until navigation is complete before running new commands and after a click
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jul 23 01:57:32 PDT 2017
--- Comment #13 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Brian Burg from comment #12)
> Comment on attachment 315976 [details]
> View in context:
> >>> 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.
Oh, I had no idea :-( I assumed w3c was up to date. Now I understand the new request queue thing you were talking about. But I don't understand the spec, though, so we queue the request, wait for it to reach the queue and then we dequeue it?
> 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.
No problem, my only concern are the urls to the spec we have already added to the doc, we will have to check them all.
> >>> 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.
Now that I've read the spec, I don't think that's a problem, because the commands are enqueued and dequeued before the wait, so I don't think we can have both waiting at the same time.
> >>> 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...
More information about the webkit-unassigned