Closed
Bug 1153822
Opened 9 years ago
Closed 9 years ago
Adjust Marionette responses to match WebDriver protocol
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox43 fixed)
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
(Blocks 1 open bug)
Details
(Keywords: pi-marionette-server)
Attachments
(1 file, 1 obsolete file)
It's not a goal for the Marionette protocol to be 100% compatible with the WebDriver protocol, but we are currently sending across a lot of superfluous extra bytes, and bringing it closer to WebDriver _in spirit_ will perceptually make them easier to translate in the httpd. Marionette currently returns this as a response with value: {"from":"0","value":null,"status":0,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}"} And this for ok responses: {"from":"0","ok":true} And this for errors: {"from":"0","status":21,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}","error":{"message":"Error loading page, timed out (onDOMContentLoaded)","stacktrace":null,"status":21}} We can: - Drop the "from" field - Drop the "status" field in non-error responses - Drop the "sessionId" field as the local end already knows which session it's expecting a response from - Drop the "ok" field in OK responses This will make us compatible with https://w3c.github.io/webdriver/webdriver-spec.html#processing-model. This will require changes in the Python client and the Node.js client.
Assignee | ||
Updated•9 years ago
|
Keywords: ateam-marionette-server
Assignee | ||
Comment 1•9 years ago
|
||
Also: - Flatten the error response to {"error": "no such element", "message": "Error loading page, timed out (onDOMContentLoaded)", "stacktrace": null}
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
/r/9141 - Bug 1153822: Adjust Marionette responses to match WebDriver protocol Pull down this commit: hg pull -r 7230539f40a09935e6586715867e68b6963863b9 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8608290 -
Flags: review?(jgriffin)
Attachment #8608290 -
Flags: review?(dburns)
Assignee | ||
Comment 3•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47f7c2b3df5a
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8608290 [details] MozReview Request: bz://1153822/ato /r/9141 - Bug 1153822: Adjust Marionette responses to match WebDriver protocol Pull down this commit: hg pull -r c44479a9dee1c284ecec7ac6e0859ea766607301 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 5•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72ab7b4369e4
Assignee | ||
Comment 6•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b45f87f5ffb4
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8608290 [details] MozReview Request: bz://1153822/ato /r/9141 - Bug 1153822: Adjust Marionette responses to match WebDriver protocol Pull down this commit: hg pull -r 1d1956e8769c9bcfb56a4f0d967ba2c85641fc08 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8608290 [details] MozReview Request: bz://1153822/ato /r/9141 - Bug 1153822: Adjust Marionette responses to match WebDriver protocol Pull down this commit: hg pull -r a456c5ef1a3c0b3481efc2106f6debb48b5b4153 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 9•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a47a8d045cc5
Comment 10•9 years ago
|
||
Comment on attachment 8608290 [details] MozReview Request: bz://1153822/ato https://reviewboard.mozilla.org/r/9139/#review7963 Looks good! I didn't attempt to match the Python changes to the JS; hopefully the unit tests are adequate to catch any problems there. Thanks for all the cleanup. ::: testing/marionette/transport/marionette_transport/transport.py:33 (Diff revision 4) > - self.sock (assuming it's open and connected). > + (assuming it's open and connected).""" nit: the docstring delimiter should be on its own line, according to PEP-257. Same with a couple of methods below.
Attachment #8608290 -
Flags: review?(jgriffin) → review+
Comment 11•9 years ago
|
||
Comment on attachment 8608290 [details] MozReview Request: bz://1153822/ato https://reviewboard.mozilla.org/r/9139/#review8053 Ship It!
Attachment #8608290 -
Flags: review?(dburns) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Bug 1153822: Adjust Marionette responses to match WebDriver protocol The Marionette is more expressive than it needs to be and this expressiveness has previously resulted in subtle inconsistencies in the fields returned. Reduce the amount of data sent will make requests faster. Aligning the protocol closer to what WebDriver expects, will reduce the amount of post-processing required in the httpd. Previous to this patch, this is a value response: {"from":"0","value":null,"status":0,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}"} And this for ok responses: {"from":"0","ok":true} And this for errors: {"from":"0","status":21,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}","error":{"message":"Error loading page, timed out (onDOMContentLoaded)","stacktrace":null,"status":21}} This patch drops the "from" and "sessionId" fields and "status" field from non-error responses. It also drops the "ok" field in non-value responses and flattes the error response. r=dburns r=jgriffin
Assignee | ||
Comment 13•9 years ago
|
||
Rebased and addressed the nitpicks pointed out above.
Comment 14•9 years ago
|
||
(In reply to Andreas Tolfsen (:ato) from comment #13) > Rebased and addressed the nitpicks pointed out above. Is this ready to review? The actual MozReview request isn't flagged, so wasn't sure.
Flags: needinfo?(ato)
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #14) > (In reply to Andreas Tolfsen (:ato) from comment #13) > > Rebased and addressed the nitpicks pointed out above. > > Is this ready to review? The actual MozReview request isn't flagged, so > wasn't sure. You already reviewed this patch. The latest push is just amending the nitpicks you highlighted.
Flags: needinfo?(ato)
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8614650 [details] MozReview Request: Bug 1153822: Adjust Marionette responses to match WebDriver protocol Bug 1153822: Adjust Marionette responses to match WebDriver protocol The Marionette is more expressive than it needs to be and this expressiveness has previously resulted in subtle inconsistencies in the fields returned. Reduce the amount of data sent will make requests faster. Aligning the protocol closer to what WebDriver expects, will reduce the amount of post-processing required in the httpd. Previous to this patch, this is a value response: {"from":"0","value":null,"status":0,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}"} And this for ok responses: {"from":"0","ok":true} And this for errors: {"from":"0","status":21,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}","error":{"message":"Error loading page, timed out (onDOMContentLoaded)","stacktrace":null,"status":21}} This patch drops the "from" and "sessionId" fields and "status" field from non-error responses. It also drops the "ok" field in non-value responses and flattes the error response. r=dburns r=jgriffin
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8608290 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8614650 [details] MozReview Request: Bug 1153822: Adjust Marionette responses to match WebDriver protocol Bug 1153822: Adjust Marionette responses to match WebDriver protocol The Marionette is more expressive than it needs to be and this expressiveness has previously resulted in subtle inconsistencies in the fields returned. Reduce the amount of data sent will make requests faster. Aligning the protocol closer to what WebDriver expects, will reduce the amount of post-processing required in the httpd. Previous to this patch, this is a value response: {"from":"0","value":null,"status":0,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}"} And this for ok responses: {"from":"0","ok":true} And this for errors: {"from":"0","status":21,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}","error":{"message":"Error loading page, timed out (onDOMContentLoaded)","stacktrace":null,"status":21}} This patch drops the "from" and "sessionId" fields and "status" field from non-error responses. It also drops the "ok" field in non-value responses and flattes the error response. r=dburns r=jgriffin
Assignee | ||
Comment 19•9 years ago
|
||
Changed the dependency of bug 1168396 because it now adds backwards compatibility support for protocol version 1, which means it can land independently of this change.
Assignee | ||
Comment 20•9 years ago
|
||
Adding bug 1194968 about splitting out the Python bindings changes from this patch.
Depends on: 1194968
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8614650 [details] MozReview Request: Bug 1153822: Adjust Marionette responses to match WebDriver protocol Bug 1153822: Adjust Marionette responses to match WebDriver protocol Introduce protocol version levels in the Marionette server. On establishing a connection to a local end, the remote will return a `marionetteProtocol` field indicating which level it speaks. The protocol level can be used by local ends to either fall into compatibility mode or warn the user that the local end is incompatible with the remote. The protocol is currently also more expressive than it needs to be and this expressiveness has previously resulted in subtle inconsistencies in the fields returned. This patch reduces the amount of superfluous fields, reducing the amount of data sent. Aligning the protocol closer to the WebDriver specification's expectations will also reduce the amount of post-processing required in the httpd. Previous to this patch, this is a value response: {"from":"0","value":null,"status":0,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}"} And this for ok responses: {"from":"0","ok":true} And this for errors: {"from":"0","status":21,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}","error":{"message":"Error loading page, timed out (onDOMContentLoaded)","stacktrace":null,"status":21}} This patch drops the `from` and `sessionId` fields, and the `status` field from non-error responses. It also drops the `ok` field in non-value responses and flattens the error response to a simple dictionary with the `error` (previously `status`), `message`, and `stacktrace` properties, which are now all required. r=jgriffin
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8614650 [details] MozReview Request: Bug 1153822: Adjust Marionette responses to match WebDriver protocol Bug 1153822: Adjust Marionette responses to match WebDriver protocol Introduce protocol version levels in the Marionette server. On establishing a connection to a local end, the remote will return a `marionetteProtocol` field indicating which level it speaks. The protocol level can be used by local ends to either fall into compatibility mode or warn the user that the local end is incompatible with the remote. The protocol is currently also more expressive than it needs to be and this expressiveness has previously resulted in subtle inconsistencies in the fields returned. This patch reduces the amount of superfluous fields, reducing the amount of data sent. Aligning the protocol closer to the WebDriver specification's expectations will also reduce the amount of post-processing required in the httpd. Previous to this patch, this is a value response: {"from":"0","value":null,"status":0,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}"} And this for ok responses: {"from":"0","ok":true} And this for errors: {"from":"0","status":21,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}","error":{"message":"Error loading page, timed out (onDOMContentLoaded)","stacktrace":null,"status":21}} This patch drops the `from` and `sessionId` fields, and the `status` field from non-error responses. It also drops the `ok` field in non-value responses and flattens the error response to a simple dictionary with the `error` (previously `status`), `message`, and `stacktrace` properties, which are now all required. r=jgriffin
Attachment #8614650 -
Flags: review?(jgriffin)
Comment 23•9 years ago
|
||
Comment on attachment 8614650 [details] MozReview Request: Bug 1153822: Adjust Marionette responses to match WebDriver protocol https://reviewboard.mozilla.org/r/9141/#review14663 ::: .hgignore:93 (Diff revision 8) > -^testing/mozharness/logs/ > +.*\.egg-info Looks like a merge conflict here which should be fixed.
Attachment #8614650 -
Flags: review?(jgriffin)
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8614650 [details] MozReview Request: Bug 1153822: Adjust Marionette responses to match WebDriver protocol Bug 1153822: Adjust Marionette responses to match WebDriver protocol Introduce protocol version levels in the Marionette server. On establishing a connection to a local end, the remote will return a `marionetteProtocol` field indicating which level it speaks. The protocol level can be used by local ends to either fall into compatibility mode or warn the user that the local end is incompatible with the remote. The protocol is currently also more expressive than it needs to be and this expressiveness has previously resulted in subtle inconsistencies in the fields returned. This patch reduces the amount of superfluous fields, reducing the amount of data sent. Aligning the protocol closer to the WebDriver specification's expectations will also reduce the amount of post-processing required in the httpd. Previous to this patch, this is a value response: {"from":"0","value":null,"status":0,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}"} And this for ok responses: {"from":"0","ok":true} And this for errors: {"from":"0","status":21,"sessionId":"{6b6d68d2-4ac9-4308-9f07-d2e72519c407}","error":{"message":"Error loading page, timed out (onDOMContentLoaded)","stacktrace":null,"status":21}} This patch drops the `from` and `sessionId` fields, and the `status` field from non-error responses. It also drops the `ok` field in non-value responses and flattens the error response to a simple dictionary with the `error` (previously `status`), `message`, and `stacktrace` properties, which are now all required. r=jgriffin
Attachment #8614650 -
Flags: review?(jgriffin)
Assignee | ||
Comment 25•9 years ago
|
||
https://reviewboard.mozilla.org/r/9141/#review14663 > Looks like a merge conflict here which should be fixed. Well spotted. Fixed in 30b35d141cad.
Comment 26•9 years ago
|
||
Comment on attachment 8614650 [details] MozReview Request: Bug 1153822: Adjust Marionette responses to match WebDriver protocol https://reviewboard.mozilla.org/r/9141/#review14743 Ship It!
Attachment #8614650 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 27•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41207968a718
Assignee | ||
Comment 28•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9db4bae7ad9e
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a81ba282e16
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
•