[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