817836 - [profiler] Hook up console.profile and profileEnd to the Profiler

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

Description

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

Currently, profile and profileEnd exist but do nothing. We should listen for those events and add profiles to the built-in profiler when they occur.

Comment 1

[Benoit Girard (:BenWa)]

As mention on IRC ideally we should allow the profiler add-on to overwrite these.

Comment 2

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

bumping priority on this.

Comment 3

[Anton Kovalyov (:anton)]

Attachment: Pt. 1: Support for multiple simultaneously running profiles

Added support for multiple profiles. This is part 1 of 2. I will upload a patch that adds actual hooks for the console API as a separate patch.

Comment 4

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

Comment on attachment 726409 [details] [diff] [review]
Pt. 1: Support for multiple simultaneously running profiles

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

I found that I could reliably reproduce the following error, if I switched to a different profile right after pressing Stop in another profile and before cleopatra finished displaying the timeline:

JavaScript error: chrome://browser/content/devtools/profiler/cleopatra/js/ui.js, line 524: NS_ERROR_FAILURE: Failure

r=me only if this is unrelated to this patch, in which case we need to file another bug.

::: browser/devtools/profiler/ProfilerController.jsm
@@ +48,5 @@
> +   *
> +   * @return number
> +   */
> +  get currentTime() {
> +    return (new Date()).getTime() - this.startTime;

The parens around new Date() are redundant, because |new| and member (.) operators share the same priority with a left-to-right associativity that will DTRT:

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/Operator_Precedence#Table

@@ +214,5 @@
> +      return;
> +    }
> +
> +    let profile = this.pool[name] = makeProfile(name);
> +    let profiler = this.profiler;    

trailing whitespace here and elsewhere in this file.

@@ +219,5 @@
> +
> +    this.isActive(function (err, isActive) {
> +      if (isActive) {
> +        profile.timeStarted = profiler.currentTime;
> +        return void cb();

So restarting an already running profiler will reset the timeStarted, right? This should be documented in the method comment I think.

Also, can this ever happen with the available UI, or is it a precaution against future changes?

::: browser/devtools/profiler/test/browser_profiler_bug_830664_multiple_profiles.js
@@ +42,5 @@
> +  gPanel.profiles.get(gPanel.activeProfile.uid).once("started", function () {
> +    setTimeout(function () {
> +      sendFromProfile(2, "start");
> +      gPanel.profiles.get(2).once("started", function () setTimeout(stopProfiling, 50));
> +    }, 50);

Don't we use 100 msec in other tests? Do you feel particularly lucky lately? :-)

Comment 5

[Anton Kovalyov (:anton)]

Thanks!

(In reply to Panos Astithas [:past] from comment #4)
> Comment on attachment 726409 [details] [diff] [review]
> Pt. 1: Support for multiple simultaneously running profiles
> 
> Review of attachment 726409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I found that I could reliably reproduce the following error, if I switched
> to a different profile right after pressing Stop in another profile and
> before cleopatra finished displaying the timeline:
> 
> JavaScript error:
> chrome://browser/content/devtools/profiler/cleopatra/js/ui.js, line 524:
> NS_ERROR_FAILURE: Failure
> 
> r=me only if this is unrelated to this patch, in which case we need to file
> another bug.

Will file a bug. This seems to be unrelated to this patch.

> 
> ::: browser/devtools/profiler/ProfilerController.jsm
> @@ +48,5 @@
> > +   *
> > +   * @return number
> > +   */
> > +  get currentTime() {
> > +    return (new Date()).getTime() - this.startTime;
> 
> The parens around new Date() are redundant, because |new| and member (.)
> operators share the same priority with a left-to-right associativity that
> will DTRT:
> 
> https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/
> Operator_Precedence#Table

Its just my personal preference. new Func().blah just feels weird. :)

> @@ +214,5 @@
> > +      return;
> > +    }
> > +
> > +    let profile = this.pool[name] = makeProfile(name);
> > +    let profiler = this.profiler;    
> 
> trailing whitespace here and elsewhere in this file.
> 
> @@ +219,5 @@
> > +
> > +    this.isActive(function (err, isActive) {
> > +      if (isActive) {
> > +        profile.timeStarted = profiler.currentTime;
> > +        return void cb();
> 
> So restarting an already running profiler will reset the timeStarted, right?
> This should be documented in the method comment I think.
> 
> Also, can this ever happen with the available UI, or is it a precaution
> against future changes?

This is actually an oversight. Our UI doesn't allow profiles to be restarted so I never thought about this scenario. We should probably throw an error or do nothing here.

> :::
> browser/devtools/profiler/test/browser_profiler_bug_830664_multiple_profiles.
> js
> @@ +42,5 @@
> > +  gPanel.profiles.get(gPanel.activeProfile.uid).once("started", function () {
> > +    setTimeout(function () {
> > +      sendFromProfile(2, "start");
> > +      gPanel.profiles.get(2).once("started", function () setTimeout(stopProfiling, 50));
> > +    }, 50);
> 
> Don't we use 100 msec in other tests? Do you feel particularly lucky lately?
> :-)

Haha. If I use 100 msec the total will be 200 msec (first profile -- 100 msec -- second profile -- 100 msec -- stop first and second profiles) which is kind of slow.

Comment 6

[Anton Kovalyov (:anton)]

(In reply to Panos Astithas [:past] from comment #4)
> Comment on attachment 726409 [details] [diff] [review]
> 
> r=me only if this is unrelated to this patch, in which case we need to file
> another bug.
> 

Filed bug 853653.

Comment 7

[Anton Kovalyov (:anton)]

Comment on attachment 726409 [details] [diff] [review]
Pt. 1: Support for multiple simultaneously running profiles

># HG changeset patch
># User Anton Kovalyov <[e-mail]>
># Date 1363649099 25200
># Node ID 40d9ae691d81601b6f76a8cb8a2e71d1a49e0ff6
># Parent  f923d52bfbb18880a1bb8fd75097b52819288395
>[mq]: profiler-console2
>
>diff --git a/browser/devtools/profiler/ProfilerController.jsm b/browser/devtools/profiler/ProfilerController.jsm
>--- a/browser/devtools/profiler/ProfilerController.jsm
>+++ b/browser/devtools/profiler/ProfilerController.jsm
>@@ -19,15 +19,38 @@
> });
> 
> /**
>+ * Makes a structure representing an individual profile.
>+ */
>+function makeProfile(name) {
>+  return {
>+    name: name,
>+    timeStarted: null,
>+    timeEnded: null
>+  };
>+}
>+
>+/**
>  * Object acting as a mediator between the ProfilerController and
>  * DebuggerServer.
>  */
> function ProfilerConnection(client) {
>   this.client = client;
>+  this.startTime = 0;
> }
> 
> ProfilerConnection.prototype = {
>   actor: null,
>+  startTime: null,
>+
>+  /**
>+   * Returns how many milliseconds have passed since the connection
>+   * was started (start time is specificed by the startTime property).
>+   *
>+   * @return number
>+   */
>+  get currentTime() {
>+    return (new Date()).getTime() - this.startTime;
>+  },
> 
>   /**
>    * Connects to a debugee and executes a callback when ready.
>@@ -71,7 +94,13 @@
>       interval: 1,
>       features: ["js"],
>     };
>-    this.client.request(message, aCallback);
>+
>+    this.client.request(message, function () {
>+      // Record the current time so we could split profiler data
>+      // in chunks later.
>+      this.startTime = (new Date()).getTime();
>+      aCallback.apply(null, Array.slice(arguments));
>+    }.bind(this));
>   },
> 
>   /**
>@@ -113,9 +142,12 @@
>  */
> function ProfilerController(target) {
>   this.profiler = new ProfilerConnection(target.client);
>-  // Chrome debugging targets have already obtained a reference to the profiler
>-  // actor.
>+  this.pool = {};
>+
>+  // Chrome debugging targets have already obtained a reference to the
>+  // profiler actor.
>   this._connected = !!target.chrome;
>+
>   if (target.chrome) {
>     this.profiler.actor = target.form.profilerActor;
>   }
>@@ -157,44 +189,113 @@
>   },
> 
>   /**
>-   * Starts the profiler.
>+   * Checks whether the profile is currently recording.
>    *
>-   * @param function aCallback
>+   * @param object profile
>+   *        An object made by calling makeProfile function.
>+   * @return boolean
>+   */
>+  isProfileRecording: function PC_isProfileRecording(profile) {
>+    return profile.timeStarted !== null && profile.timeEnded === null;
>+  },
>+
>+  /**
>+   * Creates a new profile and starts the profiler, if needed.
>+   *
>+   * @param string name
>+   *        Name of the profile.
>+   * @param function cb
>    *        Function to be called once the profiler is started
>    *        or we get an error. It will be called with a single
>    *        argument: an error object (may be null).
>    */
>-  start: function PC_start(aCallback) {
>-    this.profiler.startProfiler(function onStart(aResponse) {
>-      aCallback(aResponse.error);
>+  start: function PC_start(name, cb) {
>+    if (this.pool[name]) {
>+      return;
>+    }
>+
>+    let profile = this.pool[name] = makeProfile(name);
>+    let profiler = this.profiler;    
>+
>+    this.isActive(function (err, isActive) {
>+      if (isActive) {
>+        profile.timeStarted = profiler.currentTime;
>+        return void cb();
>+      }
>+
>+      profiler.startProfiler(function onStart(aResponse) {
>+        if (aResponse.error) {
>+          return void cb(aResponse.error);
>+        }
>+
>+        profile.timeStarted = profiler.currentTime;
>+        cb();
>+      });
>     });
>   },
> 
>   /**
>    * Stops the profiler.
>    *
>-   * @param function aCallback
>+   * @param string name
>+   *        Name of the profile that needs to be stopped.
>+   * @param function cb
>    *        Function to be called once the profiler is stopped
>    *        or we get an error. It will be called with a single
>    *        argument: an error object (may be null).
>    */
>-  stop: function PC_stop(aCallback) {
>-    this.profiler.getProfileData(function onData(aResponse) {
>-      let data = aResponse.profile;
>+  stop: function PC_stop(name, cb) {
>+    let profiler = this.profiler;    
>+    let profile = this.pool[name];
>+
>+    if (!profile || !this.isProfileRecording(profile)) {
>+      return;
>+    }
>+
>+    let isRecording = function () {
>+      for (let name in this.pool) {
>+        if (this.isProfileRecording(this.pool[name])) {
>+          return true;
>+        }
>+      }
>+
>+      return false;
>+    }.bind(this);
>+    
>+    let onStop = function (data) {
>+      if (isRecording()) {
>+        return void cb(null, data);
>+      }
>+
>+      profiler.stopProfiler(function onStopProfiler(response) {
>+        cb(response.error, data);
>+      });
>+    }.bind(this);
>+
>+    profiler.getProfileData(function onData(aResponse) {
>       if (aResponse.error) {
>         Cu.reportError("Failed to fetch profile data before stopping the profiler.");
>+        return void cb(aResponse.error, null);
>       }
> 
>-      this.profiler.stopProfiler(function onStop(aResponse) {
>-        aCallback(aResponse.error, data);
>+      let data = aResponse.profile;
>+      profile.timeEnded = profiler.currentTime;
>+
>+      data.threads = data.threads.map(function (thread) {
>+        let samples = thread.samples.filter(function (sample) {
>+          return sample.time >= profile.timeStarted;
>+        });
>+        return { samples: samples };
>       });
>-    }.bind(this));
>+
>+      onStop(data);
>+    });
>   },
> 
>   /**
>    * Cleanup.
>    */
>-  destroy: function PC_destroy(aCallback) {
>+  destroy: function PC_destroy() {
>     this.profiler.destroy();
>     this.profiler = null;
>   }
>diff --git a/browser/devtools/profiler/ProfilerPanel.jsm b/browser/devtools/profiler/ProfilerPanel.jsm
>--- a/browser/devtools/profiler/ProfilerPanel.jsm
>+++ b/browser/devtools/profiler/ProfilerPanel.jsm
>@@ -71,6 +71,8 @@
>     }
> 
>     let label = doc.querySelector("li#profile-" + this.uid + " > h1");
>+    let name = label.textContent.replace(/\s\*$/, "");
>+
>     switch (event.data.status) {
>       case "loaded":
>         if (this.panel._runningUid !== null) {
>@@ -89,9 +91,10 @@
>         // so that it could update the UI. Also, once started, we add a
>         // star to the profile name to indicate which profile is currently
>         // running.
>-        this.panel.startProfiling(function onStart() {
>+        this.panel.startProfiling(name, function onStart() {
>+          label.textContent = name + " *";
>           this.panel.broadcast(this.uid, {task: "onStarted"});
>-          label.textContent = label.textContent + " *";
>+          this.emit("started");
>         }.bind(this));
> 
>         break;
>@@ -99,9 +102,10 @@
>         // Stop profiling and, once stopped, notify the underlying page so
>         // that it could update the UI and remove a star from the profile
>         // name.
>-        this.panel.stopProfiling(function onStop() {
>+        this.panel.stopProfiling(name, function onStop() {
>+          label.textContent = name;
>           this.panel.broadcast(this.uid, {task: "onStopped"});
>-          label.textContent = label.textContent.replace(/\s\*$/, "");
>+          this.emit("stopped");
>         }.bind(this));
>         break;
>       case "disabled":
>@@ -372,8 +376,8 @@
>    *   A function to call once we get the message
>    *   that profiling had been successfuly started.
>    */
>-  startProfiling: function PP_startProfiling(onStart) {
>-    this.controller.start(function (err) {
>+  startProfiling: function PP_startProfiling(name, onStart) {
>+    this.controller.start(name, function (err) {
>       if (err) {
>         Cu.reportError("ProfilerController.start: " + err.message);
>         return;
>@@ -392,7 +396,7 @@
>    *   A function to call once we get the message
>    *   that profiling had been successfuly stopped.
>    */
>-  stopProfiling: function PP_stopProfiling(onStop) {
>+  stopProfiling: function PP_stopProfiling(name, onStop) {
>     this.controller.isActive(function (err, isActive) {
>       if (err) {
>         Cu.reportError("ProfilerController.isActive: " + err.message);
>@@ -403,18 +407,19 @@
>         return;
>       }
> 
>-      this.controller.stop(function (err, data) {
>+      this.controller.stop(name, function (err, data) {
>         if (err) {
>           Cu.reportError("ProfilerController.stop: " + err.message);
>           return;
>         }
> 
>+        this.activeProfile.data = data;
>         this.activeProfile.parse(data, function onParsed() {
>           this.emit("parsed");
>         }.bind(this));
> 
>         onStop();
>-        this.emit("stopped");
>+        this.emit("stopped", data);
>       }.bind(this));
>     }.bind(this));
>   },
>diff --git a/browser/devtools/profiler/cleopatra/js/devtools.js b/browser/devtools/profiler/cleopatra/js/devtools.js
>--- a/browser/devtools/profiler/cleopatra/js/devtools.js
>+++ b/browser/devtools/profiler/cleopatra/js/devtools.js
>@@ -54,31 +54,20 @@
>   var profilerMessage = document.getElementById("profilerMessage");
>   var msg = JSON.parse(event.data);
> 
>+  if (msg.task !== "receiveProfileData" && !msg.isCurrent) {
>+    return;
>+  }
>+
>   switch (msg.task) {
>     case "onStarted":
>-      if (msg.isCurrent) {
>-        start.style.display = "none";
>-        start.querySelector("button").removeAttribute("disabled");
>-        stop.style.display = "inline";
>-      } else {
>-        start.querySelector("button").setAttribute("disabled", true);
>-        var text = gStrings.getFormatStr("profiler.alreadyRunning", [msg.uid]);
>-        profilerMessage.textContent = text;
>-        profilerMessage.style.display = "block";
>-        notifyParent("disabled");
>-      }
>+      start.style.display = "none";
>+      start.querySelector("button").removeAttribute("disabled");
>+      stop.style.display = "inline";
>       break;
>     case "onStopped":
>-      if (msg.isCurrent) {
>-        stop.style.display = "none";
>-        stop.querySelector("button").removeAttribute("disabled");
>-        start.style.display = "inline";
>-      } else {
>-        start.querySelector("button").removeAttribute("disabled");
>-        profilerMessage.textContent = "";
>-        profilerMessage.style.display = "none";
>-        notifyParent("enabled");
>-      }
>+      stop.style.display = "none";
>+      stop.querySelector("button").removeAttribute("disabled");
>+      start.style.display = "inline";
>       break;
>     case "receiveProfileData":
>       loadProfile(JSON.stringify(msg.rawProfile));
>@@ -267,4 +256,4 @@
>   Parser.updateLogSetting();
> 
>   return reporter;
>-}
>\ No newline at end of file
>+}
>diff --git a/browser/devtools/profiler/test/browser_profiler_bug_830664_multiple_profiles.js b/browser/devtools/profiler/test/browser_profiler_bug_830664_multiple_profiles.js
>--- a/browser/devtools/profiler/test/browser_profiler_bug_830664_multiple_profiles.js
>+++ b/browser/devtools/profiler/test/browser_profiler_bug_830664_multiple_profiles.js
>@@ -33,43 +33,44 @@
>   ];
> }
> 
>-function sendFromActiveProfile(msg) {
>-  let [win, doc] = getProfileInternals();
>-
>-  win.parent.postMessage({
>-    uid: gPanel.activeProfile.uid,
>-    status: msg
>-  }, "*");
>+function sendFromProfile(uid, msg) {
>+  let [win, doc] = getProfileInternals(uid);
>+  win.parent.postMessage({ uid: uid, status: msg }, "*");
> }
> 
> function startProfiling() {
>-  gPanel.profiles.get(gUid).once("disabled", stopProfiling);
>-  sendFromActiveProfile("start");
>+  gPanel.profiles.get(gPanel.activeProfile.uid).once("started", function () {
>+    setTimeout(function () {
>+      sendFromProfile(2, "start");
>+      gPanel.profiles.get(2).once("started", function () setTimeout(stopProfiling, 50));
>+    }, 50);
>+  });
>+  sendFromProfile(gPanel.activeProfile.uid, "start");
> }
> 
> function stopProfiling() {
>   let [win, doc] = getProfileInternals(gUid);
>   let [btn, msg] = getCleoControls(doc);
> 
>-  ok(msg.textContent.match("Profile 1") !== null, "Message is visible");
>-  ok(btn.hasAttribute("disabled"), "Button is disabled");
>-
>   is(gPanel.document.querySelector("li#profile-1 > h1").textContent,
>     "Profile 1 *", "Profile 1 has a star next to it.");
>   is(gPanel.document.querySelector("li#profile-2 > h1").textContent,
>-    "Profile 2", "Profile 2 doesn't have a star next to it.");
>+    "Profile 2 *", "Profile 2 has a star next to it.");
> 
>-  gPanel.profiles.get(gUid).once("enabled", confirmAndFinish);
>-  sendFromActiveProfile("stop");
>+  gPanel.profiles.get(gPanel.activeProfile.uid).once("stopped", function () {
>+    is(gPanel.document.querySelector("li#profile-1 > h1").textContent,
>+      "Profile 1", "Profile 1 doesn't have a star next to it anymore.");
>+
>+    sendFromProfile(2, "stop");
>+    gPanel.profiles.get(2).once("stopped", confirmAndFinish);
>+  });
>+  sendFromProfile(gPanel.activeProfile.uid, "stop");
> }
> 
>-function confirmAndFinish() {
>+function confirmAndFinish(ev, data) {
>   let [win, doc] = getProfileInternals(gUid);
>   let [btn, msg] = getCleoControls(doc);
> 
>-  ok(msg.style.display === "none", "Message is hidden");
>-  ok(!btn.hasAttribute("disabled"), "Button is enabled");
>-
>   is(gPanel.document.querySelector("li#profile-1 > h1").textContent,
>     "Profile 1", "Profile 1 doesn't have a star next to it.");
>   is(gPanel.document.querySelector("li#profile-2 > h1").textContent,
>@@ -80,4 +81,4 @@
>     gTab = null;
>     gUid = null;
>   });
>-}
>\ No newline at end of file
>+}
>diff --git a/browser/devtools/profiler/test/browser_profiler_controller.js b/browser/devtools/profiler/test/browser_profiler_controller.js
>--- a/browser/devtools/profiler/test/browser_profiler_controller.js
>+++ b/browser/devtools/profiler/test/browser_profiler_controller.js
>@@ -12,7 +12,7 @@
>     gTab = tab;
>     gPanel = panel;
> 
>-    testInactive(testStart);
>+    testInactive(startFirstProfile);
>   });
> }
> 
>@@ -32,23 +32,33 @@
>   });
> }
> 
>-function testStart() {
>-  gPanel.controller.start(function (err) {
>-    ok(!err, "Profiler started without errors");
>-    testActive(testStop);
>+function startFirstProfile() {
>+  gPanel.controller.start("Profile 1", function (err) {
>+    ok(!err, "Profile 1 started without errors");
>+    testActive(startSecondProfile);
>   });
> }
> 
>-function testStop() {
>-  gPanel.controller.stop(function (err, data) {
>-    ok(!err, "Profiler stopped without errors");
>+function startSecondProfile() {
>+  gPanel.controller.start("Profile 2", function (err) {
>+    ok(!err, "Profile 2 started without errors");
>+    testActive(stopFirstProfile);
>+  });
>+}
>+
>+function stopFirstProfile() {
>+  gPanel.controller.stop("Profile 1", function (err, data) {
>+    ok(!err, "Profile 1 stopped without errors");
>     ok(data, "Profiler returned some data");
> 
>-    testInactive(function () {
>-      tearDown(gTab, function () {
>-        gTab = null;
>-        gPanel = null;
>-      });
>-    });
>+    testActive(stopSecondProfile);
>   });
> }
>+
>+function stopSecondProfile() {
>+  gPanel.controller.stop("Profile 2", function (err, data) {
>+    ok(!err, "Profile 2 stopped without errors");
>+    ok(data, "Profiler returned some data");
>+    testInactive(tearDown.call(null, gTab, function () gTab = gPanel = null));
>+  });
>+}
>\ No newline at end of file
>diff --git a/browser/devtools/profiler/test/head.js b/browser/devtools/profiler/test/head.js
>--- a/browser/devtools/profiler/test/head.js
>+++ b/browser/devtools/profiler/test/head.js
>@@ -80,4 +80,4 @@
> 
>     finish();
>   });
>-}
>\ No newline at end of file
>+}

Comment 8

[Anton Kovalyov (:anton)]

Moved the part 1 patch to bug 853650, will land it separately.

Comment 9

[Anton Kovalyov (:anton)]

Attachment: Connect to the profiler on toolbox start

Problem: we need to receive notifications about console.profile and console.profileEnd whenever the toolbox is open, even if the profiler tab has never been opened. Currently, we create a ProfilerController instance only when the tab is open. This patch changes it so that we create an instance every time the toolbox is opened for a tab.

Any downsides I missed?

If you have a question why don't I just do everything server-side, we don't want to start the profiler unless the developer toolbox is open (performance/security). And there's no way to know that without a signal from the client that sets some kind of a flag on the server (to be implemented).

Comment 10

[Dave Camp (:dcamp)]

(In reply to Anton Kovalyov (:anton) from comment #9)
> Created attachment 736051 [details] [diff] [review]
> Connect to the profiler on toolbox start
> 
> Problem: we need to receive notifications about console.profile and
> console.profileEnd whenever the toolbox is open, even if the profiler tab
> has never been opened. Currently, we create a ProfilerController instance
> only when the tab is open. This patch changes it so that we create an
> instance every time the toolbox is opened for a tab.
> 
> Any downsides I missed?
> 
> If you have a question why don't I just do everything server-side, we don't
> want to start the profiler unless the developer toolbox is open
> (performance/security). And there's no way to know that without a signal
> from the client that sets some kind of a flag on the server (to be
> implemented).

I don't see any significant downsides, but won't the fact that there's a connection to the debug server at all be enough signal that a toolbox (or some other potentially-interested party) exists?

Comment 11

[Anton Kovalyov (:anton)]

(In reply to Dave Camp (:dcamp) from comment #10)
> 
> I don't see any significant downsides, but won't the fact that there's a
> connection to the debug server at all be enough signal that a toolbox (or
> some other potentially-interested party) exists?

Well yeah we don't need a flag I guess. But we still need to connect on toolbox open.

Comment 12

[Dave Camp (:dcamp)]

(In reply to Anton Kovalyov (:anton) from comment #11)
> (In reply to Dave Camp (:dcamp) from comment #10)
> > 
> > I don't see any significant downsides, but won't the fact that there's a
> > connection to the debug server at all be enough signal that a toolbox (or
> > some other potentially-interested party) exists?
> 
> Well yeah we don't need a flag I guess. But we still need to connect on
> toolbox open.

Yeah, the code looks fine as-is.

Comment 13

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

Comment on attachment 736051 [details] [diff] [review]
Connect to the profiler on toolbox start

looks fine to me

Comment 14

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

Comment on attachment 736051 [details] [diff] [review]
Connect to the profiler on toolbox start

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

LGTM.

Comment 15

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

This patch is somewhat broken and doesn't have tests. Will fix tomorrow. Uploading to share progress. Almost there!

Comment 16

[Anton Kovalyov (:anton)]

Working on the urgent multi-threaded cleopatra bug so this one is up for next week now.

Comment 17

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

Rebased and got around iframe issues so console.profile and console.profileEnd work now. Need to cleanup the code and write tests still.

Comment 18

[Anton Kovalyov (:anton)]

Comment on attachment 748302 [details] [diff] [review]
Support for console.profile and console.profileEnd

I'm working on tests now but the profiler code is ready to be reviewed I think. One last thing I will need to do (besides the overall cleanup and comments) is to make sure we don't do anything when the Profiler Addon is installed.

Comment 19

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

Now with tests! Still remaining: profiler addon support and comments/cleanup.

Comment 20

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

Cleaned up the patch and added code to detect Gecko Profiler Addon.

Comment 21

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

Rebased. Try: https://tbpl.mozilla.org/?tree=Try&rev=ac799b5ded6b

Comment 22

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

I think I fixed the failures.

Comment 23

[Anton Kovalyov (:anton)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=806796625fed

Comment 24

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

Forgot to import gDevTools. Tests still failing. :-(

Comment 25

[Dave Camp (:dcamp)]

Attachment: my changes

This should work.

Comment 26

[Dave Camp (:dcamp)]

(In reply to Dave Camp (:dcamp) from comment #25)
> Created attachment 749947 [details] [diff] [review]
> my changes
> 
> This should work.

(it doesn't, but it *should*)

Comment 27

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

I think I fixed all the webconsole failures and made sure that profiler tests still pass. Fingers crossed.

(I can't push to try right now. Will update with the link once resolved)

Comment 28

[Dave Camp (:dcamp)]

Comment on attachment 750159 [details] [diff] [review]
Support for console.profile and console.profileEnd

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

Clearing my review since it was pointed out on irc that this doesn't work from content script, but it looks more or less ok to me.

::: browser/devtools/framework/gDevTools.jsm
@@ +607,5 @@
>  
>    /**
> +   * Connects to the SPS profiler when the developer tools are open.
> +   */
> +  _connectToProfiler: function DT_connectToProfiler() {

Could this logic be in ProfilerController.jsm?  The gDevTools module tries to be pretty minimal, with as little per-tool stuff as possible.

::: browser/devtools/framework/target.js
@@ +207,5 @@
>      return this._form;
>    },
>  
> +  get root() {
> +    return this._root;

I know I named this function, but root isn't quite the right name here.  Maybe "globalActors"?

@@ +290,4 @@
>      if (this.isLocalTab) {
>        this._client.connect((aType, aTraits) => {
>          this._client.listTabs(aResponse => {
> +          this._root = aResponse;

And maybe we should delete globalActors.tabs so that globalActors is an accurate name (which won't be possible with a local packet because it's frozen, so might need to deep copy).

::: browser/devtools/profiler/ProfilerController.jsm
@@ +230,5 @@
> +        cb();
> +      });
> +    };
> +
> +    if (this.target.root) {

Maybe just make this.target.globalActors a promise that is resolved by listTabs and kicks it off as needed?

Comment 29

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

(In reply to Dave Camp (:dcamp) from comment #28)
> @@ +290,4 @@
> >      if (this.isLocalTab) {
> >        this._client.connect((aType, aTraits) => {
> >          this._client.listTabs(aResponse => {
> > +          this._root = aResponse;
> 
> And maybe we should delete globalActors.tabs so that globalActors is an
> accurate name (which won't be possible with a local packet because it's
> frozen, so might need to deep copy).

This is how we do it in the console:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/HUDService.jsm#656

Comment 30

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

Comment on attachment 750159 [details] [diff] [review]
Support for console.profile and console.profileEnd

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

I think Dave got all the important bits.

::: browser/devtools/profiler/ProfilerPanel.jsm
@@ +175,5 @@
> +   *
> +   * @param function startFn
> +   *        A function to use instead of the default
> +   *        this.panel.startProfiling. Useful when you
> +   *        need mark panel as started after the profiler

Nit: grammar.

@@ +201,5 @@
> +   *
> +   * @param function stopFn
> +   *        A function to use instead of the default
> +   *        this.panel.stopProfiling. Useful when you
> +   *        need mark panel as stopped after the profiler

Ditto.

Comment 31

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

Comment on attachment 750159 [details] [diff] [review]
Support for console.profile and console.profileEnd

clearing review request based on others' comments.

Resubmit when this works from content.

Comment 32

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

Fixed race conditions that were breaking the profiler. Works from content now.

Try: https://tbpl.mozilla.org/?tree=Try&rev=142af6bb4a42

Comment 33

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

There were xpcshell test failures in the previous push try. This patch fixes them. Carrying over dcamp's r+ because the fix was literally "return undefined;" added to the end of an anonymous function.

Comment 34

[Anton Kovalyov (:anton)]

https://hg.mozilla.org/integration/fx-team/rev/4f2586fe4489

Comment 35

[Anton Kovalyov (:anton)]

Backed out: https://hg.mozilla.org/integration/fx-team/rev/33c3d03ff3c1 Will re-land in a sec.

Comment 36

[Dave Camp (:dcamp)]

RE-LAND MOTHAFUCKA

Comment 37

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

Removed dump() calls, carrying over r+.

fx-team $ hg qdiff | grep "dump("
fx-team $

Comment 38

[Anton Kovalyov (:anton)]

Attachment: Support for console.profile and console.profileEnd

This patch will be the death of me. Forgot to qref.

Comment 39

[Anton Kovalyov (:anton)]

Re-landed: https://hg.mozilla.org/integration/fx-team/rev/cfcce7c5eb74

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/cfcce7c5eb74