[Webkit-unassigned] [Bug 174672] WebDriver: implement page load timeout

[bugzilla-daemon at webkit.org]

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

Brian Burg <bburg at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #315982|                            |review+
              Flags|                            |

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

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

r=me with a few questions

> Source/WebDriver/ChangeLog:8
> +        Handle timeout errors and pass the pageLoadtimeout to waitForNavigationToComplete and all other navigation

Nit: pageLoadTimeout (or just in English)

> Source/WebKit/ChangeLog:8
> +        Always start a timer when waiting for a navigation to complete. When the timer fired callbacks pending to be

"when the timer fires, pending callbacks for navigations are removed and invoked with a timeout error."

> Source/WebKit/ChangeLog:11
> +        not provided the default timeout (300 seconds) is used.

Nice. I really like the stateless/explicit arguments to each command approach, as you may have figured out.

> Source/WebKit/UIProcess/Automation/Automation.json:282
> +                { "name": "pageLoadTimeout", "type": "integer", "optional": true, "description": "The timeout in milliseconds that the navigation is expected to be completed in, otherwise a <code>Timeout</code> error is returned." }

It would be okay to use a "type": "number" here and pass as a double. If we are using int milliseconds elsewhere, then it's probably better to be consistent though.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:52
> +static const Seconds defaultPageLoadTimeout = 300_s;

I love these new duration classes, so much easier to use than std::chrono!

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:407
> +    ASSERT(!m_loadTimer.isActive());

I had to think about this change for a minute, but I think it's okay.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:431
> +    for (auto frameID : m_pendingNavigationInBrowsingContextCallbacksPerFrame.keys()) {

Now I'm wondering how we could have multiple callbacks at all. Is there a way that one command can enqueue multiple pending navigation callbacks, aside from a programming error like calling this protocol method twice? It seems that the duration of two calls to this method shouldn't really overlap inside a command's implementation, and the timer should either be cleared or fire only once at a time. Am I missing something?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:450
> +    waitForNavigationToCompleteOnPage(*page, optionalPageLoadTimeout ? Seconds::fromMilliseconds(*optionalPageLoadTimeout) : defaultPageLoadTimeout, WTFMove(callback));

Yuck, hopefully I'll find time to make optional parameters use std::optional in generated code.

-- 
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/65c7f360/attachment.html>