Closed
Bug 828052
Opened 11 years ago
Closed 11 years ago
JS only option might not be working correctly
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 21
People
(Reporter: anton, Assigned: anton)
Details
Attachments
(1 file, 1 obsolete file)
88.23 KB,
patch
|
anton
:
review+
|
Details | Diff | Splinter Review |
There have been reports that non-JS code is slipping through the profiler output.
Assignee | ||
Comment 1•11 years ago
|
||
I was setting gJavascriptOnly which wasn't triggering required updates. toggleJavascriptOnly function takes care of that. Try: https://tbpl.mozilla.org/?tree=Try&rev=d9a08a21b5f5
Attachment #702610 -
Flags: review?(rcampbell)
Attachment #702610 -
Flags: review?(past)
Assignee | ||
Comment 2•11 years ago
|
||
Err, it should have been "I was setting gJavascriptOnly variable directly..."
Comment 3•11 years ago
|
||
Comment on attachment 702610 [details] [diff] [review] Updated Cleo and fixed the issue with gJavascriptOnly flag not working Review of attachment 702610 [details] [diff] [review]: ----------------------------------------------------------------- Yes! 1000 times yes! Can we ship it now? ::: browser/devtools/profiler/cleopatra/js/devtools.js @@ +128,5 @@ > + pane.border = "0"; > + pane.cellPadding = "0"; > + pane.cellSpacing = "0"; > + pane.borderCollapse = "collapse"; > + pane.className = "finishedProfilePane"; We prefer to keep the styling in the CSS files, but I'm not sure about the situation with the upstream project, so I'll defer to you and Rob on this. @@ +153,5 @@ > + > + gTreeManager = new ProfileTreeManager(); > + gTreeManager.treeView.setColumns([ > + { name: "sampleCount", title: "Running time" }, > + { name: "selfSampleCount", title: "Self" }, Shouldn't these be localized somewhere? Is this another issue with our upstream dependency? @@ +168,5 @@ > + > + // sampleBar > + > + gPluginView = new PluginView(); > + //currRow = finishedProfilePane.insertRow(4); Forgotten comment?
Attachment #702610 -
Flags: review?(past) → review+
Assignee | ||
Comment 4•11 years ago
|
||
Thanks for the review! (In reply to Panos Astithas [:past] from comment #3) > Comment on attachment 702610 [details] [diff] [review] > Updated Cleo and fixed the issue with gJavascriptOnly flag not working > > Review of attachment 702610 [details] [diff] [review]: > ----------------------------------------------------------------- > > Yes! 1000 times yes! Can we ship it now? > > ::: browser/devtools/profiler/cleopatra/js/devtools.js > @@ +128,5 @@ > > + pane.border = "0"; > > + pane.cellPadding = "0"; > > + pane.cellSpacing = "0"; > > + pane.borderCollapse = "collapse"; > > + pane.className = "finishedProfilePane"; > > We prefer to keep the styling in the CSS files, but I'm not sure about the > situation with the upstream project, so I'll defer to you and Rob on this. Yeah, you're probably right. I copied that from Cleopatra's source but I'll try putting them into CSS. > @@ +153,5 @@ > > + > > + gTreeManager = new ProfileTreeManager(); > > + gTreeManager.treeView.setColumns([ > > + { name: "sampleCount", title: "Running time" }, > > + { name: "selfSampleCount", title: "Self" }, > > Shouldn't these be localized somewhere? Is this another issue with our > upstream dependency? Filed a follow-up: bug 831399. > @@ +168,5 @@ > > + > > + // sampleBar > > + > > + gPluginView = new PluginView(); > > + //currRow = finishedProfilePane.insertRow(4); > > Forgotten comment? Yep.
Assignee | ||
Comment 5•11 years ago
|
||
Moving properties to an external stylesheet didn't work: Cleopatra started to break in unexpected ways. New patch simply fixes comments in a few places, carrying over r+.
Attachment #702610 -
Attachment is obsolete: true
Attachment #702610 -
Flags: review?(rcampbell)
Attachment #702944 -
Flags: review+
Assignee | ||
Comment 6•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/99a5aeaf36ea
Whiteboard: [fixed-in-fx-team]
Comment 7•11 years ago
|
||
\o/\o/\o/
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/99a5aeaf36ea
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Comment 9•11 years ago
|
||
How can QA verify this fix?
Assignee | ||
Comment 10•11 years ago
|
||
You can verify this by generating a profiling report and making sure that all entries there point to JavaScript files i.e. you can click on them and their code opens either in the view-source window or the debugger.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•