907451 - Implement a Tracer panel

Status: RESOLVED WONTFIX

(DevTools :: General, defect)

Description

[Jake Bailey]

The devtools should have a separate panel for the JavaScript Tracer, pref'd off by default.

Comment 1

[Jake Bailey]

Attachment: Patch

Comment 2

[Jake Bailey]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=3c7adfe70482

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 799696 [details] [diff] [review]
Patch

Moving review to Rob, because this seems more like something he should review.

Comment 4

[Jake Bailey]

Comment on attachment 799696 [details] [diff] [review]
Patch

In light of those try results, I'm requesting feedback instead of review.

Comment 5

[Rob Campbell [:rc] (:robcee)]

ouch. yeah, that's some orange and red. Need to dig into that.

I'm enmeetinged for a bunch of today, but hope to curl up with this a bit later or tomorrow.

Comment 6

[Jake Bailey]

The only failure I can reproduce locally is the browser_toolbox_options timeout (I get three other failures, but they also occur on a clean fx-team build...).

All tools added via the defaultTools list in browser/devtools/main.js get a checkbox under "Default Firefox Developer Tools" in the options panel, and the browser_toolbox_options test expects all of these default tools to be enabled at the beginning of the test. The jstracer pref is turned off by default, but since it's added as a defaultTool, it gets an (unchecked) checkbox in this section. The test times out while waiting for the tool-unregistered event after a click that actually *registers* the jstracer.

I wrote a patch that makes the test not dependent on the initial enablement state of the tools, although I'm not sure if it makes sense to have a "Default Tool" checkbox which is disabled by default.

Comment 7

[Jake Bailey]

Attachment: browser_toolbox_options test patch

Prevents browser_toolbox_options from timing out when a default tool is disabled by default.

Comment 8

[Jake Bailey]

I didn't open a new bug for this patch because I'm not sure it's what we want (having unchecked "Default Tools" is a little weird). I'm also not sure if there should be a checkbox for the tracer yet in the first place--until more functionality lands, Nightly's devtools will have an options checkbox that enables a panel that does nothing.

Comment 9

[Jake Bailey]

Thought about it some more and opened bug 914861.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 799696 [details] [diff] [review]
Patch

Review of attachment 799696 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, Jake. Not sure we even need a separate tracer-model.js as it's pretty light on content, but it won't hurt having it.

This stuff looks fine to me, but we should get a round of feedback from Victor and Dave to check the loader usage.

::: browser/devtools/tracer/Makefile.in
@@ +6,5 @@
> +topsrcdir	= @top_srcdir@
> +srcdir		= @srcdir@
> +VPATH		= @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk

Don't need this boilerplate anymore.

::: browser/devtools/tracer/test/Makefile.in
@@ +5,5 @@
> +DEPTH          = @DEPTH@
> +topsrcdir      = @top_srcdir@
> +srcdir         = @srcdir@
> +VPATH          = @srcdir@
> +relativesrcdir = @relativesrcdir@

we don't need this setup boilerplate anymore. (MPL comment can stay)

::: browser/devtools/tracer/test/browser_tracer_controller.js
@@ +57,5 @@
> +  gPanel.controller.stopTrace("Trace 2", function(response) {
> +    ok(!response.error, "Trace 2 stopped without errors");
> +
> +    testInactive(tearDown.call(null, gTab, function() gTab = gPanel = null));
> +  });

does this call finish() anywhere?

::: browser/devtools/tracer/tracer.xul
@@ +15,5 @@
> +]>
> +
> +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +        macanimationtype="document"
> +        fullscreenbutton="true"

not sure we need or want these two attributes here.

Comment 11

[Rob Campbell [:rc] (:robcee)]

(In reply to Jake Bailey from comment #8)
> I didn't open a new bug for this patch because I'm not sure it's what we
> want (having unchecked "Default Tools" is a little weird). I'm also not sure
> if there should be a checkbox for the tracer yet in the first place--until
> more functionality lands, Nightly's devtools will have an options checkbox
> that enables a panel that does nothing.

I think having a checkbox for it is fine as long as its unchecked by default. It gives people a chance to play with it in an early form at least in nightly and aurora. I might ship it with the checkbox removed in beta and final release.

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 802596 [details] [diff] [review]
browser_toolbox_options test patch

Review of attachment 802596 [details] [diff] [review]:
-----------------------------------------------------------------

in both directions!

Comment 13

[Rob Campbell [:rc] (:robcee)]

some rebasing required around the appmanager landing. Seems to be just jar files affected.

Comment 14

[Rob Campbell [:rc] (:robcee)]

maybe I missed a pref or misunderstood comment 8 (or missed something during my rebase?) but running with this patch applied, I get an empty tracer panel (toolbar's intact, but greyed out, unresponsive buttons). What am I missing to make this go?

Comment 15

[Rob Campbell [:rc] (:robcee)]

I guess this is just the panel shell and doesn't have any UI to go along with it. We shouldn't land this without something to actually go in it.

Comment 16

[Jake Bailey]

This is just the panel shell. I'll open some dependent bugs for UI components and we can land them all at once.

Comment 17

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

Comment on attachment 799696 [details] [diff] [review]
Patch

Review of attachment 799696 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/main.js
@@ +209,5 @@
> +  id: "jstracer",
> +  accesskey: l10n("tracer.accesskey", tracerStrings),
> +  key: l10n("tracer.commandkey", tracerStrings),
> +  ordinal: 7,
> +  modifiers: osString == "Darwin" ? "accel,alt" : "accel,shift",

Oh man, did you find an available keyboard shortcut? :) Do we really want a shortcut for the profiler?

::: browser/devtools/tracer/test/Makefile.in
@@ +7,5 @@
> +srcdir         = @srcdir@
> +VPATH          = @srcdir@
> +relativesrcdir = @relativesrcdir@
> +
> +include $(DEPTH)/config/autoconf.mk

We don't need this include anymore.

@@ +16,5 @@
> +		$(NULL)
> +
> +MOCHITEST_BROWSER_FILES_PARTS = MOCHITEST_BROWSER_TESTS
> +
> +include $(topsrcdir)/config/rules.mk

We don't need this include either.

::: browser/devtools/tracer/test/browser_tracer_controller.js
@@ +20,5 @@
> +    testInactive(startFirstTrace);
> +  });
> +}
> +
> +function testInactive(next=function(){}) {

The default callback param don't seem to be needed here.

@@ +25,5 @@
> +  ok(!gPanel.model.isTracing, "Tracer is idle");
> +  next();
> +}
> +
> +function testActive(next=function(){}) {

Neither here.

::: browser/devtools/tracer/test/head.js
@@ +10,5 @@
> +
> +const {DebuggerServer} =  Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {});
> +
> +registerCleanupFunction(function () {
> +  Services.prefs.clearUserPref(TRACER_ENABLED);

Why do you enable and then disable the tracer during tests? It looks like it's enabled by default isn't it? Moreover, a disabled tool only means "not accessible via the toolbox, or menus, or any other UI" as far as I know. The tests shouldn't touch this pref, the tracer should *work* even if it's disabled.

::: browser/devtools/tracer/tracer-consts.js
@@ +5,5 @@
> +"use strict";
> +
> +module.exports = {
> +  L10N_BUNDLE:        "chrome://browser/locale/devtools/tracer.properties"
> +};

Do we really need an extra file just for this? Loading files means disk activity, and if you're not on an SSD or Flash storage, it might hurt. Granted, just one extra file shouldn't make much of a difference, but I don't see any good reason for having it. You can safely put these constants in tracer-controller.js

::: browser/devtools/tracer/tracer-controller.js
@@ +138,5 @@
> +   * @param {function} callback
> +   *        Called with the RDP response once the trace is started.
> +   */
> +  startTrace: function(name, callback) {
> +    this.traceClient.startTrace(this.traceTypes, name, (response) => {

Nit: you can ditch the parens when there's only one param in an arrow function.

::: browser/devtools/tracer/tracer-model.js
@@ +21,5 @@
> +};
> +
> +TracerModel.prototype = {
> +  /**
> +   * True iff the tracer server is collecting trace data.

typo: iff.

@@ +66,5 @@
> +   */
> +  traceStopped: function(name) {
> +    this.emit("stoppedTrace", { name: name });
> +  }
> +};

I think having a tracer model is too much layering at the moment. All of these functions do is just emit an event or simply wrap _client getters/calls. I would argue it's better to avoid it for now.

::: browser/devtools/tracer/tracer.xul
@@ +18,5 @@
> +        macanimationtype="document"
> +        fullscreenbutton="true"
> +        screenX="4" screenY="4"
> +        width="960" height="480"
> +        persist="screenX screenY width height sizemode">

The tracer will never live in its own window, so you don't need any of these.

::: browser/themes/shared/devtools/tracer.inc.css
@@ +6,5 @@
> +
> +/* Toolbar */
> +
> +.devtools-toolbar {
> +  min-height: 33px;

Is this necessary?

@@ +40,5 @@
> +  box-shadow: 0 1px 0 hsla(210,16%,76%,.15) inset,
> +              0 0 0 1px hsla(210,16%,76%,.15) inset,
> +              0 1px 0 hsla(210,16%,76%,.15);
> +  border: 1px solid hsla(210,8%,5%,.45);
> +  border-radius: 3px;

This radius should be @toolbarbuttonCornerRadius@ on OS X and 3px on other platforms IIRC.

Comment 18

[Girish Sharma [:Optimizer]]

Ctrl + Shift + R reloads the page on Windows.

Comment 19

[Jake Bailey]

(In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> ::: browser/devtools/tracer/test/browser_tracer_controller.js
> @@ +57,5 @@
> > +  gPanel.controller.stopTrace("Trace 2", function(response) {
> > +    ok(!response.error, "Trace 2 stopped without errors");
> > +
> > +    testInactive(tearDown.call(null, gTab, function() gTab = gPanel = null));
> > +  });
> 
> does this call finish() anywhere?

Yes, tearDown (defined in head.js) calls finish.

> ::: browser/devtools/tracer/tracer.xul
> @@ +15,5 @@
> > +]>
> > +
> > +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> > +        macanimationtype="document"
> > +        fullscreenbutton="true"
> 
> not sure we need or want these two attributes here.

Nope, none of the attributes here are needed. Copied them from the debugger before I realized why they were there.

Comment 20

[Dave Camp (:dcamp)]

Comment on attachment 799696 [details] [diff] [review]
Patch

Review of attachment 799696 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/tracer/tracer-consts.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +module.exports = {
> +  L10N_BUNDLE:        "chrome://browser/locale/devtools/tracer.properties"

This file/export doesn't seem to be used? (if it is, then victor's comment applies).

Comment 21

[Jake Bailey]

(In reply to Victor Porof [:vp] from comment #17)
> Oh man, did you find an available keyboard shortcut? :) Do we really want a
> shortcut for the profiler?

I had thought so, but as Optimizer pointed out, this one's not gonna work. I figured it would make sense to have a shortcut, since every other panel has one.

> Why do you enable and then disable the tracer during tests? It looks like
> it's enabled by default isn't it? Moreover, a disabled tool only means "not
> accessible via the toolbox, or menus, or any other UI" as far as I know. The
> tests shouldn't touch this pref, the tracer should *work* even if it's
> disabled.

The frontend tool is not enabled by default, but the tracer server is (and is not controlled by a preference). This test is intended to ensure that the frontend components can communicate with the backend. The "TracerController" is the controller for the UI and is constructed with the panel, so it can't be tested without enabling the pref.

> ::: browser/devtools/tracer/tracer-model.js
> @@ +21,5 @@
> > +};
> > +
> > +TracerModel.prototype = {
> > +  /**
> > +   * True iff the tracer server is collecting trace data.
> 
> typo: iff.

That's intentional--shorthand for "if and only if."

> @@ +66,5 @@
> > +   */
> > +  traceStopped: function(name) {
> > +    this.emit("stoppedTrace", { name: name });
> > +  }
> > +};
> 
> I think having a tracer model is too much layering at the moment. All of
> these functions do is just emit an event or simply wrap _client
> getters/calls. I would argue it's better to avoid it for now.

In future patches the model maintains the list of traces that's displayed in the sidebar and the Trace objects containing all the trace data.

Also, when running several profiles with the same name in Chrome, the sidebar entry for that name expands to show "Run 1", "Run 2", etc. This version of the tracer doesn't do that (a new trace with an already existing name just replaces the old one), but I figured it'd be easier to support that feature in the future if all the trace data was stored in one model object.

> ::: browser/themes/shared/devtools/tracer.inc.css
> @@ +6,5 @@
> > +
> > +/* Toolbar */
> > +
> > +.devtools-toolbar {
> > +  min-height: 33px;
> 
> Is this necessary?

Yes, since the toolbar is split into two, and without this rule they aren't the same height. The toolbar is split so that the sidebar can appear above the visualization when the container is narrow.

Comment 22

[Jake Bailey]

(In reply to Dave Camp (:dcamp) from comment #20)
> Comment on attachment 799696 [details] [diff] [review]
> Patch
> 
> Review of attachment 799696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/tracer/tracer-consts.js
> @@ +4,5 @@
> > +
> > +"use strict";
> > +
> > +module.exports = {
> > +  L10N_BUNDLE:        "chrome://browser/locale/devtools/tracer.properties"
> 
> This file/export doesn't seem to be used? (if it is, then victor's comment
> applies).

It's used in the patches for other UI components, but even then there's only like two constants here. I'll probably just move them to the controller as Victor suggested.

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Not planning on making a whole panel for the tracer anymore, instead it is going to be integrated with the debugger.