[webkit-reviews] review granted: [Bug 174670] WebDriver: wait until navigation is complete before running new commands and after a click : [Attachment 315976] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 20 11:37:29 PDT 2017
Brian Burg <bburg at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 174670: WebDriver: wait until navigation is complete before running new
commands and after a click
https://bugs.webkit.org/show_bug.cgi?id=174670
Attachment 315976: Patch
https://bugs.webkit.org/attachment.cgi?id=315976&action=review
--- Comment #9 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
r=me with a few questions. Great work!
> 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?
> Source/WebDriver/ChangeLog:19
> + 14.1 Element Click. Step 10. If the click causes navigation: 1. Run
the post-navigation checks and return its
I don't understand substep 1. How could the post-navigation checks do anything
useful if the click just initiated a load? Ostensibly, the load has likely not
been sent out yet, or hasn't received a response in the time waited during Step
9.
> Source/WebDriver/Session.cpp:947
> + if (!m_toplevelBrowsingContext) {
Nit: add quotations from specific sections/steps of the spec, especially for
error cases.
> 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.
> Source/WebKit/ChangeLog:24
> + all the frames. Complete page operations of it's a main frame, or
frame operations otherwise.
Nit: of->if
> 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?
More information about the webkit-reviews
mailing list