Last Comment Bug 676594 - Use Orion for displaying script source in the debugger
: Use Orion for displaying script source in the debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Panos Astithas [:past]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 660784
Blocks: minotaur
  Show dependency treegraph
 
Reported: 2011-08-04 11:15 PDT by Panos Astithas [:past]
Modified: 2011-09-15 09:53 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(Almost) Working patch (6.80 KB, patch)
2011-09-14 10:48 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
updated patch (7.32 KB, patch)
2011-09-14 14:56 PDT, Dave Camp (:dcamp)
mihai.sucan: feedback+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2011-08-04 11:15:48 PDT
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 [:past] 2011-09-14 10:48:48 PDT
Created attachment 560186 [details] [diff] [review]
(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) 2011-09-14 14:56:26 PDT
Created attachment 560259 [details] [diff] [review]
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] 2011-09-15 06:11:49 PDT
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 [:past] 2011-09-15 09:53:00 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.