Closed
Bug 1356237
Opened 8 years ago
Closed 8 years ago
Stop using devtools transport modules
Categories
(Remote Protocol :: Marionette, enhancement)
Remote Protocol
Marionette
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(1 file)
We plan to move devtools to an add-on while moving the codebase to github, out of mozilla-central.
So, ideally, marionette shouldn't depend on devtools modules. It depends on transport.js from here:
http://searchfox.org/mozilla-central/source/testing/marionette/server.js#23
loader.loadSubScript("resource://devtools/shared/transport/transport.js");
I think we should just put a copy of it in marionette. Marionette doesn't really depends Devtools. It just happen to reuse its protocol. But I don't think it cares about whatever update we could do to the protocol. It may even be easier for marionette to keep a fork and do not have to update whenever devtools decide to change something to the transport layer.
Assignee | ||
Comment 1•8 years ago
|
||
Comment 2•8 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #0)
> I think we should just put a copy of it in marionette. Marionette doesn't
> really depends Devtools. It just happen to reuse its protocol. But I don't
> think it cares about whatever update we could do to the protocol. It may
> even be easier for marionette to keep a fork and do not have to update
> whenever devtools decide to change something to the transport layer.
It’s worse than that: Marionette implements a custom message format described in https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Protocol on top of transport.js.
I support the idea of copying transport.js over to Marionette if you intend on making devtools an add-on. Feel free to r? me for a review.
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
This patch bundles today's version of the protocol. I had another version of this patch based on an older version of transport.js, before we introduced "bulk data messages". This old version was still a single file. I think it would work if we downgrade PROTOCOL_VERSION to 1 or 2... But I'm not sure that's something we want ?
So, this patch, comes with the two mandatory dependencies of transport.js: stream-utils and packets.
I got rid of any other useless dependencies, like some for special dump() calls.
I also removed some conditional dump completely.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → poirot.alex
Comment 7•8 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> This patch bundles today's version of the protocol. I had another version of
> this patch based on an older version of transport.js, before we introduced
> "bulk data messages". This old version was still a single file. I think it
> would work if we downgrade PROTOCOL_VERSION to 1 or 2... But I'm not sure
> that's something we want ?
Bundling/forking the HEAD of m-c makes sense.
> So, this patch, comes with the two mandatory dependencies of transport.js:
> stream-utils and packets. I got rid of any other useless dependencies, like
> some for special dump() calls. I also removed some conditional dump
> completely.
Thanks for doing this!
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8861159 [details]
Bug 1356237 - Ship a copy of transport module in marionette.
https://reviewboard.mozilla.org/r/133112/#review135938
r+ if you expand the commit message per comment.
::: commit-message-73752:1
(Diff revision 1)
> +Bug 1356237 - Ship a copy of transport module in marionette. r=ato
Please expand on why we are doing this, e.g. that devtools is getting moved into an add-on and what consequences this has.
Attachment #8861159 -
Flags: review?(ato) → review+
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16a917f9afbc
Ship a copy of transport module in marionette. r=ato
Comment 11•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•