1062748 - Allow connection-manager to use a custom transport

Status: RESOLVED FIXED

(DevTools :: General, defect)

Comment 1

[Paul Rouget [:paul]]

Attachment: v1

Comment 2

[Paul Rouget [:paul]]

Attachment: v1.1

Ryan, 90% of this patch is re-indenting the comments and replacing utf8 dots with normal dots (it always bugged me to see garbage in my diffs).

Also, I thought I could drop the magic pipe behavior (not setting a host creates a pipe transport) and just use this new transport argument and create the pipe transport by doing: connection.connect(DebuggerServer.connectPipe()), but for some reason, creating the pipe transport too early would break tests. I didn't figure out why (it doesn't look trivial (to me)).

Anyway, this is required for feverdream.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8484021 [details] [diff] [review]
v1.1

Review of attachment 8484021 [details] [diff] [review]:
-----------------------------------------------------------------

I'm surprised that creating the pipe early like you say breaks tests...  but this approach seems fines, so I am not worried about it.

::: toolkit/devtools/client/connection-manager.js
@@ +22,5 @@
>   *
>   * # ConnectionManager
>   *
>   * Methods:
> + *  . Connection createConnection(host, port)

Huh, I've never noticed this issue in the past...  Guess I've never tried non-ASCII chars before.  Some searching suggests the combo of git & less is to blame, since git without a pager does the right thing, but I couldn't find a way to get it working.

I guess I would have used an asterisk, but periods are fine too. :)

@@ +187,5 @@
>        this._client.close();
>      }
>    },
>  
> +  connect: function(transport=null) {

This default value seems unnecessary...  It will just end up undefined if it's not passed in, which works the same for your various if checks.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Allow connection-manager to use a custom transport

Updated to remove the default value.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5fe374b65f97

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://hg.mozilla.org/integration/fx-team/rev/c0748b8ad50f

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c0748b8ad50f