Closed Bug 1356237 Opened 4 years ago Closed 4 years ago

Stop using devtools transport modules

Categories

(Testing :: 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
https://hg.mozilla.org/mozilla-central/rev/16a917f9afbc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.