Closed Bug 817836 Opened 9 years ago Closed 8 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: rcampbell, Assigned: anton)

References

Details

Attachments

(1 file, 14 obsolete files)

38.22 KB, patch
anton
: review+
Details | Diff | Splinter Review
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.
As mention on IRC ideally we should allow the profiler add-on to overwrite these.
Component: Developer Tools → Developer Tools: Profiler
bumping priority on this.
Priority: -- → P2
Assignee: nobody → anton
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.
Attachment #726409 - Flags: review?(past)
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? :-)
Attachment #726409 - Flags: review?(past) → review+
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.
Depends on: 853650
(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 on attachment 726409 [details] [diff] [review]
Pt. 1: Support for multiple simultaneously running profiles

># HG changeset patch
># User Anton Kovalyov <anton@mozilla.com>
># 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
>+}
Attachment #726409 - Attachment is obsolete: true
Moved the part 1 patch to bug 853650, will land it separately.
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).
Attachment #736051 - Flags: feedback?(vporof)
Attachment #736051 - Flags: feedback?(rcampbell)
Attachment #736051 - Flags: feedback?(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?
(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.
(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 on attachment 736051 [details] [diff] [review]
Connect to the profiler on toolbox start

looks fine to me
Attachment #736051 - Flags: feedback?(rcampbell) → feedback+
Comment on attachment 736051 [details] [diff] [review]
Connect to the profiler on toolbox start

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

LGTM.
Attachment #736051 - Flags: feedback?(vporof) → feedback+
This patch is somewhat broken and doesn't have tests. Will fix tomorrow. Uploading to share progress. Almost there!
Attachment #736051 - Attachment is obsolete: true
Attachment #736051 - Flags: feedback?(dcamp)
Working on the urgent multi-threaded cleopatra bug so this one is up for next week now.
Rebased and got around iframe issues so console.profile and console.profileEnd work now. Need to cleanup the code and write tests still.
Attachment #738885 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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.
Attachment #748302 - Flags: review?(rcampbell)
Attachment #748302 - Flags: review?(past)
Now with tests! Still remaining: profiler addon support and comments/cleanup.
Attachment #748302 - Attachment is obsolete: true
Attachment #748302 - Flags: review?(rcampbell)
Attachment #748302 - Flags: review?(past)
Attachment #749098 - Flags: review?(rcampbell)
Attachment #749098 - Flags: review?(past)
Cleaned up the patch and added code to detect Gecko Profiler Addon.
Attachment #749098 - Attachment is obsolete: true
Attachment #749098 - Flags: review?(rcampbell)
Attachment #749098 - Flags: review?(past)
Attachment #749142 - Flags: review?(rcampbell)
Attachment #749142 - Flags: review?(past)
Rebased. Try: https://tbpl.mozilla.org/?tree=Try&rev=ac799b5ded6b
Attachment #749142 - Attachment is obsolete: true
Attachment #749142 - Flags: review?(rcampbell)
Attachment #749142 - Flags: review?(past)
Attachment #749157 - Flags: review?(rcampbell)
Attachment #749157 - Flags: review?(past)
I think I fixed the failures.
Attachment #749157 - Attachment is obsolete: true
Attachment #749157 - Flags: review?(rcampbell)
Attachment #749157 - Flags: review?(past)
Attachment #749442 - Flags: review?(rcampbell)
Attachment #749442 - Flags: review?(past)
Forgot to import gDevTools. Tests still failing. :-(
Attachment #749442 - Attachment is obsolete: true
Attachment #749442 - Flags: review?(rcampbell)
Attachment #749442 - Flags: review?(past)
Attachment #749525 - Flags: review?(rcampbell)
Attachment #749525 - Flags: review?(past)
Attached patch my changes (obsolete) — Splinter Review
This should work.
(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*)
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)
Attachment #749525 - Attachment is obsolete: true
Attachment #749947 - Attachment is obsolete: true
Attachment #749525 - Flags: review?(rcampbell)
Attachment #749525 - Flags: review?(past)
Attachment #750159 - Flags: review?(rcampbell)
Attachment #750159 - Flags: review?(past)
Attachment #750159 - Flags: review?(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?
Attachment #750159 - Flags: review?(dcamp)
(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 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.
Attachment #750159 - Flags: review?(past)
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.
Attachment #750159 - Flags: review?(rcampbell)
Fixed race conditions that were breaking the profiler. Works from content now.

Try: https://tbpl.mozilla.org/?tree=Try&rev=142af6bb4a42
Attachment #750159 - Attachment is obsolete: true
Attachment #754023 - Flags: review?(rcampbell)
Attachment #754023 - Flags: review?(past)
Attachment #754023 - Flags: review?(dcamp)
Attachment #754023 - Flags: review?(dcamp) → review+
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.
Attachment #754023 - Attachment is obsolete: true
Attachment #754023 - Flags: review?(rcampbell)
Attachment #754023 - Flags: review?(past)
Attachment #754958 - Flags: review+
Backed out: https://hg.mozilla.org/integration/fx-team/rev/33c3d03ff3c1 Will re-land in a sec.
RE-LAND MOTHAFUCKA
Removed dump() calls, carrying over r+.

fx-team $ hg qdiff | grep "dump("
fx-team $
Attachment #754958 - Attachment is obsolete: true
Attachment #754969 - Flags: review+
This patch will be the death of me. Forgot to qref.
Attachment #754969 - Attachment is obsolete: true
Attachment #754973 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/cfcce7c5eb74
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.