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)

defect
Not set
normal

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>}}
Mentor: ato
Whiteboard: [good first bug][lang=js]
Hi Andreas,

I'm going to start working on this bug. If I have any questions I'll make sure to ask.
Blocks: 1238095
Assignee: nobody → ato
Mentor: ato
Status: NEW → ASSIGNED
Whiteboard: [good first bug][lang=js]
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)
Note that the patch does not fix the CPOW issue reported in bug 1238095.
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/
Try has a lot of failures for WPT and Mn fails.
Flags: needinfo?(ato)
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/
Results are looking a lot better now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a869757032fb&group_state=expanded
Flags: needinfo?(ato)
I filed bug 1242595 to follow up on making AsyncContentSender self-contained and stateless.
Attachment #8710619 - Flags: review?(dburns) → review+
Comment on attachment 8710619 [details]
MozReview Request: Bug 1153828 - Merge message listeners in AsyncContentSender; r?automatedtester

https://reviewboard.mozilla.org/r/31805/#review29167
https://hg.mozilla.org/mozilla-central/rev/73acd1c066c2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Blocks: 1243704
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: