929349 - Integrate a tracing debugger into our existing debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

We should have a tracing debugging mode that lets you see calls, returns, exceptions, etc.

Comment 1

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

Attachment: un-rebased WIP patch from summit

Comment 2

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

TODO

* Needs to be localize-able

* Needs keyboard nevigation

* Should use the widgets framework

Comment 3

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> 
> * Needs keyboard nevigation

Theoretically, by using the WidgetMethods, you'll get keyboard navigation for free (modulo bugs in the framework): https://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#592

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Nick's WIP rebased

Rebased and converted the Unicode characters that were garbled in the patch upload to HTML entities and Unicode escape sequences. Also made the same style changes to Linux and Windows style sheets.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v3

Fixed chrome debugging which was broken in the previous versions of the patch. Still need to hide the tracer UI in the browser debugger or add a global tracer actor and use it.

Comment 6

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

Cannot for my life figure out how to interdiff with bugzilla/splinter. What was causing the chrome debugging breakage?

Also, thanks for taking this patch to the finish line!

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

IIRC it needed a this.client instead of aClient in debugger-controller.js.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v4

Fix a bug and add a devtools.debugger.tracer pref that is disabled by default and hides any Tracer UI until we're ready to flip the switch.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Forgot to mention that all debugger tests pass with this patch.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Panos Astithas [:past] from comment #5)
> Created attachment 824221 [details] [diff] [review]
> WIP v3
> 
> Fixed chrome debugging which was broken in the previous versions of the
> patch. Still need to hide the tracer UI in the browser debugger or add a
> global tracer actor and use it.

Obviously I'd prefer a global actor. I think if its not done here it won't be taken care of for a while, I'd really appreciate if you could make it happen :)

Comment 11

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

Note that TracerView.prototype.push is currently thrashing and causing reflows to happen every single time we add a frame enter/exit log message. I'm sad to say that the stuff that is causing the thrashing doesn't even work :(

diff --git a/browser/devtools/debugger/debugger-panes.js b/browser/devtools/debugger/debugger-panes.js
index 31628d4..d3d5ba1 100644
--- a/browser/devtools/debugger/debugger-panes.js
+++ b/browser/devtools/debugger/debugger-panes.js
@@ -1310,23 +1310,23 @@ TracerView.prototype = {
 
     const view = this._createView(aItem);
     this._elemsToData.set(view, aItem);
 
     if (type === "call") {
       this._indent++;
     }
 
-    const { offsetHeight, scrollTop, scrollHeight } = this._traces;
-    const atBottom = offsetHeight + scrollTop >= scrollHeight;
-    console.log('atBottom', atBottom);
     this._traces.appendChild(view);
-    if (atBottom) {
-      this._traces.scrollTop = this._traces.scrollHeight;
-    }
   }
 };
 
 /**
  * Utility functions for handling sources.
  */
 let SourceUtils = {
   _labelsCache: new Map(), // Can't use WeakMaps because keys are strings.

Comment 12

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> Note that TracerView.prototype.push is currently thrashing and causing
> reflows to happen every single time we add a frame enter/exit log message.
> I'm sad to say that the stuff that is causing the thrashing doesn't even
> work :(

I haven't looked much at the patch, but:
* Might want to batch items in the frontend when pushing, if you use the widget methods. (see the 'staged' flag)
* Might also want to take a look at how SideMenuWidget is doing this, trying to be sure a reflow won't be caused if not necessary
* Lastly, maybe bug 876271 is worth visiting for making all of this more elegant?

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v5

In this version:

* applied the fix from comment 11
* completed l10n
* added tooltips
* used commands for buttons
* added method comments
* fixed a bug caused by a typo
* fixed a XUL warning
* fixed a CSS glitch in the tracing toggle

Next up:

* widgetification

Comment 14

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

(In reply to Victor Porof [:vp] from comment #12)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> > Note that TracerView.prototype.push is currently thrashing and causing
> > reflows to happen every single time we add a frame enter/exit log message.
> > I'm sad to say that the stuff that is causing the thrashing doesn't even
> > work :(
> 
> I haven't looked much at the patch, but:
> * Might want to batch items in the frontend when pushing, if you use the
> widget methods. (see the 'staged' flag)

I think we need to go even further and both stage items AND use DevToolsUtils.yieldingEach[0] when we are adding them. Perhaps this should be added to the widget methods?

[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.js#l105

> * Might also want to take a look at how SideMenuWidget is doing this, trying
> to be sure a reflow won't be caused if not necessary
> * Lastly, maybe bug 876271 is worth visiting for making all of this more
> elegant?

+1

Comment 15

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #14)
> 
> I think we need to go even further and both stage items AND use
> DevToolsUtils.yieldingEach[0] when we are adding them. Perhaps this should
> be added to the widget methods?
> 
> [0]
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/DevToolsUtils.
> js#l105
> 

I like that.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v6

This version uses the SideMenuWidget under the hood. This fixes the keybinding issues and has a more consistent look with the rest of the debugger. I also used the netmonitor icon for the trace button as it looks somewhat like the tracer output. I expect we will get a dedicated icon at some point. There still remain a few UI glitches here and there, but I'd like to get some feedback on the approach so far.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 828853 [details] [diff] [review]
WIP v6

Victor I'm explicitly asking for feedback about the SideMenuWidget embedding and Nick let me know how it feels.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Screenshot of v6

This is what it looks like.

Comment 19

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

Nice job! I think this would look better without the borders between logs. It makes it just a bit too crowded IMO. About to apply the patch and play...

Comment 20

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

Comment on attachment 828853 [details] [diff] [review]
WIP v6

It feels like performance has gotten even worse :(

I had to force kill the build on the CodeMirror demo site.

Also, you can no longer select text from the trace and copy paste it.

Comment 21

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

Comment on attachment 828853 [details] [diff] [review]
WIP v6

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1438,5 @@
> +      "throw",
> +      "yield"
> +    ], this._trace, ({ error }) => {
> +      if (error) {
> +        DevToolsUtils.reportException(error);

This was probably my bad (sorry!), but |reportException|'s first argument is "who" aka "startTracing". Second argument is the actual error.

Should probably double check the rest of the patch for this mistake as well.

@@ +1472,5 @@
> +      location: location,
> +      parameterNames: parameterNames,
> +      arguments: args,
> +      frameId: item.id
> +    });

We should stage these traces and commit every 50ms or so.

::: browser/devtools/debugger/debugger-panes.js
@@ +1207,5 @@
> +      if (name.contains(query)) {
> +        elem.target.removeAttribute("hidden");
> +      } else {
> +        elem.target.setAttribute("hidden", true);
> +      }

I believe this can be abstracted away a bit by using the |filterContents| widget method.

@@ +1279,5 @@
> +    nameNode.setAttribute("value", name);
> +
> +    if (parameterNames) {
> +      const syntax = (p) => {
> +        const el = document.createElement("label");

If you go back to using HTML nodes, we can select and copy text again.

Comment 22

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

Comment on attachment 828853 [details] [diff] [review]
WIP v6

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

::: browser/devtools/debugger/debugger-panes.js
@@ +1039,5 @@
> +   */
> +  initialize: function() {
> +    dumpn("Initializing the TracerView");
> +
> +    this.widget = new SideMenuWidget(document.getElementById("tracer-traces"));

Before looking at the rest of the patch, I don't think you should be using the SideMenuWidget. It has too many options and an internal structure too heavy for this particular use case.

I think all of this TracerView is mostly inspired by the Network Monitor, where customizing the items was ok enough (since the items weren't inserted sufficiently repeatedly fast for it to matter). However, in this particular case, it's important.

When Nick was saying that it'd be a good idea to reuse the WidgetMethods, I think he was referring to creating a new widget, specifically designed for tracer logs, while taking advantage of the public methods that allow for querying items, batching insertions etc.

See the description here [1], as well as how everything works [2].

[1] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#l562
[2] https://gist.github.com/victorporof/5749386

You'll need to define a new TracerLogsWidget (or something), that implements the api described at [1]. It's internal UI shouldn't be customizable to the extent the SideMenuWidget is, to skip creting redundant nodes.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Thank you both for the feedback, this is exactly what I was looking for.

Nick, how important do you think copying and pasting values from the traces pane is? I can't think of any case where I would have used this particular representation. Would a context menu to copy lines work?

Comment 24

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

(In reply to Panos Astithas [:past] from comment #23)
> Thank you both for the feedback, this is exactly what I was looking for.
> 
> Nick, how important do you think copying and pasting values from the traces
> pane is? I can't think of any case where I would have used this particular
> representation. Would a context menu to copy lines work?

I don't think you want to copy single lines at a time, but whole call trees. Imagine filing a bug report and including the relevant part of the call tree. I think this is fairly important; don't see a reason not to do it. Shouldn't be hard, its just replacing |createElement| with |createElementNS| and maybe using flexbox css rules instead of hbox/vbox in xul.

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v7

A FastListWidget WIP.

Comment 26

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

Attachment: tracing-perf.patch

This is on top of the patch from bug 934163 and WIP 7 from here and also resurrects the server side buffering.

After profiling, the largest bottle neck is adding items to the dom. A lot of this could be alleviated with document fragments, but the widget methods don't support them. Victor, how hard would it be to have .commit() use a document fragment? It seems like it would be not easy because widget methods care about sorting, but we are always appending here.

The second biggest bottleneck was just allocating the dom nodes to be appended. Since we are capping the number of logs at 1000, we could pre-allocate dom nodes in an object pool. There would be some minor kinks to work out with regards to the sub dom nodes that are only visible on enter frame traces but not exit frame traces.

The xpcshell tests aren't passing yet because I haven't rebased the test changes needed for trace packet buffering.

Comment 27

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

Attachment: WIP v8

I split out the buffering back into bug 934163. This is the frontend code for the tracer UI only.

Comment 28

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #26)
> Created attachment 8335710 [details] [diff] [review]
> tracing-perf.patch
> 
> This is on top of the patch from bug 934163 and WIP 7 from here and also
> resurrects the server side buffering.
> 
> After profiling, the largest bottle neck is adding items to the dom. A lot
> of this could be alleviated with document fragments, but the widget methods
> don't support them. Victor, how hard would it be to have .commit() use a
> document fragment? It seems like it would be not easy because widget methods
> care about sorting, but we are always appending here.
> 

The sorting shouldn't matter at all, because the DOM nodes aren't even created at that point. Sorting is done on data only, before creating the corresponding nodes, so using a document fragment is possible regardless.

As for how exactly this would look like *in code*, let's talk about it on vidyo :) Long story short though, the FastListWidget could maintain an internal document fragment, adding a node to it every time insertItemAt() is called. After commit() finishes, the fragment is flushed to the view's node.

Btw, when a fragment is flushed, it is emptied, so there's no reason to create multiple ones.

> The second biggest bottleneck was just allocating the dom nodes to be
> appended. Since we are capping the number of logs at 1000, we could
> pre-allocate dom nodes in an object pool. There would be some minor kinks to
> work out with regards to the sub dom nodes that are only visible on enter
> frame traces but not exit frame traces.

Having a predefined structure and using cloneNode can also help a bunch. But this should also be the job of the FastListWidget.

Ping me on IRC.

Comment 29

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

Attachment: WIP v9

* Append all staged items in a document fragment

* Use cloneNode in FastListWidget (but not in addTrace yet)

* If we get more traces than TracerView.MAX_TRACES in onTraces in the controller, slice them down to the last MAX_TRACES number of traces instead of adding and removing one at a time in the view. This was the biggest gain in performance!! I'm also emptying the TracerView if we get more than MAX_TRACES which also helps perf a ton, but unfortunately causes some UI flickering. If we could stage emptying / removing items the way we stage adding items, this would go away.

TLDR: THE MOTHER F'ING TRACER IS NOT SLOW AS F*** ANYMORE

Comment 30

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

Stealing this back from you, Panos.

Comment 31

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

Attachment: Screen Shot 2013-12-09 at 1.31.54 PM.png

Uploading a screen shot of the latest work I have. Patch to follow.

Darrin: We need an icon for the tracer button. The button toggles recording the logging of every call/return/exception of JS execution on the page. The button is next to the stepping and resuming buttons. Currently, we are re-using the network monitor icon, but it would be awesome if this had its own icon.

Comment 32

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

Attachment: WIP v10

Just gonna write a bunch of tests now, and then I will flag for review.

Comment 33

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

Are you sure it's still ok to have the Traces tab in that sidebar? It is mutually exclusive with the Call Stack tab, while not mutually exclusive with the Sources tab. I think users might find it confusing, and a little bit crowded, but I may be wrong.

I think it's worth discussing the UI placement/arrangement/behavior a bit. The more I think about it, the more it feels a bit "unnatural".

Comment 34

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

(In reply to Victor Porof [:vp] from comment #33)
> Are you sure it's still ok to have the Traces tab in that sidebar? It is
> mutually exclusive with the Call Stack tab, while not mutually exclusive
> with the Sources tab. I think users might find it confusing, and a little
> bit crowded, but I may be wrong.

It's not mutually exclusive with anything. You can still pause and start stepping while you are tracing.

> I think it's worth discussing the UI placement/arrangement/behavior a bit.
> The more I think about it, the more it feels a bit "unnatural".

I see two main benefits of having it in the left:

1. You can use the variables view concurrently with the trace logs. This is important because you want to be able to quickly see all of the parameters passed to a traced frame enter or values thrown or returned from a traced frame exit.

2. All of the source navigating views will be on the left. You can navigate the source view by clicking on a source in the list of all sources, clicking on a frame in the list of stack frames when paused, or by clicking on a trace log when tracing. It is consistent.

That said I am open to ideas and alternatives.

Comment 35

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #34)
> (In reply to Victor Porof [:vp] from comment #33)
> > Are you sure it's still ok to have the Traces tab in that sidebar? It is
> > mutually exclusive with the Call Stack tab, while not mutually exclusive
> > with the Sources tab. I think users might find it confusing, and a little
> > bit crowded, but I may be wrong.
> 
> It's not mutually exclusive with anything. You can still pause and start
> stepping while you are tracing.
> 

Conceptually, it is! For example, the Call Stack tab has no use while tracing, and the fact that it is accessible makes things confusing.

> I see two main benefits of having it in the left:
> 
> 1. You can use the variables view concurrently with the trace logs. This is
> important because you want to be able to quickly see all of the parameters
> passed to a traced frame enter or values thrown or returned from a traced
> frame exit.

I agree with this, however we might want to revisit the way this information is presented in the variables view. For example, having a "Return" scope with a <return> variable just to show the returned value is clunky. But that's easy to fix.

> 2. All of the source navigating views will be on the left. You can navigate
> the source view by clicking on a source in the list of all sources, clicking
> on a frame in the list of stack frames when paused, or by clicking on a
> trace log when tracing. It is consistent.
> 

Not true, at least not after bug 918416 and bug 786069, both of which make a lot of sense. With the Tracer tab, we'll no longer have a clear separation for the sources interaction and debugger "modes".

Consider this:
We now have a "resumed" mode, in which only the Sources tab and Events tab are useful.
We now have a "paused" mode, in which all the tabs are useful.
We will have a "tracing" mode, in which the Tracer and Variables tabs are useful. The other tabs are accessible, but I'm not sure if they'll ever get used, since they aren't usually present in Tracers. The Call Stack tab makes no sense in this mode.

> That said I am open to ideas and alternatives.

Yeah, I am too :) I'm still thinking about this and trying to understand what the use flow in the debugger will be and how to make it feel natural.

Having said that, if we're introducing "modes" to the debugger (and IMHO it's hard to argue that Tracing doesn't actually add a "mode", at least from a user perspective if not implementation-wise), we should treating things as such.

How about hiding the Call Stack and Events tabs, while tracing, and showing the Tracer tab only after enabling Tracing. That is, have a single on/off toggle button for Tracing that adapts the UI accordingly to hide parts of the UI that don't make sense in this mode.

The fact that we can start tracing in 2 ways via 2 separate buttons, while being able to close it only by 1 button made me initially wonder "how do I turn this off?".

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

I'm not strongly against a modal tracing workflow that hides other widgets, but in general I've found that it's not a good idea to constrain user interactions, when the implementation allows them. Who knows why someone would need to observe the registered event listeners while tracing, but if it doesn't cause any trouble why forbid it? Same thing for the stack trace.

The two buttons to start tracing might be overkill and it might be a good idea to use the profiler's "record" icon for the single tracer button. The usual workflow of the tracer is similar to a profiler recording, so we could hint at that by using the same icon.

Comment 37

[Darrin Henein [:darrin]]

Attachment: js-tracer icon @2x

Does this icon make sense?

Comment 38

[Darrin Henein [:darrin]]

Attachment: js-tracer icon

Comment 39

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

(In reply to Darrin Henein [:darrin] from comment #37)
> Created attachment 8346036 [details]
> js-tracer icon @2x
> 
> Does this icon make sense?

LGTM! An earlier iteration of this patch was actually using the unicode version of that! ⇄

Yours looks much better though :) Thanks!

Comment 40

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

Attachment: WIP 11

+ tests

not using the new icons yet

Comment 41

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

Attachment: tracer-ui.patch

Ready for review! This patch leaves the tracer pref'd off by default so there isn't too much risk to landing this now even if we still want to do large overhauls in the near future.

About to file follow ups for:

* Maintaining scroll at the bottom of the logs
* High DPI icon
* HTML-ify / make the trace tree copy & paste
* Remove the extra hbox wrapper around each log element
* Ability to start/stop tracing from the console object
* Source map support for the tracer actor
* Black box support for the tracer actor

https://tbpl.mozilla.org/?tree=Try&rev=5e03f43a0c08

Comment 42

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

Comment on attachment 8346823 [details] [diff] [review]
tracer-ui.patch

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

Quick drive-by, hope you don't mind.

I filed some bugs as well, please take a look at them as well. None of them are necessarily important while the tracer is disabled (maybe just bug 949933, but it's minimal), but I strongly think they should block an enabled Tracer.

::: browser/devtools/debugger/debugger-controller.js
@@ +229,5 @@
>        target.on("navigate", this._onTabNavigated);
>        target.on("will-navigate", this._onTabNavigated);
>  
> +      if (!client) {
> +        DevToolsUtils.reportException("No client found!");

I don't think this check is necessary anymore here, in _startDebuggingTab or _startChromeDebugging or _startTracingTab, now that we have targets.

Furthermore, this will make the connect function return undefined instead of a promise. I vote for removing this superfluous conditional.

::: browser/devtools/debugger/debugger-panes.js
@@ +1074,5 @@
> + *
> + * @param nsIDOMNode aNode
> + *        The element associated with the widget.
> + */
> +function FastListWidget(aNode) {

I think it'd be better to have FastListWidget in a different file inside /shared, to keep debugger-panes.js under 3500 lines :)

@@ +1081,5 @@
> +  this._parent = aNode;
> +  this._fragment = this.document.createDocumentFragment();
> +  // Create a new unique time out name for each instance of FastListWidget so
> +  // that they can flush their buffered items independantly from one another.
> +  this._timeOutName = "fast-list-widget-" + String(Math.random()).slice(2);

Collisions are rare, but possible. This is just a little bit fragile.

@@ +1085,5 @@
> +  this._timeOutName = "fast-list-widget-" + String(Math.random()).slice(2);
> +
> +  // This is a prototype element that each item added to the list clones.
> +  this._protoElement = this.document.createElement("hbox");
> +  this._protoElement.className = "side-menu-widget-item side-menu-widget-item-contents";

Nit: "template" sounds better than "prototype" when it comes to UI.

@@ +1109,5 @@
> +  // required by MenuContainer instances.
> +  ViewHelpers.delegateWidgetEventMethods(this, aNode);
> +}
> +
> +FastListWidget.prototype = {

This misses removeChild for it to fully satisfy the Widgets interface. Doesn't seem to be used anywhere, but until bug 895514 it might be a good idea to at least add a method throwing "Not implemented" or similar.

@@ +1116,5 @@
> +   * grouping by name.
> +   *
> +   * @param number aIndex
> +   *        The position in the container intended for this item.
> +   * @param string | nsIDOMNode aContents

This doesn't really ever expect a string, does it?

@@ +1128,5 @@
> +    let element = this._protoElement.cloneNode();
> +    element.appendChild(aContents);
> +
> +    if (!element.focus) {
> +      element.focus = () => {};

What is this?

@@ +1132,5 @@
> +      element.focus = () => {};
> +    }
> +
> +    if (aIndex >= 0) {
> +      this._list.insertBefore(element, ownerList.childNodes[aIndex]);

Hehe, taking the slow path here.
Since this isn't tested or used anywhere, how about throwing "Not implemented".

@@ +1143,5 @@
> +
> +    return element;
> +  },
> +
> +  flush: function() {

Document this please.

@@ +1301,5 @@
> +    this._tracerTab = document.getElementById("tracer-tab");
> +    // Display the Tracer UI if the pref is set.
> +    if (Prefs.tracerEnabled) {
> +      this._traceButton.removeAttribute("hidden");
> +      this._tracerTab.removeAttribute("hidden");

Should probably .remove() the nodes themselves to avoid bug 949933.

@@ +1371,5 @@
> +  _onClear: function() {
> +    this.empty();
> +  },
> +
> +  _populateVariable: function(aName, aParent, aValue) {

Can you please document this?

@@ +1384,5 @@
> +
> +    return child;
> +  },
> +
> +  _onSelect: function({ detail: traceItem }) {

And this?

@@ +1435,5 @@
> +   * Add the hover frame enter/exit highlighting to a given item.
> +   */
> +  _highlightItem: function(aItem) {
> +    aItem.target.querySelector(".trace-item")
> +      .classList.add("selected-matching");

Wow, this is fugly. Is it really necessary? Why not use :hover and sexy operators like + or ~ in css?

@@ +1470,5 @@
> +   * Highlight the frame enter/exit pair of items for the given item.
> +   */
> +  _highlightMatchingItems: function(aItem) {
> +    this._unhighlightMatchingItems();
> +    this._matchingItems = this.visibleItems.filter(t => t.value == aItem.value);

Do you really *always* need visibleItems here? It will make this O(n^2) while not searching. Just using .items will be sufficient when not searching.

@@ +1490,5 @@
> +  /**
> +   * Listener for typing in the search box.
> +   */
> +  _onSearch: function() {
> +    const query = this._search.value.trim().toLowerCase();

Are you basically reimplementing WidgetMethods.filterContents here? Can you use that instead?

@@ +1511,5 @@
> +    tabs.selectedIndex = [].slice.call(tabs.children).indexOf(this._tracerTab);
> +    this._tracerDeck.selectedIndex = 0;
> +  },
> +
> +  commit: function() {

Document this please, note why we're overriding this.

@@ +1513,5 @@
> +  },
> +
> +  commit: function() {
> +    WidgetMethods.commit.call(this);
> +    this.widget.flush();

// TODO: Accessing non-standard widget properties. Figure out what's the best
// way to expose such things. Bug 895514.

@@ +1548,5 @@
> +                          location, depth, arguments: args }) {
> +    let template = document.querySelector(".trace-item-template");
> +    let fragment = document.createDocumentFragment();
> +
> +    let item = template.querySelector(".trace-item");

If you use getElementsByClassName, or even better, cache the template child nodes beforehand so you don't need to query them, you'll squeeze a bit more performance out of this.

@@ +1550,5 @@
> +    let fragment = document.createDocumentFragment();
> +
> +    let item = template.querySelector(".trace-item");
> +    item.setAttribute("tooltiptext", SourceUtils.trimUrl(location.url));
> +    item.style.paddingLeft = depth + "em";

MozPaddingStart or MozMarginStart to localize this.

@@ +1597,5 @@
> +    for (let node of template.childNodes) {
> +      fragment.appendChild(node.cloneNode(true));
> +    }
> +
> +    // Remove any added nodes from the template.

You should only do this if you actually added syntax and param nodes. querySelectorAll is slower than you'd think.

::: browser/devtools/debugger/debugger-view.js
@@ +236,5 @@
>        }
>      });
>    },
>  
> +  _syncGripClient: function(aObject) {

This really doesn't belong in DebuggerView. Please document what it does and why it's necessary, and move it somewhere in the controller. In the Tracer object even.

@@ +1052,5 @@
>    left: 0,
>    top: 0
>  });
> +
> +function TracerObject(aObject) {

This is pretty lonely here. How about moving it in debugger-controller as a member of Tracer. Like Tracer.WrappedObject.

::: browser/devtools/debugger/debugger.xul
@@ +367,5 @@
>            </tabpanel>
> +          <tabpanel id="tracer-tabpanel" flex="1">
> +            <deck id="tracer-deck" selectedIndex="1" flex="1">
> +              <vbox flex="1">
> +                <scrollbox id="tracer-traces" flex="1">

Why do you need a scrollbox when the FastListWidget already creates one internally?

@@ +383,5 @@
> +                                 command="clearTraces"
> +                                 class="devtools-toolbarbutton"/>
> +                  <textbox id="tracer-search"
> +                           class="devtools-searchinput"
> +                           type="search"/>

This needs to flex or something. Bug 949911.

@@ +389,5 @@
> +              </vbox>
> +              <vbox id="tracer-message"
> +                    flex="1"
> +                    align="center">
> +                <label>&debuggerUI.tracingNotStarted.label;</label>

If you need to inline this for it to wrap, use <description>. <xul:labels> should always have a value attribute.

::: browser/devtools/debugger/panel.js
@@ +59,5 @@
>          return this;
>        })
>        .then(null, function onError(aReason) {
> +        DevToolsUtils.reportException("DebuggerPane.prototype.open",
> +                                      aReason);

Nit: I think it's safe to keep aReason on the same line :)

::: browser/themes/windows/devtools/debugger.css
@@ +70,5 @@
>  /* Black box message and source progress meter */
>  
>  #black-boxed-message,
> +#source-progress-container,
> +#tracer-message {

You should keep the #tracer-message along the other ones in /* Tracer */, since the blackbox message and source progress container styling have nothing to do with the tracer tab, and will only make things harder for bug 943883.

@@ +104,5 @@
> +#trace[checked] {
> +  -moz-image-region: rect(0px,32px,16px,16px);
> +}
> +
> +.tracer-traces {

Why is this class necessary when the FastListWidget creates its own scrollbox?

@@ +109,5 @@
> +  overflow: scroll;
> +}
> +
> +.trace-item {
> +  color: rgb(245, 247, 250);

Needs to be themable. Easier now with CSS variables, bug 947242. Either file a separate bug, or wait for that to land first.

@@ +123,5 @@
> +  padding: 0;
> +}
> +
> +.trace-call {
> +  color: #99f;

Themes! All these colors need to be themable and use the palette defined at https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

@@ +152,5 @@
> +}
> +
> +.trace-name {
> +  margin: 0;
> +  padding: 0;

Instead of margin: 0, padding: 0 everywhere, have you tried using the built-in "plain" class? https://developer.mozilla.org/en-US/docs/XUL/Style/plain

@@ +302,5 @@
>  
>  /* Instruments pane (watch expressions, variables, event listeners...) */
>  
> +#instruments-pane > tabs > tab,
> +#sources-pane > tabs > tab {

None of this is necessary, because of http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/debugger.css#442

(not even the #instruments-pane rules, remove them while you're here).

Comment 43

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

Attachment: tracer-ui.patch

Updating based on feedback from Victor.

* Now theme-able with colors from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

* Caching sub nodes in the template element to avoid unnecessary traversals

* No more unnecessary check for |client|

* FastListWidget moved to its own file

* Removed _timeOutName from DebuggerView.Tracer, which was not used

* s/_protoElement/_templateElement/ in FastListWidget

* Added FastListWidget.prototype.removeChild which just throws Error("Not yet implemented")

* Removed optional string param from FastListWidget.prototype.insertItemAt

* Removed element.focus crap which was just leftover from a prototype code that I didn't end up keeping

* FastListWidget.prototype.insertItemAt will now throw an error if you aren't appending

* Added docs for various functions

* I remove the tracer nodes when the pref is off now (bug 949933)

* Now using WidgetMethods.filterContents in when searching the trace logs

* The indentation of trace logs is localizable now

* Avoid calling querySelectorAll unnecessarily by keeping track of syntax and parameter nodes we add to the template node in _createView

* The search box flexes properly now (bug 949911)

* Use description instead of label in the "not tracing" message

* Using the "plain" CSS class when applicable

* Moved TracerObject and syncGripClient stuff to debugger-controller.js

TODO:

* Figure out why parameters stopped working and fix them

Comment 44

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

Attachment: screenshot of tracer w/ dark theme

Comment 45

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

(In reply to Victor Porof [:vp] from comment #42)
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +1074,5 @@
> > + *
> > + * @param nsIDOMNode aNode
> > + *        The element associated with the widget.
> > + */
> > +function FastListWidget(aNode) {
> 
> I think it'd be better to have FastListWidget in a different file inside
> /shared, to keep debugger-panes.js under 3500 lines :)

Agreed and done. Also, I realized that all of our files end up in the jar file, which is kept in memory, so we can actually split out all of our various files and it won't be a perf hit because of extra file IO! Sometime in the future...

> 
> @@ +1081,5 @@
> > +  this._parent = aNode;
> > +  this._fragment = this.document.createDocumentFragment();
> > +  // Create a new unique time out name for each instance of FastListWidget so
> > +  // that they can flush their buffered items independantly from one another.
> > +  this._timeOutName = "fast-list-widget-" + String(Math.random()).slice(2);
> 
> Collisions are rare, but possible. This is just a little bit fragile.

This was just a leftover from a binned experiment; removed the whole thing.

> @@ +1435,5 @@
> > +   * Add the hover frame enter/exit highlighting to a given item.
> > +   */
> > +  _highlightItem: function(aItem) {
> > +    aItem.target.querySelector(".trace-item")
> > +      .classList.add("selected-matching");
> 
> Wow, this is fugly. Is it really necessary? Why not use :hover and sexy
> operators like + or ~ in css?

We can't do this because we are highlighting all matching calls / returns / and (when the Debugger supports it) yields on the same frame. It isn't just the hovered element that is getting highlighted.

We could bring this down from O(n) (with n = MAX_TRACES) to O(1) by keeping a map of frame id -> set of trace elements for that frame. This isn't our bottleneck but it is worth investigating sometime in the future.

Filed bug 950273.

> 
> @@ +1470,5 @@
> > +   * Highlight the frame enter/exit pair of items for the given item.
> > +   */
> > +  _highlightMatchingItems: function(aItem) {
> > +    this._unhighlightMatchingItems();
> > +    this._matchingItems = this.visibleItems.filter(t => t.value == aItem.value);
> 
> Do you really *always* need visibleItems here? It will make this O(n^2)
> while not searching. Just using .items will be sufficient when not searching.

Well actually O(n) + O(n) so it's not horribad, but you're right we can get away with just using this.items. Latest patch uses this.items.

Comment 46

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #45)

Thanks for addressing all my drive-by nits Nick :)

> (In reply to Victor Porof [:vp] from comment #42)
> > ::: browser/devtools/debugger/debugger-panes.js
> > @@ +1074,5 @@
> > > + *
> > > + * @param nsIDOMNode aNode
> > > + *        The element associated with the widget.
> > > + */
> > > +function FastListWidget(aNode) {
> > 
> > I think it'd be better to have FastListWidget in a different file inside
> > /shared, to keep debugger-panes.js under 3500 lines :)
> 
> Agreed and done. Also, I realized that all of our files end up in the jar
> file, which is kept in memory, so we can actually split out all of our
> various files and it won't be a perf hit because of extra file IO! Sometime
> in the future...
> 

I think you're right! Brave new world!

Comment 47

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

Comment on attachment 8347507 [details] [diff] [review]
tracer-ui.patch

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

All of this is beautiful! I'm very happy :)

Function calls look crappy when selected on a light theme (dark text on dark blue background). Functions arguments are very hard to read while hovering, on both themes (light gray on light blue background). But I'm just nitpicking.

::: browser/devtools/debugger/debugger-panes.js
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
> +const FastListWidget = require("devtools/shared/widgets/FastListWidget");

Nit: Would it be possible to move this import in debugger-controller.js, to keep all imports together? Or we can do it in a followup and switch all the widget imports in the debugger frontend to be done via require.

@@ +1085,5 @@
> +   */
> +  initialize: function() {
> +    dumpn("Initializing the TracerView");
> +
> +    this.widget = new FastListWidget(document.getElementById("tracer-traces"));

Nit: Can this be moved inside the Prefs.tracerEnabled conditional, to avoid instantiating a FastListWidget while the Tracer is disabled?

@@ +1329,5 @@
> +   * Select the traces tab in the sidebar.
> +   */
> +  selectTab: function() {
> +    const tabs = this._tracerTab.parentElement;
> +    tabs.selectedIndex = [].slice.call(tabs.children).indexOf(this._tracerTab);

Super nit (and FYI): I think you can use Array.indexOf(tabs.children, this._tracerTab) to avoid instantiating an empty array and slicing a NodeList into it.

@@ +1429,5 @@
> +      fragment.appendChild(node.cloneNode(true));
> +    }
> +
> +    // Remove any added nodes from the template.
> +    if (addedNodes.length) {

Super nit: You don't need the conditional, since addedNodes is always an array. The for loop would simply do nothing.

::: browser/devtools/shared/widgets/FastListWidget.js
@@ +156,5 @@
> +   *        The name of the attribute.
> +   * @return string
> +   *         The current attribute value.
> +   */
> +  getAttribute: function(name) {

FYI: Since all of these get/set/removeAttribute methods don't do anything special, you could simply use ViewHelpers.delegateWidgetAttributeMethods along with ViewHelpers.delegateWidgetEventMethods instead of writing all of this redundant code.

However, we might use them for setting an empty notice, so it's probably be a good idea the keep them around. https://bugzilla.mozilla.org/show_bug.cgi?id=949909#c3

Comment 48

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #45)
> (In reply to Victor Porof [:vp] from comment #42)
> > @@ +1470,5 @@
> > > +   * Highlight the frame enter/exit pair of items for the given item.
> > > +   */
> > > +  _highlightMatchingItems: function(aItem) {
> > > +    this._unhighlightMatchingItems();
> > > +    this._matchingItems = this.visibleItems.filter(t => t.value == aItem.value);
> > 
> > Do you really *always* need visibleItems here? It will make this O(n^2)
> > while not searching. Just using .items will be sufficient when not searching.
> 
> Well actually O(n) + O(n) so it's not horribad, but you're right we can get
> away with just using this.items. Latest patch uses this.items.

Isn't visibleItems just the items visible in the widget? At least that's what I assumed from this:

https://gist.github.com/victorporof/5749386

And I imagined items is what is mentioned there as orderedItems, i.e. the entire collection.

Comment 49

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

(In reply to Panos Astithas [:past] from comment #48)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #45)
> > (In reply to Victor Porof [:vp] from comment #42)
> > > @@ +1470,5 @@
> > > > +   * Highlight the frame enter/exit pair of items for the given item.
> > > > +   */
> > > > +  _highlightMatchingItems: function(aItem) {
> > > > +    this._unhighlightMatchingItems();
> > > > +    this._matchingItems = this.visibleItems.filter(t => t.value == aItem.value);
> > > 
> > > Do you really *always* need visibleItems here? It will make this O(n^2)
> > > while not searching. Just using .items will be sufficient when not searching.
> > 
> > Well actually O(n) + O(n) so it's not horribad, but you're right we can get
> > away with just using this.items. Latest patch uses this.items.
> 
> Isn't visibleItems just the items visible in the widget? At least that's
> what I assumed from this:
> 

Yes. But to get the visible items, we do a filter on the items, so O(2n), as Nick said. We don't keep a separate store for the visible items.

Comment 50

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

(In reply to Panos Astithas [:past] from comment #48)
> 
> https://gist.github.com/victorporof/5749386

Just updated the gist to latest API :)

Comment 51

[Panos Astithas (he/him) [:past] (please ni?)]

I see, thanks for the update! I thought that the visibleItems property would be memoized given that it's a getter and not a method (you don't usually expect side-effects or long execution time from property access, even though I agree that you picked the right tradeoff in this case). Maybe you could add an API comment to that effect in the gist?

Comment 52

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

Attachment: tracer-ui.patch

With a bunch of perf enhancements / corner cutting I did trying to fix bug 949928.

Comment 53

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

Attachment: tracer-ui.patch

https://tbpl.mozilla.org/?tree=Try&rev=798f4405d361

* Fixed parameters and return values in the VV.

* Updated based on feedback from victor's last review.

Comment 54

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

Comment on attachment 8349105 [details] [diff] [review]
tracer-ui.patch

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

Modulo the visual issues based on the selected theme described in comment 47, and the fact that function parameters are *still* not shown in the variables view, I'm happy :)

Steps for not getting params:
1. Go to http://htmlpad.org/debugger/
2. Start tracing 
3. Click me
4. Resume until there's no more code to execute
5. Select any function with params from the tracer view

You should add a test for that.

I'm also getting these errors: 
Handler function _onSelect threw an exception: TypeError: DevToolsUtils.zip is not a function
Stack: _onSelect@chrome://browser/content/devtools/debugger-panes.js:1229
makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:75

Comment 55

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

(In reply to Victor Porof [:vp] from comment #54)
> Comment on attachment 8349105 [details] [diff] [review]
> tracer-ui.patch
> 
> Review of attachment 8349105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Modulo the visual issues based on the selected theme described in comment
> 47,

Would like to push the color stuff off to a follow up, if you don't mind. I'm using theme colors that best match the old readable colors. If our theme colors don't handle our use cases, we need to talk to ux again.

>and the fact that function parameters are *still* not shown in the
> variables view, I'm happy :)

Works for me, and there is a test. Are you building toolkit as well as browser?

Comment 56

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

Attachment: screenshot of arguments being shown

Comment 57

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

Just confirmed with Victor that he wasn't building toolkit, and everything works as expected now.

Comment 58

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8349105 [details] [diff] [review]
tracer-ui.patch

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

Just took a look at the server bits and they look good to me. One UI thought that occurred to me, is that perhaps we should put the Traces tab at the end of the tab strip, as the other two (Sources and Stack) are more closely related. But that's just an idea.

::: toolkit/devtools/server/actors/tracer.js
@@ +632,2 @@
>  
>    if (Cu.isDeadWrapper(aObject)) {

Cu.isDeadWrapper doesn't seem to work for Debugger.Object and you need to use aObject.class == "DeadObject". See bug 843004 for Mihai's investigation.

Comment 59

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

https://hg.mozilla.org/integration/fx-team/rev/657e3059fee2

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/657e3059fee2