[Webkit-unassigned] [Bug 174672] WebDriver: implement page load timeout
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jul 23 02:25:42 PDT 2017
https://bugs.webkit.org/show_bug.cgi?id=174672
--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Brian Burg from comment #5)
> Comment on attachment 315982 [details]
> 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)
Sure.
> > 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."
Ok.
> > 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.
The spec explicitly says the tiemout must be an integer in the range 0 to 2^64 - 1. We are doing the same for the script timeouts. See https://w3c.github.io/webdriver/webdriver-spec.html#set-timeouts
> > 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?
No, I wondered the same while working on this. That's also why added the ASSERT to ensure the timer is not active (which would mean another command is still waiting for navigations).
> > 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/20170723/0fa13197/attachment.html>
More information about the webkit-unassigned
mailing list