JS only option might not be working correctly

RESOLVED FIXED in Firefox 21

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: anton, Assigned: anton)

Tracking

Trunk
Firefox 21
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
There have been reports that non-JS code is slipping through the profiler output.
(Assignee)

Comment 1

5 years ago
Created attachment 702610 [details] [diff] [review]
Updated Cleo and fixed the issue with gJavascriptOnly flag not working

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

5 years ago
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+
(Assignee)

Comment 4

5 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

5 years ago
Created attachment 702944 [details] [diff] [review]
Updated Cleo and fixed the issue with gJavascriptOnly flag not working

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

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/99a5aeaf36ea
Whiteboard: [fixed-in-fx-team]
\o/\o/\o/
https://hg.mozilla.org/mozilla-central/rev/99a5aeaf36ea
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
How can QA verify this fix?
(Assignee)

Comment 10

5 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.
You need to log in before you can comment on or make changes to this bug.