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)
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•12 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•12 years ago
|
||
Err, it should have been "I was setting gJavascriptOnly variable directly..."
Comment 3•12 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•12 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•12 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•12 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 7•12 years ago
|
||
\o/\o/\o/
Comment 8•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Comment 9•12 years ago
|
||
How can QA verify this fix?
Assignee | ||
Comment 10•12 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•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•