Closed Bug 828052 Opened 13 years ago Closed 12 years ago

JS only option might not be working correctly

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: anton, Assigned: anton)

Details

Attachments

(1 file, 1 obsolete file)

There have been reports that non-JS code is slipping through the profiler output.
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)
Err, it should have been "I was setting gJavascriptOnly variable directly..."
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+
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.
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+
\o/\o/\o/
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
How can QA verify this fix?
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: