Closed
Bug 1153828
Opened 9 years ago
Closed 8 years ago
Merge Marionette:done, Marionette:ok, and Marionette:error messages
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox47 fixed)
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
Details
(Keywords: pi-marionette-server)
Attachments
(1 file)
We currently use three different messages for sending back a response from the listener context to the chrome context when we've made a request: 1. Marionette:ok for responses that executed successfully and didn't return anything (undefined). 2. Marionette:done for responses that executed successfully and returned with a value. 3. Marionette:error for responses with a value where the execution of the command resulted in an error. Having three different messages means setting up and tearing down three individual message listeners for every single request we make to listener space, and makes our code fairly complex because we have to remove the listeners for the other two responses we _didn't_ get. An example of this complexity can be found in the ListenerProxy: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver.js?from=marionette/driver.js#128 To optimise and reduce code complexity we can reduce this to a single message with three different modes: 1. OK: {json: {}} 2. Value: {json: {value: <value>}} 3. Error: {json: {}, objects: {error: <error>}}
Assignee | ||
Updated•9 years ago
|
Keywords: ateam-marionette-server
Assignee | ||
Comment 1•9 years ago
|
||
Updated link: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/proxy.js?from=marionette%2Fproxy.js
Assignee | ||
Updated•9 years ago
|
Mentor: ato
Whiteboard: [good first bug][lang=js]
Comment 2•8 years ago
|
||
Hi Andreas, I'm going to start working on this bug. If I have any questions I'll make sure to ask.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ato
Mentor: ato
Status: NEW → ASSIGNED
Whiteboard: [good first bug][lang=js]
Assignee | ||
Comment 3•8 years ago
|
||
This change reduces the number of content frame script message senders from three to one by imposing a message format. Review commit: https://reviewboard.mozilla.org/r/31805/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31805/
Attachment #8710619 -
Flags: review?(dburns)
Assignee | ||
Comment 4•8 years ago
|
||
Note that the patch does not fix the CPOW issue reported in bug 1238095.
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8710619 [details] MozReview Request: Bug 1153828 - Merge message listeners in AsyncContentSender; r?automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31805/diff/1-2/
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8710619 [details] MozReview Request: Bug 1153828 - Merge message listeners in AsyncContentSender; r?automatedtester Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31805/diff/2-3/
Assignee | ||
Comment 8•8 years ago
|
||
Results are looking a lot better now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a869757032fb&group_state=expanded
Flags: needinfo?(ato)
Assignee | ||
Comment 9•8 years ago
|
||
I filed bug 1242595 to follow up on making AsyncContentSender self-contained and stateless.
Updated•8 years ago
|
Attachment #8710619 -
Flags: review?(dburns) → review+
Comment 10•8 years ago
|
||
Comment on attachment 8710619 [details] MozReview Request: Bug 1153828 - Merge message listeners in AsyncContentSender; r?automatedtester https://reviewboard.mozilla.org/r/31805/#review29167
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/73acd1c066c2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•