Closed Bug 1356237 Opened 8 years ago Closed 8 years ago

Stop using devtools transport modules

Categories

(Remote Protocol :: Marionette, enhancement)

enhancement
Not set
normal

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.
(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.
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: nobody → poirot.alex
(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 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+
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/16a917f9afbc Ship a copy of transport module in marionette. r=ato
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1000814
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: