Bug 156052

Summary: Web Automation: Add support for script timeouts to the evaluateJavaScriptFunction command
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: Web InspectorAssignee: Timothy Hatcher <timothy>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch bburg: review+, timothy: commit-queue-

Description Timothy Hatcher 2016-03-30 19:57:46 PDT
The spec requires a timeout for async scripts. This should be an optional argument on evaluateJavaScriptFunction.
Comment 1 Radar WebKit Bug Importer 2016-03-30 19:58:01 PDT
<rdar://problem/25457151>
Comment 2 Timothy Hatcher 2016-03-30 20:01:29 PDT
Created attachment 275259 [details]
Patch
Comment 3 BJ Burg 2016-03-31 09:15:28 PDT
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.
Comment 4 Timothy Hatcher 2016-03-31 13:55:04 PDT
https://trac.webkit.org/r198913
Comment 5 Joseph Pecoraro 2016-03-31 13:55:22 PDT
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.
Comment 6 Timothy Hatcher 2016-03-31 14:04:19 PDT
Follow up fixes addressing Joe's comments: https://trac.webkit.org/r198915