[webkit-reviews] review granted: [Bug 174672] WebDriver: implement page load timeout : [Attachment 315982] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jul 21 10:32:31 PDT 2017
Brian Burg <bburg at apple.com> has granted review:
Bug 174672: WebDriver: implement page load timeout
https://bugs.webkit.org/show_bug.cgi?id=174672
Attachment 315982: Patch
https://bugs.webkit.org/attachment.cgi?id=315982&action=review
--- 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.
More information about the webkit-reviews
mailing list