Closed
Bug 815757
Opened 10 years ago
Closed 10 years ago
refactor script timeouts so only marionette-actors keeps a reference
Categories
(Testing :: Marionette, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla20
People
(Reporter: mdas, Assigned: mdas)
References
Details
Attachments
(2 files, 1 obsolete file)
7.64 KB,
patch
|
mdas
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
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 1•10 years ago
|
||
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+
Assignee | ||
Comment 2•10 years ago
|
||
uploading new version with changes for bookkeeping! carrying r+ forward
Attachment #685777 -
Attachment is obsolete: true
Attachment #685853 -
Flags: review+
Assignee | ||
Comment 3•10 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=21da5d827a8f Ash: https://tbpl.mozilla.org/?tree=Ash&rev=7323a14fcad4
Assignee | ||
Comment 4•10 years ago
|
||
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
Updated•10 years ago
|
Whiteboard: [automation-needed-in-beta]
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a5f259c99090
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Assignee | ||
Comment 6•10 years ago
|
||
I found a spot where the 'async' rename wasn't enforced.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
pushed change to try: https://tbpl.mozilla.org/?tree=Try&rev=421c0a323ba8
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
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]
Assignee | ||
Comment 12•10 years ago
|
||
landed in beta: https://hg.mozilla.org/releases/mozilla-beta/rev/3c9228f1c893
Whiteboard: [automation-needed-in-beta][leave-open]
Assignee | ||
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•