676594 - Use Orion for displaying script source in the debugger

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

The current textarea that holds the script source in the debugger UI should be replaced with a proper editor. This will have to wait until Orion lands in the tree.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: (Almost) Working patch

This patch integrates the source editor component into the debugger. When using the textarea fallback, everything runs smooth. When using orion, the debugger client-server interaction breaks after a while. What seems to happen is that a second client is started somehow after a pause (or the subsequent resume) and then both clients try to talk to the server. 

Mihai, I'd like you to take a look into the way I did the integration and see if I did something stupid that could trigger a bug like this. I don't know how orion works very much at this point, so I'll dive into it afterwards if nothing obvious comes up.

Comment 2

[Dave Camp (:dcamp)]

Attachment: updated patch

We were initializing some stuff in DOMContentLoaded, which (with the addition of the orion iframe) was called multiple times.  I pushed the attached patch, which Mihai still might be interested in looking over.

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 560259 [details] [diff] [review]
updated patch

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

Patch looks fine. Yes, that really sounds like it was the case - DOMContentLoaded is fired twice.

Some comments below.

::: browser/devtools/debugger/content/debugger.js
@@ +336,5 @@
>        .attr("id", aScript.url)
>        .attr("sourceScript", JSON.stringify(aScript))
>        .text(aScript.url)
>        .appendTo("#scripts");
> +    if (!window.editor.getText()) {

This is not fine from the performance perspective. getText() does a lot more than getCharCount(). If you only want to check if there's some text, then please use getCharCount().

@@ +351,5 @@
>        // Notify the chrome code that we need to load a script file.
>        var evt = document.createEvent("CustomEvent");
>        evt.initCustomEvent("Debugger:LoadSource", true, false, aScript.url);
>        document.documentElement.dispatchEvent(evt);
> +      window.editor.setText("Loading...");

This string needs to be localized.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #3)
> Comment on attachment 560259 [details] [diff] [review]
> updated patch
> 
> Review of attachment 560259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks fine. Yes, that really sounds like it was the case -
> DOMContentLoaded is fired twice.
> 
> Some comments below.
> 
> ::: browser/devtools/debugger/content/debugger.js
> @@ +336,5 @@
> >        .attr("id", aScript.url)
> >        .attr("sourceScript", JSON.stringify(aScript))
> >        .text(aScript.url)
> >        .appendTo("#scripts");
> > +    if (!window.editor.getText()) {
> 
> This is not fine from the performance perspective. getText() does a lot more
> than getCharCount(). If you only want to check if there's some text, then
> please use getCharCount().

Nice catch, thanks. Fixed in:

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/9c1e90f046b7

> @@ +351,5 @@
> >        // Notify the chrome code that we need to load a script file.
> >        var evt = document.createEvent("CustomEvent");
> >        evt.initCustomEvent("Debugger:LoadSource", true, false, aScript.url);
> >        document.documentElement.dispatchEvent(evt);
> > +      window.editor.setText("Loading...");
> 
> This string needs to be localized.

Yeah, I know, there's more of that elsewhere. I'll sweep the code for this kind of stuff when we get closer to landing and reviews.