866642 - We need to add general telemetry / FHR info to devtools

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

General
    How many users actually use devtools?
    Are our tools performing well? (e.g. packet times)
    Which features are valuable to users, and thus deserve investment?

Implementation questions:
    Should we use FHR or Telemetry? (Telemetry appears to be disabled by default)
    Is there some way to differentiate between Nightly, Aurora, Beta and Release versions?

How many people opened each of these? How often?
    Web Console
    Markup View
    Rule View
    Computed View
    Font Inspector
    Box Model
    Debugger
    Style Editor
    Profiler
    Network Panel
    Developer Toolbar
    Browser Debugger
    Browser Console
    Responsive View
    Scratchpad
    Tilt
    Paint flashing

For a more complete list of possible probes see:
https://etherpad.mozilla.org/devtools-telemetry

I will contact Metrics for feedback.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch

Comment 2

[Joe Walker [:jwalker]]

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

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

We can have tests for the parts that don't talk to remote services at least can't we?

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +2067,5 @@
>      var target = devtools.TargetFactory.forTab(tab);
>      target.off("navigate", fireChange);
>      target.once("navigate", fireChange);
> +
> +    var window = gBrowser.contentWindow;

'context.environment.window;' would probably be better here - that way we're getting at the actual target window, not whatever gBrowser points at.

::: browser/devtools/framework/toolbox.js
@@ +195,5 @@
>  
> +    this._host.create().then(iframe => {
> +      let domReady = () => {
> +        // We use a try catch to prevent promises and event listeners from
> +        // swallowing errors.

I think that the right thing to do, is to find the top-most .then() call (i.e. the thing that is dropping the error) and adding an error handler to it.

   longStringOfPromises.then(handler, console.error);

Or even:

   longStringOfPromises.then(handler)
                       .then(null, console.error);

The latter has the advantage that it handles exceptions thrown by 'handler' too.

@@ +347,2 @@
>        container.appendChild(button);
> +    });

Or even:

    buttons.forEach(container.appendChild);

?

@@ +733,5 @@
>      let container = this.doc.getElementById("toolbox-buttons");
> +    let buttons = container.querySelectorAll("toolbarbutton");
> +
> +    for (let button of buttons) {
> +      button.parentNode.removeChild(button);

I'm not sure I understand this change - could you explain?

::: browser/devtools/scratchpad/scratchpad-manager.jsm
@@ +15,5 @@
>  const SCRATCHPAD_WINDOW_FEATURES = "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no";
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +let {Telemetry} = Cu.import("resource:///modules/devtools/shared/telemetry.js", {});

Are we actually using this?

::: browser/devtools/shared/telemetry.js
@@ +25,5 @@
> + *      or maybe
> + *      loader.lazyGetter(this, "Telemetry", () => require("devtools/shared/telemetry"));
> + *
> + * 4. Create a telemetry instance in your tool's constructor:
> + *      this._telemetry = new Telemetry();

Is it worth considering if we can combine steps 2 and 4?

    this._telemetry = new Telemetry({
      histogram: "DEVTOOLS_BLAH_OPENED_BOOLEAN",
      userHistogram: "DEVTOOLS_BLAH_OPENED_PER_USER_FLAG",
      timerHistogram: "DEVTOOLS_BLAH_TIME_ACTIVE_SECONDS"
    });

@@ +252,5 @@
> +      this.log(perUserHistogram, value);
> +    }
> +  },
> +
> +  destroy: function TM_destroy() {

The debugger team are going around removing function names from everywhere because the js-engine is now better at naming functions that we are. I suggest we don't add them any more.

::: browser/devtools/tilt/CmdTilt.jsm
@@ +7,5 @@
>  
> +var {utils: Cu} = Components;
> +
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +Cu.import("resource:///modules/devtools/gcli.jsm");

Cu.import("resource://gre/modules/devtools/gcli.jsm");

@@ +9,5 @@
> +
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +Cu.import("resource:///modules/devtools/gcli.jsm");
> +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> +Cu.import("resource:///modules/devtools/gDevTools.jsm");

Is that a double import?

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch v2

OMG: You really can tell that I had a lot of distractions when working on this.

(In reply to Joe Walker [:jwalker] from comment #2)
> Comment on attachment 746940 [details] [diff] [review]
> Patch
> 
> Review of attachment 746940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We can have tests for the parts that don't talk to remote services at least
> can't we?
> 

I wanted to do so but there is no telemetry info logged during tests so it can't really be tested. When I pinged the metrics guys about this I was told that it is disabled during tests in order to prevent the metrics from being skewed.

> ::: browser/devtools/commandline/BuiltinCommands.jsm
> @@ +2067,5 @@
> >      var target = devtools.TargetFactory.forTab(tab);
> >      target.off("navigate", fireChange);
> >      target.once("navigate", fireChange);
> > +
> > +    var window = gBrowser.contentWindow;
> 
> 'context.environment.window;' would probably be better here - that way we're
> getting at the actual target window, not whatever gBrowser points at.
> 

It probably would if context.environment.window was defined at this point. I have logged bug 874023 about this error.

> ::: browser/devtools/framework/toolbox.js
> @@ +195,5 @@
> >  
> > +    this._host.create().then(iframe => {
> > +      let domReady = () => {
> > +        // We use a try catch to prevent promises and event listeners from
> > +        // swallowing errors.
> 
> I think that the right thing to do, is to find the top-most .then() call
> (i.e. the thing that is dropping the error) and adding an error handler to
> it.
> 
>    longStringOfPromises.then(handler, console.error);
> 
> Or even:
> 
>    longStringOfPromises.then(handler)
>                        .then(null, console.error);
> 
> The latter has the advantage that it handles exceptions thrown by 'handler'
> too.
> 

I have tried a few variations but they don't seem as reliable as the try catch. For the moment I have removed the try catch and logged bug 874021 to properly address this issue across our tools.

> @@ +347,2 @@
> >        container.appendChild(button);
> > +    });
> 
> Or even:
> 
>     buttons.forEach(container.appendChild);
> 
> ?
> 

Yeah, I had tried that but it throws "Value does not implement interface Node." I have often wondered why this happens when using native functions inside Array.forEach() but never enough to fully investigate. We should ask jimb about this.

> @@ +733,5 @@
> >      let container = this.doc.getElementById("toolbox-buttons");
> > +    let buttons = container.querySelectorAll("toolbarbutton");
> > +
> > +    for (let button of buttons) {
> > +      button.parentNode.removeChild(button);
> 
> I'm not sure I understand this change - could you explain?
> 

A relic from a previous iteration, reverted.

> ::: browser/devtools/scratchpad/scratchpad-manager.jsm
> @@ +15,5 @@
> >  const SCRATCHPAD_WINDOW_FEATURES = "chrome,titlebar,toolbar,centerscreen,resizable,dialog=no";
> >  
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  
> > +let {Telemetry} = Cu.import("resource:///modules/devtools/shared/telemetry.js", {});
> 
> Are we actually using this?
> 

A relic from a previous iteration, reverted.

> ::: browser/devtools/shared/telemetry.js
> @@ +25,5 @@
> > + *      or maybe
> > + *      loader.lazyGetter(this, "Telemetry", () => require("devtools/shared/telemetry"));
> > + *
> > + * 4. Create a telemetry instance in your tool's constructor:
> > + *      this._telemetry = new Telemetry();
> 
> Is it worth considering if we can combine steps 2 and 4?
> 
>     this._telemetry = new Telemetry({
>       histogram: "DEVTOOLS_BLAH_OPENED_BOOLEAN",
>       userHistogram: "DEVTOOLS_BLAH_OPENED_PER_USER_FLAG",
>       timerHistogram: "DEVTOOLS_BLAH_TIME_ACTIVE_SECONDS"
>     });
> 

I don't like that idea for two reasons:
- Having all of the histogram names in one place makes the code more maintainable because we can see at a glance which histograms we have for each tool.
- We would lose functionality. At the moment the toolbox uses a single instance of Telemetry and sends the appropriate id depending which tool was opened or closed. If we used your proposed model we would need an instance of Telemetry for each tool and use each depending on the id.

> @@ +252,5 @@
> > +      this.log(perUserHistogram, value);
> > +    }
> > +  },
> > +
> > +  destroy: function TM_destroy() {
> 
> The debugger team are going around removing function names from everywhere
> because the js-engine is now better at naming functions that we are. I
> suggest we don't add them any more.
> 

Removed

> ::: browser/devtools/tilt/CmdTilt.jsm
> @@ +7,5 @@
> >  
> > +var {utils: Cu} = Components;
> > +
> > +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> > +Cu.import("resource:///modules/devtools/gcli.jsm");
> 
> Cu.import("resource://gre/modules/devtools/gcli.jsm");
> 
> @@ +9,5 @@
> > +
> > +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> > +Cu.import("resource:///modules/devtools/gcli.jsm");
> > +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> > +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> 
> Is that a double import?

It was code from a previous iteration ... I probably didn't undo enough times in that file. Reverted.

Comment 4

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> Created attachment 751652 [details] [diff] [review]
> Patch v2
> 
> OMG: You really can tell that I had a lot of distractions when working on
> this.
> 
> (In reply to Joe Walker [:jwalker] from comment #2)
> > Comment on attachment 746940 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 746940 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > We can have tests for the parts that don't talk to remote services at least
> > can't we?
> > 
> 
> I wanted to do so but there is no telemetry info logged during tests so it
> can't really be tested. When I pinged the metrics guys about this I was told
> that it is disabled during tests in order to prevent the metrics from being
> skewed.

I'm fairly sure we can find something that we can test. How about monkey patching telemetry in the test like:

var counter = 0;
var realToolOpened = Telemetry.prototype.toolOpened;

Telemetry.prototype.toolOpened = function() {
  counter++;
  realToolOpened.apply(this, arguments);
};

// Open a tool

is(counter, 1, ...);

Telemetry.prototype.toolOpened = realToolOpened;


> > ::: browser/devtools/framework/toolbox.js
> > @@ +195,5 @@
> > >  
> > > +    this._host.create().then(iframe => {
> > > +      let domReady = () => {
> > > +        // We use a try catch to prevent promises and event listeners from
> > > +        // swallowing errors.
> > 
> > I think that the right thing to do, is to find the top-most .then() call
> > (i.e. the thing that is dropping the error) and adding an error handler to
> > it.
> > 
> >    longStringOfPromises.then(handler, console.error);
> > 
> > Or even:
> > 
> >    longStringOfPromises.then(handler)
> >                        .then(null, console.error);
> > 
> > The latter has the advantage that it handles exceptions thrown by 'handler'
> > too.
> > 
> 
> I have tried a few variations but they don't seem as reliable as the try
> catch. For the moment I have removed the try catch and logged bug 874021 to
> properly address this issue across our tools.

I really don't think we need this. I don't know of a case where Promises swallow errors incorrectly if we use them properly.

Generally speaking, at the end of a chain of promises, we should do something about errors - it's failure to do that that causes the problems. If you're not sure about where the problem is being ignored, then you can set Promise._reportErrors = true; to have them logged.

> > @@ +347,2 @@
> > >        container.appendChild(button);
> > > +    });
> > 
> > Or even:
> > 
> >     buttons.forEach(container.appendChild);
> > 
> > ?
> > 
> 
> Yeah, I had tried that but it throws "Value does not implement interface
> Node." I have often wondered why this happens when using native functions
> inside Array.forEach() but never enough to fully investigate. We should ask
> jimb about this.

buttons.forEach(container.appendChild.bind(container));
?

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch v3

(In reply to Joe Walker [:jwalker] from comment #4)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> > Created attachment 751652 [details] [diff] [review]
> > Patch v2
> > 
> > OMG: You really can tell that I had a lot of distractions when working on
> > this.
> > 
> > (In reply to Joe Walker [:jwalker] from comment #2)
> > > Comment on attachment 746940 [details] [diff] [review]
> > > Patch
> > > 
> > > Review of attachment 746940 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > We can have tests for the parts that don't talk to remote services at least
> > > can't we?
> > > 
> > 
> > I wanted to do so but there is no telemetry info logged during tests so it
> > can't really be tested. When I pinged the metrics guys about this I was told
> > that it is disabled during tests in order to prevent the metrics from being
> > skewed.
> 
> I'm fairly sure we can find something that we can test. How about monkey
> patching telemetry in the test like:
> 
> var counter = 0;
> var realToolOpened = Telemetry.prototype.toolOpened;
> 
> Telemetry.prototype.toolOpened = function() {
>   counter++;
>   realToolOpened.apply(this, arguments);
> };
> 
> // Open a tool
> 
> is(counter, 1, ...);
> 
> Telemetry.prototype.toolOpened = realToolOpened;
> 
> 

Monkey patching is not an option as require returns a fresh object every time. If we were using jsm modules this would not have been a problem.

To workaround the issue we now trigger a devtools-telemetry-ping observer from telemetry.js. This observer contains the histogram name and the value that we are sending. The test file browser_telemetry.js uses these observers to test all tool's telemetry stats are written correctly ... at least all of them that run in the current thread.

> > > ::: browser/devtools/framework/toolbox.js
> > > @@ +195,5 @@
> > > >  
> > > > +    this._host.create().then(iframe => {
> > > > +      let domReady = () => {
> > > > +        // We use a try catch to prevent promises and event listeners from
> > > > +        // swallowing errors.
> > > 
> > > I think that the right thing to do, is to find the top-most .then() call
> > > (i.e. the thing that is dropping the error) and adding an error handler to
> > > it.
> > > 
> > >    longStringOfPromises.then(handler, console.error);
> > > 
> > > Or even:
> > > 
> > >    longStringOfPromises.then(handler)
> > >                        .then(null, console.error);
> > > 
> > > The latter has the advantage that it handles exceptions thrown by 'handler'
> > > too.
> > > 
> > 
> > I have tried a few variations but they don't seem as reliable as the try
> > catch. For the moment I have removed the try catch and logged bug 874021 to
> > properly address this issue across our tools.
> 
> I really don't think we need this. I don't know of a case where Promises
> swallow errors incorrectly if we use them properly.
> 
> Generally speaking, at the end of a chain of promises, we should do
> something about errors - it's failure to do that that causes the problems.
> If you're not sure about where the problem is being ignored, then you can
> set Promise._reportErrors = true; to have them logged.
> 

Which is what the new bug is about ... we don't do this often enough.

> > > @@ +347,2 @@
> > > >        container.appendChild(button);
> > > > +    });
> > > 
> > > Or even:
> > > 
> > >     buttons.forEach(container.appendChild);
> > > 
> > > ?
> > > 
> > 
> > Yeah, I had tried that but it throws "Value does not implement interface
> > Node." I have often wondered why this happens when using native functions
> > inside Array.forEach() but never enough to fully investigate. We should ask
> > jimb about this.
> 
> buttons.forEach(container.appendChild.bind(container));
> ?

Yup, that works.

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

We need to ensure that bc tests run multiple times on each platform to catch any intermittent errors, particularly those caused by GC issues, which are obviously a problem when opening all of our tools in close succession.

Comment 6

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> Monkey patching is not an option as require returns a fresh object every
> time. If we were using jsm modules this would not have been a problem.

Ooch - that's weird.
I'll ask about that.

...
> > Generally speaking, at the end of a chain of promises, we should do
> > something about errors - it's failure to do that that causes the problems.
> > If you're not sure about where the problem is being ignored, then you can
> > set Promise._reportErrors = true; to have them logged.
> > 
> 
> Which is what the new bug is about ... we don't do this often enough.

And we shouldn't. Promise._reportErrors is a global for debugging purposes only, and shouldn't ever be checked in.

Comment 7

[Joe Walker [:jwalker]]

Comment on attachment 752753 [details] [diff] [review]
Patch v3

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

Thanks.

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +13,5 @@
>  const DBG_STRINGS_URI = "chrome://browser/locale/devtools/debugger.properties";
>  const CHROME_DEBUGGER_PROFILE_NAME = "-chrome-debugger";
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +let {Telemetry} = Cu.import("resource:///modules/devtools/shared/telemetry.js", {});

Didn't think of this earlier, but I think we should probably be consistent about how we load telemetry.js. Either it's a js or it's a jsm. Especially if the rules are different on how they act.
It would be tragic if sometimes `instanceof Telemetry` worked and sometimes it didn't.

::: browser/devtools/scratchpad/scratchpad-manager.jsm
@@ +162,5 @@
>    uninit: function SDO_uninit()
>    {
>      Services.obs.removeObserver(this, "quit-application-granted");
>    }
> +};

We're added a blank line at the end of the file here? I'm not sure what this change is about?

::: browser/devtools/shared/telemetry.js
@@ +214,5 @@
> +   *         Histogram in which the data is to be stored.
> +   * @param  value
> +   *         Value to store.
> +   */
> +  log: function(histogramId, value) {

I wonder if we should make the log functions _log, because they're private?

::: browser/devtools/shared/test/browser_telemetry.js
@@ +57,5 @@
> +    // We use a timeout to check the tools active time
> +    setTimeout(function() {
> +      gDevTools.closeToolbox(target);
> +    }, TOOL_DELAY);
> +  }).then(null, info);

I think we need to do something here which would cause the test to fail if something goes wrong?

@@ +176,5 @@
> +  telemetryInfo = devtools = TargetFactory = target = null;
> +  tabsToTest = Services = Promise = null;
> +
> +  // Not sure why, but we leak window.target without this.
> +  delete window.target;

Ugg. I wonder what's causing that. I've had a quick look around, but I can't see anything. Perhaps you could add console.log(window.target) to a few points in the test to work out what's causing it? We might not need to fix the root cause right here, but I think it would be good to understand it. What do you think?

Comment 8

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] from comment #6)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> > Monkey patching is not an option as require returns a fresh object every
> > time. If we were using jsm modules this would not have been a problem.
> 
> Ooch - that's weird.
> I'll ask about that.

Paste this into a chrome scratchpad:

    var require = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools.require;
    
    var i1 = require("devtools/inspector/inspector-panel").InspectorPanel;
    var i2 = require("devtools/inspector/inspector-panel").InspectorPanel;
    
    i1 === i2; // true

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: patch v4

(In reply to Joe Walker [:jwalker] from comment #6)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> > Monkey patching is not an option as require returns a fresh object every
> > time. If we were using jsm modules this would not have been a problem.
> 
> Ooch - that's weird.
> I'll ask about that.
> 
> ...
> > > Generally speaking, at the end of a chain of promises, we should do
> > > something about errors - it's failure to do that that causes the problems.
> > > If you're not sure about where the problem is being ignored, then you can
> > > set Promise._reportErrors = true; to have them logged.
> > > 
> > 
> > Which is what the new bug is about ... we don't do this often enough.
> 
> And we shouldn't. Promise._reportErrors is a global for debugging purposes
> only, and shouldn't ever be checked in.

Agreed, but when we use promises we should clearly log errors using .then(null, console.log). We may have a bunch of errors that we don't know about because we are not consistent about doing this.

(In reply to Joe Walker [:jwalker] from comment #7)
> Comment on attachment 752753 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 752753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks.
> 
> ::: browser/devtools/debugger/DebuggerUI.jsm
> @@ +13,5 @@
> >  const DBG_STRINGS_URI = "chrome://browser/locale/devtools/debugger.properties";
> >  const CHROME_DEBUGGER_PROFILE_NAME = "-chrome-debugger";
> >  
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +let {Telemetry} = Cu.import("resource:///modules/devtools/shared/telemetry.js", {});
> 
> Didn't think of this earlier, but I think we should probably be consistent
> about how we load telemetry.js. Either it's a js or it's a jsm. Especially
> if the rules are different on how they act.
> It would be tragic if sometimes `instanceof Telemetry` worked and sometimes
> it didn't.
> 

We are now always using require.

> ::: browser/devtools/scratchpad/scratchpad-manager.jsm
> @@ +162,5 @@
> >    uninit: function SDO_uninit()
> >    {
> >      Services.obs.removeObserver(this, "quit-application-granted");
> >    }
> > +};
> 
> We're added a blank line at the end of the file here? I'm not sure what this
> change is about?
> 

Reverted

> ::: browser/devtools/shared/telemetry.js
> @@ +214,5 @@
> > +   *         Histogram in which the data is to be stored.
> > +   * @param  value
> > +   *         Value to store.
> > +   */
> > +  log: function(histogramId, value) {
> 
> I wonder if we should make the log functions _log, because they're private?
> 

No, I didn't make it private because it allows us to log anything we want using telemetry outside of the toolOpened / toolClosed methods, e.g. this.telemetry.log(histogram, value).

> ::: browser/devtools/shared/test/browser_telemetry.js
> @@ +57,5 @@
> > +    // We use a timeout to check the tools active time
> > +    setTimeout(function() {
> > +      gDevTools.closeToolbox(target);
> > +    }, TOOL_DELAY);
> > +  }).then(null, info);
> 
> I think we need to do something here which would cause the test to fail if
> something goes wrong?
> 

Done

> @@ +176,5 @@
> > +  telemetryInfo = devtools = TargetFactory = target = null;
> > +  tabsToTest = Services = Promise = null;
> > +
> > +  // Not sure why, but we leak window.target without this.
> > +  delete window.target;
> 
> Ugg. I wonder what's causing that. I've had a quick look around, but I can't
> see anything. Perhaps you could add console.log(window.target) to a few
> points in the test to work out what's causing it? We might not need to fix
> the root cause right here, but I think it would be good to understand it.
> What do you think?

Target was only declared inside functions so telemetryInfo = devtools = TargetFactory = target = null created window.target = null ... fixed.

(In reply to Joe Walker [:jwalker] from comment #8)
> (In reply to Joe Walker [:jwalker] from comment #6)
> > (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #5)
> > > Monkey patching is not an option as require returns a fresh object every
> > > time. If we were using jsm modules this would not have been a problem.
> > 
> > Ooch - that's weird.
> > I'll ask about that.
> 
> Paste this into a chrome scratchpad:
> 
>     var require = Cu.import("resource://gre/modules/devtools/Loader.jsm",
> {}).devtools.require;
>     
>     var i1 = require("devtools/inspector/inspector-panel").InspectorPanel;
>     var i2 = require("devtools/inspector/inspector-panel").InspectorPanel;
>     
>     i1 === i2; // true

I did, it worked and now I can't get it not to work :o/

I am not sure why it didn't work before ... anyhow, it works fine now so I have gone back to monkey patching.

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

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebase

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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