815757 - refactor script timeouts so only marionette-actors keeps a reference

Status: RESOLVED FIXED

Description

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 1

[Jonathan Griffin (: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.

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

Created attachment 685853 [details] [diff] [review]
patch v1.1

uploading new version with changes for bookkeeping! carrying r+ forward

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

Try: https://tbpl.mozilla.org/?tree=Try&rev=21da5d827a8f

Ash: https://tbpl.mozilla.org/?tree=Ash&rev=7323a14fcad4

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

Green on ash and try.

landed in mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/194aa91f39c3

landed in mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f259c99090

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a5f259c99090

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

I found a spot where the 'async' rename wasn't enforced.

Comment 7

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

pushed change to try: https://tbpl.mozilla.org/?tree=Try&rev=421c0a323ba8

Comment 9

[Jonathan Griffin (:jgriffin)]

Comment on attachment 686877 [details] [diff] [review]
fix v1.0

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

useFinish is a good name!

Comment 10

[Malini Das [:mdas] - Away, not checking bugmail]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb22beb1c571

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

landed in beta: 
https://hg.mozilla.org/releases/mozilla-beta/rev/3c9228f1c893