Closed
Bug 676594
Opened 13 years ago
Closed 13 years ago
Use Orion for displaying script source in the debugger
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: past, Assigned: past)
References
Details
Attachments
(1 file, 1 obsolete file)
7.32 KB,
patch
|
msucan
:
feedback+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → past
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•13 years ago
|
||
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)
Comment 2•13 years ago
|
||
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 3•13 years ago
|
||
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+
Assignee | ||
Comment 4•13 years ago
|
||
(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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•