refactor script timeouts so only marionette-actors keeps a reference

RESOLVED FIXED in mozilla20

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mdas, Assigned: mdas)

Tracking

Trunk
mozilla20
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 685777 [details] [diff] [review]
patch v1.0

We maintain the script timeout amount in both the marionette-actors and the marionette-listener. We only need to keep track of it once (in the actor) and send it to the listener when needed. This should help out with Bug 814139 as a consequence.

While I was at it, I've add a default timeout that's non-zero as per Bug 811574.
Attachment #685777 - Flags: review?(jgriffin)
Comment on attachment 685777 [details] [diff] [review]
patch v1.0

Review of attachment 685777 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I think this is much more sensible than our previous implementation.  After this lands, I'll undo the patch for bug 814139, which will then be unnecessary.

::: testing/marionette/marionette-actors.js
@@ +764,5 @@
>      else {
> +      this.sendAsync("executeJSScript", { value: aRequest.value,
> +                                          args: aRequest.args,
> +                                          newSandbox: aRequest.newSandbox,
> +                                          callback: aRequest.timeout,

This naming seems confusing, since we're taking a parameter aRequest.timeout and setting it to the callback property, and then taking a different timeout parameter and setting it to the timeout property.

Can we switch aRequest.timeout to aRequest.async (both here and in marionette.py), which I think is a better name for what it does anyway?

::: testing/marionette/marionette-listener.js
@@ +460,3 @@
>  
>    let scriptSrc;
>    if (timeout) {

If you switch the badly-named timeout parameter in marionette-actors to async, let's switch it here too.
Attachment #685777 - Flags: review?(jgriffin) → review+
Blocks: 814139
Created attachment 685853 [details] [diff] [review]
patch v1.1

uploading new version with changes for bookkeeping! carrying r+ forward
Attachment #685777 - Attachment is obsolete: true
Attachment #685853 - Flags: review+
Whiteboard: [automation-needed-in-beta]

Comment 5

6 years ago
https://hg.mozilla.org/mozilla-central/rev/a5f259c99090
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
I found a spot where the 'async' rename wasn't enforced.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 686877 [details] [diff] [review]
fix v1.0

rename 'timeout' to 'async' in marionette-actors.

Also, the use of 'async' in executeWithCallback in marionette-listener is pretty confusing, so I renamed it to 'useFinish' since that's what the parameter essentially means.
Attachment #686877 - Flags: review?(jgriffin)
Comment on attachment 686877 [details] [diff] [review]
fix v1.0

Review of attachment 686877 [details] [diff] [review]:
-----------------------------------------------------------------

useFinish is a good name!
Attachment #686877 - Flags: review?(jgriffin) → review+
landed in mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb22beb1c571

landed in mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/7345783228a3
Whiteboard: [automation-needed-in-beta] → [automation-needed-in-beta][leave-open]
landed in beta: 
https://hg.mozilla.org/releases/mozilla-beta/rev/3c9228f1c893
Whiteboard: [automation-needed-in-beta][leave-open]
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 811574
You need to log in before you can comment on or make changes to this bug.