Closed Bug 676594 Opened 13 years ago Closed 13 years ago

Use Orion for displaying script source in the debugger

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: past, Assigned: past)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: minotaur
Depends on: 660784
Assignee: nobody → past
Status: NEW → ASSIGNED
Attached patch (Almost) Working patch (obsolete) — Splinter Review
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.
Attachment #560186 - Flags: feedback?(mihai.sucan)
Attached patch updated patchSplinter Review
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.
Attachment #560186 - Attachment is obsolete: true
Attachment #560186 - Flags: feedback?(mihai.sucan)
Attachment #560259 - Flags: feedback?(mihai.sucan)
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.
Attachment #560259 - Flags: feedback?(mihai.sucan) → feedback+
(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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: