Bug 156052 – Web Automation: Add support for script timeouts to the evaluateJavaScriptFunction command

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Timothy Hatcher]

The spec requires a timeout for async scripts. This should be an optional argument on evaluateJavaScriptFunction.

[Radar WebKit Bug Importer]

<rdar://problem/25457151>

[Timothy Hatcher]

Created attachment 275259 [details]
Patch

[Blaze Burg]

Comment on attachment 275259 [details]
Patch

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

r=me

> Source/WebKit2/ChangeLog:11
> +        Also made expectsImplicitCallbackArgument, since it is not required. Added JavaScriptTimeout error.

Nit, I think the sentence is missing 'optional'.

> Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:-398
> -void WebAutomationSession::evaluateJavaScriptFunction(Inspector::ErrorString& errorString, const String& browsingContextHandle, const String* optionalFrameHandle, const String& function, const Inspector::InspectorArray& arguments, bool expectsImplicitCallbackArgument, Ref<EvaluateJavaScriptFunctionCallback>&& callback)

Haha gross, I really need to make the protocol use Optional<bool> someday.

[Timothy Hatcher]

https://trac.webkit.org/r198913

[Joseph Pecoraro]

Comment on attachment 275259 [details]
Patch

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

> Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.js:54
> +        let reportTimeoutError = () => { clearTimeout(timeoutIdentifier); resultCallback(frameID, callbackID, "JavaScriptTimeout", true); }

Nit: These are both statements, so normally we include a semicolon after them.

The clearTimeout here is not needed, since this is the timer firing. It does make things line up oh so well though.

[Timothy Hatcher]

Follow up fixes addressing Joe's comments: https://trac.webkit.org/r198915