1378480 - Pipe Browser Toolbox stdout and stderr to terminal

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

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

We should pipe the Browser Toolbox process' stdout and stderr up to the parent, so that it reaches the developer's terminal (if there is one).

This should ease finding problems like bug 1377326.

Comment 1

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

Attachment: Bug 1378480 - Dump Browser Toolbox stdout to terminal.

Review commit: https://reviewboard.mozilla.org/r/154600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154600/

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a959288e3f8ba8fa7e6ff2307828ec87b763394a

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8883690 [details]
Bug 1378480 - Dump Browser Toolbox stdout to terminal.

https://reviewboard.mozilla.org/r/154600/#review159682

I was actually planning to add a Subprocess option to just let the process inherit stdout for this, but I guess this is fine in the mean time.

::: devtools/client/framework/ToolboxProcess.jsm:283
(Diff revision 1)
>  
>        dumpn("Chrome toolbox is now running...");
>        this.emit("run", this);
>  
>        proc.stdin.close();
> +      let dumpPipe = async function (pipe) {

Nit: No need for `let`. Just `async function dumpPipe(pipe)` or, if you do use `let`, `let dumpPipe = async pipe => { ... }`

::: devtools/client/framework/ToolboxProcess.jsm:284
(Diff revision 1)
> +        let data = await pipe.readString();
> +        while (data) {
> +          dump(data);
> +          data = await pipe.readString();
> +        }

I generally prefer something like:

    let data;
    while ((data = await pipe.readString())) {
      dump(data);
    }

but I guess it's fine either way.

Comment 4

[Kris Maglione [:kmag]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)
> We should pipe the Browser Toolbox process' stdout and stderr up to the
> parent, so that it reaches the developer's terminal (if there is one).
> 
> This should ease finding problems like bug 1377326.

Note that that problem started before we switched to Subprocess, when the browser toolbox still automatically inherited stdout and stderr, but no-one seemed to notice it until it caused problems.

Comment 5

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

(In reply to Kris Maglione [:kmag] from comment #4)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #0)
> > We should pipe the Browser Toolbox process' stdout and stderr up to the
> > parent, so that it reaches the developer's terminal (if there is one).
> > 
> > This should ease finding problems like bug 1377326.
> 
> Note that that problem started before we switched to Subprocess, when the
> browser toolbox still automatically inherited stdout and stderr, but no-one
> seemed to notice it until it caused problems.

Yeah, it's definitely not a guarantee that action is taken... but it at least makes it a bit easier!

Comment 6

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

Comment on attachment 8883690 [details]
Bug 1378480 - Dump Browser Toolbox stdout to terminal.

https://reviewboard.mozilla.org/r/154600/#review159682

> Nit: No need for `let`. Just `async function dumpPipe(pipe)` or, if you do use `let`, `let dumpPipe = async pipe => { ... }`

Thanks, didn't realize async and arrow funcs could be combined.

> I generally prefer something like:
> 
>     let data;
>     while ((data = await pipe.readString())) {
>       dump(data);
>     }
> 
> but I guess it's fine either way.

Haha, actually I started with that, but DevTools ESLint rules currently disapprove of assignment in loop conditions.

Comment 7

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

Comment on attachment 8883690 [details]
Bug 1378480 - Dump Browser Toolbox stdout to terminal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154600/diff/1-2/

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c77ea8b37bb27403a096cd96ad3b47d9f26d00a9

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd669d55812d
Dump Browser Toolbox stdout to terminal. r=kmag

Comment 10

[Carsten Book [:Tomcat]]

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