Closed Bug 1153822 Opened 5 years ago Closed 4 years ago

Adjust Marionette responses to match WebDriver protocol

Categories

(Testing :: Marionette, defect)

defect
Not set

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.
Also:

  - Flatten the error response to {"error": "no such element", "message": "Error loading page, timed out (onDOMContentLoaded)", "stacktrace": null}
Assignee: nobody → ato
Status: NEW → ASSIGNED
Attached file MozReview Request: bz://1153822/ato (obsolete) —
/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)
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/
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/
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/
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+
Blocks: 1168396
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+
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
Rebased and addressed the nitpicks pointed out above.
(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)
(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)
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
Attachment #8608290 - Attachment is obsolete: true
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
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.
Blocks: webdriver
No longer blocks: 1168396
Depends on: 1168396
Adding bug 1194968 about splitting out the Python bindings changes from this patch.
Depends on: 1194968
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
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 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)
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)
https://reviewboard.mozilla.org/r/9141/#review14663

> Looks like a merge conflict here which should be fixed.

Well spotted.  Fixed in 30b35d141cad.
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+
https://hg.mozilla.org/mozilla-central/rev/2a81ba282e16
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.