828052 - JS only option might not be working correctly

Status: RESOLVED FIXED

Description

[Anton Kovalyov (:anton)]

There have been reports that non-JS code is slipping through the profiler output.

Comment 1

[Anton Kovalyov (:anton)]

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

Comment 2

[Anton Kovalyov (:anton)]

Err, it should have been "I was setting gJavascriptOnly variable directly..."

Comment 3

[Panos Astithas [:past] (please ni?)]

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?

Comment 4

[Anton Kovalyov (:anton)]

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.

Comment 5

[Anton Kovalyov (:anton)]

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+.

Comment 6

[Anton Kovalyov (:anton)]

https://hg.mozilla.org/integration/fx-team/rev/99a5aeaf36ea

Comment 7

[Victor Porof [:vporof][:vp]]

\o/\o/\o/

Comment 8

[Matthew N. [:MattN] (huge backlog; PM if requests are blocking you)]

https://hg.mozilla.org/mozilla-central/rev/99a5aeaf36ea

Comment 9

[Mihaela Velimiroviciu (:mihaelav)]

How can QA verify this fix?

Comment 10

[Anton Kovalyov (:anton)]

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.