Closed Bug 1359855 Opened 7 years ago Closed 7 years ago

Don't initialize devtools right after first window paint

Categories

(DevTools :: General, enhancement, P1)

55 Branch
enhancement

Tracking

(Performance Impact:medium, firefox56 fixed)

RESOLVED FIXED
Firefox 56
Iteration:
56.4 - Aug 1
Performance Impact medium
Tracking Status
firefox56 --- fixed

People

(Reporter: mconley, Assigned: ochameau)

References

(Blocks 6 open bugs)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(8 files, 1 obsolete file)

59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Honza
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
It looks like we initialize DevTools stuff (and require() SDK things :( ) just after the first window paints:

http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/devtools/client/devtools-startup.js#51-61

In this profile: https://perf-html.io/public/75cea1de7333eb66c69f39778d9c21d21e3c2b67/calltree/?callTreeFilters=prefixjs-xHuBUpxHuBZ2BZ3&implementation=js&range=1.1996_1.2507&thread=0

That's taking a good 38ms on my uber-beefy MBP, which is twice our frame budget.

Can we do this more lazily? Perhaps when the first devtool is used?
Summary: Don't initialize devtools during → Don't initialize devtools right after first window paint
(In reply to Mike Conley (:mconley) - PTO on April 28th. from comment #0)
> Can we do this more lazily? Perhaps when the first devtool is used?

I know Alex has been making a lot of improvements here (including removing SDK dependencies), so forwarding this to him.
Flags: needinfo?(poirot.alex)
Flags: qe-verify?
Priority: -- → P2
I'm wondering how similar this report is to bug 1353548.

I'm afraid this is already quite minimal. I worked hard last year to reduce DevTools impact on Firefox startup. Last work was bug 1302996, to ensure we were loading only modules that was stricly necessary. (We may have regressed a bit since then, but trust me, it was wayyy more impactful [like loading tons of SDK modules])
What you see left is core devtools modules which listen for new browser window and hook up menus and key shortcuts.
Unfortunately, we have to execute this code at some point during Firefox startup.

We can surely delay the whole thing a bit if that's on a critical path. Would you suggest to listen to some other event fired after browser-delayed-startup-finished?

In bug 1353548 we will most likely try to delay some specific pieces just about menuitem creation, so that code may be slightly faster.
Finally, note that devtools are in process of becoming an addon, so we are most likely going to shuffle this code (bug 1356226).
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #2)
> I'm wondering how similar this report is to bug 1353548.

Bug 1353548 is about the overhead for each new window. Mike's profile here is about the first window. Bug 1353548 accounts for 10ms / 26% of the time shown here.

> I'm afraid this is already quite minimal.

38ms on a super fast MBP is not 'minimal'. I think loader.require can't be used here if it causes so much overhead. It's unclear to me why devtools needs anything during startup outside of registering some keyboard bindings and menu items (and that's only 26% of the total time here). What do you actually need to do here?
Flags: needinfo?(poirot.alex)
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> > I'm afraid this is already quite minimal.
> 
> 38ms on a super fast MBP is not 'minimal'. I think loader.require can't be
> used here if it causes so much overhead. It's unclear to me why devtools
> needs anything during startup outside of registering some keyboard bindings
> and menu items (and that's only 26% of the total time here). What do you
> actually need to do here?

I didn't looked in detail, but that code should be mostly doing that. Key shortcut, menu items and registering many observers.
But in order to do that and keep a sane level of code sharing, we end up loading a couple of modules: devtools-browser, devtools, definitions, l10n, ...

At first sight, what I see is mostly a significant cost in module loading.
It is still a unclear if that's SDK loader being slow (still), or if loading any piece of JS is a slow operation?
I imagine you would see similar cost if a significant JSM was loaded? Do you?
If not, it would be worth looking at SDK loader performances again.
This devtools code isn't doing much, it may just be loading too many bytes of JS.
We may be able to split these modules to reduce the size of JS to be evaluated here.

And please don't over react. That's not because I say it is quite minimal that you should translate it as it can't be optimized. I just think it is getting harder.
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #4)

> I imagine you would see similar cost if a significant JSM was loaded? Do you?

Yes, we see similar costs when a significant JSM has several dependencies and loads them directly instead of using lazy getters. Bug 1357116 is a recent example of this.

> If not, it would be worth looking at SDK loader performances again.

I need to check this assumption, but I'm currently hoping the SDK will disappear soon enough for us to not have to care about optimizing it.

> This devtools code isn't doing much, it may just be loading too many bytes
> of JS.
> We may be able to split these modules to reduce the size of JS to be
> evaluated here.

Maybe I didn't phrase this clearly, but this is what I meant. If the user hasn't interacted with devtools yet, we shouldn't be loading more than one .js or .jsm file related to devtools.
Whiteboard: [photon-performance][qf] → [photon-performance][qf:p1]
What is the target for these photon-performance bugs?

We are currently working on refactoring devtools to be an addon.
In bug 1361080, we are going to refactor this very precise code. In a way that looks like what Florian described in comment 5.

But we are targeting Firefox 56 for shipping devtools as an add-on. We will most likely land pieces of this refactoring during 55 timeframe, but there may be signifiant pieces still around. So I was wondering if that's fine or if we have to find mitigation plan?
Flags: needinfo?(mconley)
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> What is the target for these photon-performance bugs?

When possible, the idea is to land improvements immediately. This is all leading up to the 57 release though - fixes can and should land up until then. Not sure if that clears it up.

> We are currently working on refactoring devtools to be an addon.

I assume a non-SDK add-on?

> But we are targeting Firefox 56 for shipping devtools as an add-on. We will
> most likely land pieces of this refactoring during 55 timeframe, but there
> may be signifiant pieces still around. So I was wondering if that's fine or
> if we have to find mitigation plan?

If you can ensure that it's out before 57, and that it doesn't negatively impact start-up performance as an add-on, then yes, that sounds great to me. Is your team able to commit to that?
Flags: needinfo?(mconley) → needinfo?(poirot.alex)
(FWIW I'm pretty sure what you are seeing here is the pure overhead of module loading.  We've seen this issue a lot.  And I think waiting for the refactoring here makes perfect sense!)
(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #7)
> (In reply to Alexandre Poirot [:ochameau] from comment #6)
> > What is the target for these photon-performance bugs?
> 
> When possible, the idea is to land improvements immediately. This is all
> leading up to the 57 release though - fixes can and should land up until
> then. Not sure if that clears it up.

Ok, that makes sense, I was wondering if you were targetting 55/56 just for photon.

> > We are currently working on refactoring devtools to be an addon.
> 
> I assume a non-SDK add-on?

We are trying to drop all SDK dependencies (bug 1350645). All but the module loader!
DevTools uses common js modules, not JSMs.
So we end up using its module loader.

There is two alternatives to that:
* go full webpack, like the web, and load one gigantic file (this is was debugger.html already does)
* switch to es6 classes (bug 1298286, but I'm not sure es6 modules are ready yet)

> > But we are targeting Firefox 56 for shipping devtools as an add-on. We will
> > most likely land pieces of this refactoring during 55 timeframe, but there
> > may be signifiant pieces still around. So I was wondering if that's fine or
> > if we have to find mitigation plan?
> 
> If you can ensure that it's out before 57, and that it doesn't negatively
> impact start-up performance as an add-on, then yes, that sounds great to me.
> Is your team able to commit to that?

Yes, we want to switch to the add-on/github during 56 release. We will most likely do it during SF workweek, or the week after if there is any conflict.

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #8)
> (FWIW I'm pretty sure what you are seeing here is the pure overhead of
> module loading.  We've seen this issue a lot.  And I think waiting for the
> refactoring here makes perfect sense!)

I tend to think it isn't really a "module loading overhead".
You get the same performances issues if you start loading many significant JSMs.
I believe it is more about the "module dependencies trap".
Modules ease loading more JS as you write many modules and rarely think loading deps lazily.
I spent a quarter adding laziness to devtools in bug 1320786, and it depress me to see that things regressed again.

Note that we have DAMP on talos, dedicated to DevTools performances.
It rarely catch perf regressions. Firefox performance is subject to dark matter. We look at the time it takes to open/close the tools which is *very* random on each run. I would like to take some other measurements, anything but time. Like number of JS bytes evaluated, number of compartments being created, memory allocated, number of reflows, ...
Flags: needinfo?(poirot.alex)
Priority: P2 → P3
Whiteboard: [photon-performance][qf:p1] → [reserve-photon-performance][qf:p1]
Whiteboard: [reserve-photon-performance][qf:p1] → [reserve-photon-performance][qf:p2]
Flags: qe-verify? → qe-verify-
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> At first sight, what I see is mostly a significant cost in module loading.
> It is still a unclear if that's SDK loader being slow (still), or if loading
> any piece of JS is a slow operation?
> I imagine you would see similar cost if a significant JSM was loaded? Do you?
> If not, it would be worth looking at SDK loader performances again.

I don't think we're going to get much more performance out of the SDK loader at this point without rewriting it in C++. I'm not entirely above that if necessary, but it seems like we're hoping to phase it out as much as possible, so I suspect it isn't worth the effort.

And even if we did replace the loader with something more efficient, I'm not sure how much it would help. devtools are still loading quite a lot of scripts at startup, and the non-syntactic object scope that the modules run in is really bad for their JIT performance.

The last time I looked at that chunk of startup overhead, it seemed like the only things it was doing were:

1) Inserting a context menu item, and
2) Registering the JSON View content converter.

I think we should be able to implement stubs for those two functions in a single, self-contained script without more than a couple of ms of overhead. Unless we're doing more things that I'm missing.
I already looked into that when bug 1353548 was reported.
The main issue is key shortcuts. See bug 1353548 comment 1353548 and attachment 8855220 [details].

Long story short is that we can surely delay menu entry creation until users open "Web Developer" menu.
But we can't do that for key shortcuts. We probably can delay it to some later steps during Firefox startup.
i.e. it doesn't have to be during "browser-delayed-startup-finished". But it isn't clear if that would help?
What would be a later step during Firefox startup?

Then we could refactor main modules loaded by devtools on startup. This is where it becomes hairy and suggested waiting for devtools to become an add-on. As of today, addons can register a new tool with a key shortcut (registerTool). All that require most of DevTools machinery to be setup (devtools/client/framework/devtools.js) and the current implementation of menus and key shortcuts depends on that.

(In reply to Kris Maglione [:kmag] from comment #10)
> (In reply to Alexandre Poirot [:ochameau] from comment #4)
> > At first sight, what I see is mostly a significant cost in module loading.
> > It is still a unclear if that's SDK loader being slow (still), or if loading
> > any piece of JS is a slow operation?
> > I imagine you would see similar cost if a significant JSM was loaded? Do you?
> > If not, it would be worth looking at SDK loader performances again.
> 
> I don't think we're going to get much more performance out of the SDK loader
> at this point without rewriting it in C++. I'm not entirely above that if
> necessary, but it seems like we're hoping to phase it out as much as
> possible, so I suspect it isn't worth the effort.

Do you think that the SDK is almost as efficient as JSM now?
I don't see why it would be so much slower. At the very end we should only be doing Cu.Sandbox() and subscriptloader.loadSubScript.

> And even if we did replace the loader with something more efficient, I'm not
> sure how much it would help. devtools are still loading quite a lot of
> scripts at startup,

Significantly less than before! Before Q4 of last year it was still pulling the SDK world via the JSON viewer component. (bug 1302996)
But I have to agree, we still load too many bytes of JS and .properties, it now becomes harder and harder to shrink it down.

> and the non-syntactic object scope that the modules run
> in is really bad for their JIT performance.

That's the first time I hear such argument. Could you elaborate?
Isn't JIT only important for functions that do significant computation?
I'm not sure DevTools codebase is doing that during startup.

> The last time I looked at that chunk of startup overhead, it seemed like the
> only things it was doing were:
> 
> 1) Inserting a context menu item, and

This context menu is still hardcoded:
http://searchfox.org/mozilla-central/source/browser/base/content/browser-context.inc#460-464

> 2) Registering the JSON View content converter.

This used to be a major issue, especially during the content process startup (bug 1302996, bug 1317701).
We may followup on that if that appear to still be an issue.

3) Web Developer menu items

4) Key shortcuts
(In reply to Alexandre Poirot [:ochameau] from comment #11)

> But we can't do that for key shortcuts. We probably can delay it to some
> later steps during Firefox startup.
> i.e. it doesn't have to be during "browser-delayed-startup-finished". But it
> isn't clear if that would help?
> What would be a later step during Firefox startup?

You could split this code into small chunks and run each of them using window.requestIdleCallback (or Services.tm.idleDispatchToMainThread).

Although registering a dozen of keyboard shortcuts shouldn't be slow, so they could all be registered at once.

The main problem here seems to still be that this is require()'ing/import()'ing too much code. A single small javascript file should be enough for all of this.
(In reply to Alexandre Poirot [:ochameau] from comment #11)
> I already looked into that when bug 1353548 was reported.
> The main issue is key shortcuts. See bug 1353548 comment 1353548 and
> attachment 8855220 [details].
> 
> Long story short is that we can surely delay menu entry creation until users
> open "Web Developer" menu.
> But we can't do that for key shortcuts. We probably can delay it to some
> later steps during Firefox startup.

I don't think we need to delay it. Creating the element should be relatively
cheap, but loading and executing all of the devtools framework code that we
currently load in order to add them is not. If we stored a cache of the
elements we need to create during the previous session, and then use a stub
initializer script to create the elements, and then load the actual devtools
code when it's needed, I think that would be sufficient.

> Do you think that the SDK is almost as efficient as JSM now?
> I don't see why it would be so much slower. At the very end we should only
> be doing Cu.Sandbox() and subscriptloader.loadSubScript.

No, it's considerably more expensive, largely due to all of the module
resolution logic, combined with the loader objects and scope setup it needs to
do for every module it loads.

It has a slight benefit in that it doesn't create a separate
global/compartment for every module it loads, while the JSM loader currently
does. But the way it does it also means we get much worse JIT performance for
the code that it loads.

> > and the non-syntactic object scope that the modules run
> > in is really bad for their JIT performance.
>
> That's the first time I hear such argument. Could you elaborate?

When we load scripts into an object scope, they're essentially running as if
they were in a with(){} scope (with a few differences), which means that a lot
of JIT optimizations are turned off, particularly for Ion.

> Isn't JIT only important for functions that do significant computation?

Not really. Ion is mostly important for functions that do significant/complex
computation, but baseline is important for pretty much anything that runs more
than once (helper functions, loops, ...). That especially shows up in things
like the SDK utility code, and in particular the bits that are used widely,
like heritage classes.
(In reply to Kris Maglione [:kmag] from comment #13)
> I don't think we need to delay it. Creating the element should be relatively
> cheap, but loading and executing all of the devtools framework code that we
> currently load in order to add them is not. If we stored a cache of the
> elements we need to create during the previous session, and then use a stub
> initializer script to create the elements, and then load the actual devtools
> code when it's needed, I think that would be sufficient.

I started doing similar things for the add-on shim (bug 1378397), we may be able to reuse that.

> No, it's considerably more expensive, largely due to all of the module
> resolution logic, combined with the loader objects and scope setup it needs
> to
> do for every module it loads.

I tried to fork the SDK loader and shrink it to just what devtools need,
which is way simplier than the full featured loader:
https://hg.mozilla.org/try/rev/92e227288b2fb5de9a96600dbf9b04ef75e1ddd0
The module resolution is really simple. No need to check for file existence.
But I'm surprised to not see any significant win:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b8e5f17496102589a073de9d697d87564d10513d&newProject=try&newRevision=0afa3580e183e20a3dbbda54b780f7db6d1fc233&framework=1&showOnlyImportant=0
I'll try to continue removing what isn't necessary.

> It has a slight benefit in that it doesn't create a separate
> global/compartment for every module it loads, while the JSM loader currently
> does. But the way it does it also means we get much worse JIT performance for
> the code that it loads.

With such description, it looks like code evaluation should be faster,
as we don't have to create a new global and compartment.
We don't execute much code when starting up the tools,
what we see in many profiles is just JS loading.
Is a JITed code faster to load?

> When we load scripts into an object scope, they're essentially running as if
> they were in a with(){} scope (with a few differences), which means that a
> lot
> of JIT optimizations are turned off, particularly for Ion.

So, it isn't really about devtools codebase, but more about how the loader is implemented, right?
I imagine that subscript.loadSubScript is what introduce this behavior?
Isn't it the main culprit that makes modules loaded via the SDK loader particularely slow?
Do you think we can tweak loadSubScript to have a better implementation?
(In reply to Alexandre Poirot [:ochameau] from comment #14)
> I tried to fork the SDK loader and shrink it to just what devtools need,
> which is way simplier than the full featured loader:
> https://hg.mozilla.org/try/rev/92e227288b2fb5de9a96600dbf9b04ef75e1ddd0
> The module resolution is really simple. No need to check for file existence.
> But I'm surprised to not see any significant win:

That's not entirely surprising. Looking at the last profile I have of devtools
startup, it looks like we only have ~2ms of overhead per module in the loader,
plus a few more ms loading the loader, and a few more setting up the devtools
resolver. And then sub-millisecond overhead each time we require() a module
that's already loaded.

That probably adds up to something like 15-20ms of extra overhead (as a rough
estimate, since the profiler doesn't give me a good way to see how much time
is spent in just that module), but it's distributed in a bunch of small chunks
that aren't easy to optimize.

> > It has a slight benefit in that it doesn't create a separate
> > global/compartment for every module it loads, while the JSM loader currently
> > does. But the way it does it also means we get much worse JIT performance for
> > the code that it loads.
>
> With such description, it looks like code evaluation should be faster,
> as we don't have to create a new global and compartment.

Creating the global and compartment is fairly cheap. The extra overhead it
adds tends to be in memory and poorer JIT performance when a lot of
cross-compartment wrappers are involved.

But the SDK loader adds a lot of extra setup and resolution overhead, and its
non-syntactic scope objects lead to poorer JIT performance overall.

> We don't execute much code when starting up the tools,
> what we see in many profiles is just JS loading.

From the profiles I've looked at, you execute quite a lot, and some of it
(like the l10n code and the overhead for 13+ modules that you load) is quite
expensive.

> Is a JITed code faster to load?

It depends on the code, and how much actual execution it does as opposed to
just parsing. In the case of devtools, though, I'd say yes.

> > When we load scripts into an object scope, they're essentially running as if
> > they were in a with(){} scope (with a few differences), which means that a
> > lot
> > of JIT optimizations are turned off, particularly for Ion.
>
> So, it isn't really about devtools codebase, but more about how the loader
> is implemented, right?

In the specific case of JIT performance, yes, the fact that the loader relies
on non-syntactic scopes is bad for performance.

> I imagine that subscript.loadSubScript is what introduce this behavior?

No, loadSubScript can be used without any negative performance impact (at
least, since it started defaulting to not compiling scripts to return a
value). But using it to load scripts into a non-global context object is
problematic.

> Isn't it the main culprit that makes modules loaded via the SDK loader
> particularely slow?

Yes, the combination of loader overhead and non-syntactic scopes. But I don't
think we can really get rid of non-syntactic scopes at this point, because
that would cause memory usage to explode (at least until bug 1357862) is
fixed, so I think we should focus on minimizing the number of modules we need
to load at startup (and ideally not even use the SDK loader for anything
that's loaded eagerly).
Here is a tentative patch to make it so that devtools-startup.js is the only module loaded until user:
* open Web Developer menu
* press a set of devtools key shortcuts (for now only the most common ones, will need to be more complete)
* expand the Web Developer menu in the hamburger menu (still TODO)
* click on Developer tool wrench in the hamburger menu (still TODO, with latest photon this icon isn't displayed by default)
* use any devtools command like argument (-jsconsole, ...)

"Tentative patch", as there is still some work to be done:
* support more than one window. so far it only catches devtools related actions in the first firefox window
* cleanup things or remove the now-duplicated action done within devtools
* fix the TODOs

Early feedback is welcomed!
Comment on attachment 8885915 [details]
Bug 1359855 - Prevent loading any DevTools module until users interact with any devtool entrypoint.

https://reviewboard.mozilla.org/r/156698/#review161800

Looks like this is going in a nice direction :-)

::: devtools/client/devtools-startup.js:30
(Diff revision 1)
> +  const kUrl = "chrome://devtools/locale/key-shortcuts.properties";
> +  return Services.strings.createBundle(kUrl);
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "KeyShortcuts", function () {
> +  const isMac = Services.appinfo.OS === "Darwin";

new code tends to do AppConstants.platform == "macosx" these days.

::: devtools/client/devtools-startup.js:133
(Diff revision 1)
> +    // on "command" event from the system menu entries.
> +    // Even listening to an earlier even like "mousedown" isn't enough.
> +    // 1) we should ensure our listener to be called before the Photon UI code
> +    // 2) and that our code to fill up the system menu is *done* before Photon UI listener is fired.
> +    let hamburgerMenu = window.document.getElementById("appMenu-developer-button");
> +    hamburgerMenu.addEventListener("mousedown", () => this.initDevTools());

Should this use {once: true}?

::: devtools/client/devtools-startup.js:143
(Diff revision 1)
> +  hookWebDeveloperMenu(window) {
> +    let menu = window.document.getElementById("webDeveloperMenu");
> +    let onPopupShowing = () => {
> +      this.initDevTools();
> +    };
> +    menu.addEventListener("popupshowing", onPopupShowing);

same here
There is also missing:
* JSON Viewer. In the current state of this patch, it is only enabled once one of the listed action is done.
* Context menu "inspect element" is only visible if one action is done.

I'm wondering if I miss something else.
Comment on attachment 8885915 [details]
Bug 1359855 - Prevent loading any DevTools module until users interact with any devtool entrypoint.

https://reviewboard.mozilla.org/r/156698/#review161828

This looks good to me.

::: devtools/client/devtools-startup.js:136
(Diff revision 3)
> +    // on "command" event from the system menu entries.
> +    // Even listening to an earlier even like "mousedown" isn't enough.
> +    // 1) we should ensure our listener to be called before the Photon UI code
> +    // 2) and that our code to fill up the system menu is *done* before Photon UI listener is fired.
> +    let hamburgerMenu = window.document.getElementById("appMenu-developer-button");
> +    hamburgerMenu.addEventListener("mousedown", () => this.initDevTools(), { once: true });

I suspect you really want a ViewShowing event here.
Pushed an update supporting all the shortcuts while removing the code that used to manage the key shortcuts.
It introduces some non-trivial dependency between this new code that only creates the shortcuts early during startup (devtools-startup) and the other code that later create the menus (browser-menu).

I also inlined the main module of jsonview but I'm not sure that's the best option.
It may be sanier to at least keep a distinct jsm...

In its current state devtools-startup.js looks like a random pile of things, but Julian suggests to delegate command line implementation to a dedicated module as this code is only useful if you happen to use one command line, which is not the typical usecase. This sounds like a good followup.

Also, one unexpected thing I saw in tests is this:
  test left unexpected property on window: DeveloperToolbar
It is related to this code, run on devtools startup (devtools-browser.js)
  http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#510-514
Before this patch, this code used to run *before* mochitest starts (during Firefox/window startup), so it was in the whitelist of globals.
Now we do this lazily, so this function happen to be executed *after* the mochitest starts.
I tried to ensure calling the related cleanup function of devtools at the end of each test, but that may introduce yet another issues. Let's see what try says.
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Priority: P3 → P1
A few new conclusions:
* trying to cleanup devtools after each test is way too much to ask, it is seriously asynchronous and easily overlap between test runs. so instead of trying to do that, I added DeveloperToolbar in the whitelist of accepted globals on mochitest.
* we also register per-tool key shortcuts into toolbox loaded in a distinct top level window, it means that we have to fetch the KeyShortcuts definition out of devtools-startup from the devtools codebase.
* the issues related to the Hamburger menu and its "Web Developer" menu was just missing the implementation of "PanelUI-developer" view, which is now part of devtools-startup. (i.e. it wasn't a timig issue)
* in order to prevent having to introduce yet another shared-head to all devtools mochitests, I'm listening for an event specific to tests in order to initalize the tools before each mochitest
* addons that used to register a key shortcut for custom panel will stop having a functional key shortcut. it should only be an issue on 56 as such addon will be rejected on 57.
Blocks: 1380806
Blocks: 1378397
Comment on attachment 8886177 [details]
Bug 1359855 - Fix the developer toggle in customize widgets.

https://reviewboard.mozilla.org/r/156978/#review162814

::: devtools/client/devtools-startup.js:206
(Diff revision 2)
>  
>      // Ensure loading the tools early when running tests
>      window.addEventListener("mochitest-load", () => this.initDevTools(), { once: true });
>    },
>  
> +  hookDeveloperToggle(window) {

Can we add a comment here to explain where this toggle can be found, and that on click it displays a menu duplicating the entries found in the main webdevelopermenu, and that it is reused by the photon devtools menu-entry (I think?)

::: devtools/client/devtools-startup.js:222
(Diff revision 2)
> +      tooltiptext: "developer-button.tooltiptext2",
> +      defaultArea: AppConstants.MOZ_DEV_EDITION ?
> +                     CustomizableUI.AREA_NAVBAR :
> +                     CustomizableUI.AREA_PANEL,
> +      onViewShowing: (event) => {
> +        this.initDevTools();

Add a comment here to explain that calling initDevtools will populate the main menu.
Attachment #8886177 - Flags: review?(jdescottes) → review+
Comment on attachment 8885915 [details]
Bug 1359855 - Prevent loading any DevTools module until users interact with any devtool entrypoint.

https://reviewboard.mozilla.org/r/156698/#review162282

Works great! Clearing the review flag for now as I want to discuss about the properties file.
Also would be nice to add a few more comments in devtools-startup, since the initialization sequence for devtools is not trivial.

::: devtools/client/definitions.js:65
(Diff revision 5)
>  Tools.inspector = {
>    id: "inspector",
> -  accesskey: l10n("inspector.accesskey"),
> -  key: l10n("inspector.commandkey"),
>    ordinal: 1,
>    modifiers: osString == "Darwin" ? "accel,alt" : "accel,shift",

modifiers are no longer needed in here (same comment applies to each definition)

::: devtools/client/devtools-startup.js:35
(Diff revision 5)
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "KeyShortcuts", function () {
> +  const isMac = AppConstants.platform == "macosx";
> +
> +  // List of all key shortcuts triggering installation UI

This comment is not related to the next line, I would expect something mentioning that these modifiers are shared by most of the shortcuts.

::: devtools/client/devtools-startup.js:53
(Diff revision 5)
> +      modifiers
> +    },
> +    // All locales are using F12
> +    {
> +      id: "toggleToolboxF12",
> +      shortcut: "VK_F12",

Let's move this to the properties file for consistency.

::: devtools/client/devtools-startup.js:147
(Diff revision 5)
> +      modifiers
> +    },
> +  ];
> +});
>  
>  function DevToolsStartup() {}

Add a small comment here that explains the role of the component is both to handle command line arguments as well as to add various hooks to initialize devtools.

::: devtools/client/devtools-startup.js:178
(Diff revision 5)
> -                                  "browser-delayed-startup-finished");
> -      // Ensure loading core module once firefox is ready
> -      this.initDevTools();
>  
>        if (devtoolsFlag) {
>          this.handleDevToolsFlag(window);

We could call initDevTools() in handleDevToolsFlag() for consistency. 

handleDevToolsFlag loads Loader.jsm & devtools.js which will actually end up loading devtools-browser.js too when creating the toolbox.

::: devtools/client/devtools-startup.js:187
(Diff revision 5)
>        }
>      };
> -    Services.obs.addObserver(onStartup, "browser-delayed-startup-finished");
> +    Services.obs.addObserver(onWindowReady, "browser-delayed-startup-finished");
>    },
>  
> +  hookWindow(window) {

Add a comment here to explain the split between what is done in this component and what is done by devtools during their initialization.

::: devtools/client/devtools-startup.js:201
(Diff revision 5)
> +
> +    // Ensure loading the tools early when running tests
> +    window.addEventListener("mochitest-load", () => this.initDevTools(), { once: true });
> +  },
> +
> +  hookWebDeveloperMenu(window) {

Add a small JS doc to explain which menu is the "webDeveloperMenu" and also to say that the content of the menu will be created by the initialization of devtools

::: devtools/client/devtools-startup.js:206
(Diff revision 5)
> +  hookWebDeveloperMenu(window) {
> +    let menu = window.document.getElementById("webDeveloperMenu");
> +    let onPopupShowing = () => {
> +      this.initDevTools();
> +    };
> +    menu.addEventListener("popupshowing", onPopupShowing, { once: true });

Let's inline this as 
> menu.addEL("showing", () => this.initDevTools(), {once: true});

::: devtools/client/devtools-startup.js:241
(Diff revision 5)
> +  createKey(doc, { id, toolId, shortcut, modifiers: mod }, oncommand) {
> +    let k = doc.createElement("key");
> +    k.id = "key_" + (id || toolId);
> +
> +    if (shortcut.startsWith("VK_")) {
> +      k.setAttribute("keycode", shortcut);

We are no longer setting the keytext on VK_* keys. From reading MDN I suppose this was used to display F8 instead of VK_F8 etc... in the menus. But I don't see any VK_ displayed anywhere when testing.

Do you know if this is now handled somewhere else? automatically stripping VK_ from keys?

If this is the case, we should cleanup all remaining keytext properties as they are no longer used.

::: devtools/client/framework/browser-menus.js:187
(Diff revision 5)
>   *
>   * @param {XULDocument} doc
>   *        The document to which keys and menus are to be added.
>   */
>  function addTopLevelItems(doc) {
>    let keys = doc.createDocumentFragment();

remove `keys`: no longer used

::: devtools/client/locales/en-US/key-shortcuts.properties:1
(Diff revision 5)
> +# This Source Code Form is subject to the terms of the Mozilla Public

This file duplicates shortcuts from both startup.properties & menus.properties. 

The ones in menus.properties are no longer used and should be removed in any case.

I think it would make sense to move towards a single properties file containing all the command keys, access keys and labels for our menus. 

I want to agree on a strategy here before duplicating a dozen of new strings to localize:
- duplicate. In this case: 1 loc note per duped item is mandatory + explicitly say it should be synced with the one still found in startup.properties
- move all commandkeys, accesskeys, menulabels to this file, and use MultiLocalizationHelper in browser-menus.js and definitions.js to fetch strings from the 2 prop files

::: devtools/client/menus.js:50
(Diff revision 5)
>      key: {
>        id: "devToolboxMenuItem",
>        modifiers: isMac ? "accel,alt" : "accel,shift",
>        // This is the only one with a letter key
>        // and needs to be translated differently
>        keytext: true,
>      },

This key entry should be deleted
Comment on attachment 8886178 [details]
Bug 1359855 - Inline jsonview main module to keep it working before user action.

https://reviewboard.mozilla.org/r/156980/#review162090

Clearing mostly to look at the destroy.

::: devtools/client/devtools-startup.js:190
(Diff revision 2)
>          devtoolsFlag = false;
>        }
>      };
>      Services.obs.addObserver(onWindowReady, "browser-delayed-startup-finished");
> +
> +    JsonView.initialize();

Is there a good reason to initialize it here rather than in onWindowReady()? If yes, add a comment.

::: devtools/client/devtools-startup.js:521
(Diff revision 2)
> +    // Register for messages coming from the child process.
> +    Services.ppmm.addMessageListener(
> +      "devtools:jsonview:save", this.onSave);
> +  },
> +
> +  destroy: function () {

We are no longer calling destroy at any point. We should probably observe [quit-application] and run some cleanup code?
Attachment #8886178 - Flags: review?(jdescottes)
Comment on attachment 8886179 [details]
Bug 1359855 - Fix support of per tool key shortcuts in toolboxes opened in a window.

https://reviewboard.mozilla.org/r/156982/#review162818

::: devtools/client/devtools-startup.js:486
(Diff revision 2)
>      if (cmdLine.state == Ci.nsICommandLine.STATE_REMOTE_AUTO) {
>        cmdLine.preventDefault = true;
>      }
>    },
>  
> +  // Used by tests

Not accurate, used by devtools-toolbox too. 

Would be nice to have a comment somewhere explaining that a toolbox window will not trigger "delayed-browser-startup-finished" and needs to attach shortcuts in a different way.

::: devtools/client/framework/toolbox.js:865
(Diff revision 2)
>      let doc = this.win.parent.document;
>  
> -    for (let [id, toolDefinition] of gDevTools.getToolDefinitionMap()) {
> -      // Prevent multiple entries for the same tool.
> -      if (!toolDefinition.key || doc.getElementById("key_" + id)) {
> +    for (let item of Startup.KeyShortcuts) {
> +      // KeyShortcuts contain tool-specific and global key shortcuts,
> +      // here we only need to copy shortcut specific to each tool.
> +      if (!item.toolId) {

It's a shame we have to duplicate the whole key creation code, but this filter makes it difficult to mutualize without complexifying. Let's leave as is.
Attachment #8886179 - Flags: review?(jdescottes) → review+
Comment on attachment 8886180 [details]
Bug 1359855 - Update key id reference if tests for responsive design.

https://reviewboard.mozilla.org/r/156984/#review162820
Attachment #8886180 - Flags: review?(jdescottes) → review+
Comment on attachment 8886181 [details]
Bug 1359855 - Prevent leaking DeveloperToolbar getter on browser windows during DevTools unload.

https://reviewboard.mozilla.org/r/156986/#review162822

Is this changeset still needed now that you added:

> window.addEventListener("mochitest-load", () => this.initDevTools(), { once: true });

in devtools-startup? Quickly ran a small suite without this changeset and got no failure.
Attachment #8886181 - Flags: review?(jdescottes)
Comment on attachment 8886182 [details]
Bug 1359855 - Remove assertion for dynamic key registration.

https://reviewboard.mozilla.org/r/156988/#review162826

In theory we can support registering new keys for new tools, we just need to expose an API on devtools-startup to handle that right? (and keep definitions/startup in sync). But I'm ok with pushing that to a follow up based on your comment that webextensions can't define keys (let's make sure this is true before landing !)
Attachment #8886182 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #44)
> ::: devtools/client/devtools-startup.js:147
> (Diff revision 5)
> > +      modifiers
> > +    },
> > +  ];
> > +});
> >  
> >  function DevToolsStartup() {}
> 
> Add a small comment here that explains the role of the component is both to
> handle command line arguments as well as to add various hooks to initialize
> devtools.

Looks redundant with the comment at module's header.
Should I move it here?

> ::: devtools/client/devtools-startup.js:241
> (Diff revision 5)
> > +  createKey(doc, { id, toolId, shortcut, modifiers: mod }, oncommand) {
> > +    let k = doc.createElement("key");
> > +    k.id = "key_" + (id || toolId);
> > +
> > +    if (shortcut.startsWith("VK_")) {
> > +      k.setAttribute("keycode", shortcut);
> 
> We are no longer setting the keytext on VK_* keys. From reading MDN I
> suppose this was used to display F8 instead of VK_F8 etc... in the menus.
> But I don't see any VK_ displayed anywhere when testing.
> 
> Do you know if this is now handled somewhere else? automatically stripping
> VK_ from keys?
> 
> If this is the case, we should cleanup all remaining keytext properties as
> they are no longer used.

No idea why we were using keytext. Looks like left-over?
It looks like there is some builtin coded in XUL to display the key shortcuts correctly...
http://searchfox.org/mozilla-central/source/layout/xul/nsMenuFrame.cpp#1082-1119

> ::: devtools/client/locales/en-US/key-shortcuts.properties:1
> (Diff revision 5)
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> 
> This file duplicates shortcuts from both startup.properties &
> menus.properties. 
> 
> The ones in menus.properties are no longer used and should be removed in any
> case.
> 
> I think it would make sense to move towards a single properties file
> containing all the command keys, access keys and labels for our menus. 
> 
> I want to agree on a strategy here before duplicating a dozen of new strings
> to localize:
> - duplicate. In this case: 1 loc note per duped item is mandatory +
> explicitly say it should be synced with the one still found in
> startup.properties
> - move all commandkeys, accesskeys, menulabels to this file, and use
> MultiLocalizationHelper in browser-menus.js and definitions.js to fetch
> strings from the 2 prop files

After some discussion on irc, I went for keeping three properties, by removing any duplication:
* key-shortcuts: contains commandkey, only the key code to use for each tool
* menus: contains label and access key to build the system menu (which is duplicated in hamburger menu "Developer Tools" list and wrench icon context menu).
* startup: contains tool title displayed in toolboxes tabs, as well as the tooltip when hovering the tab and a couple and the ARIA label. It also contains some random strings for the toolbox, but it may be renamed to toolbox-tabs.properties.

It allows lazily loading these strings at every stage:
 1) Only shortcuts on firefox startup,
 2) Only label when opening the menu,
 3) And tools title on toolbox startup.
We may merge menus.properties and startup.properties as, today, they are both loaded on startup.
(In reply to Julian Descottes [:jdescottes] from comment #46) 
> Would be nice to have a comment somewhere explaining that a toolbox window
> will not trigger "delayed-browser-startup-finished" and needs to attach
> shortcuts in a different way.

Added a comment next to the event listener.

> ::: devtools/client/framework/toolbox.js:865
> (Diff revision 2)
> >      let doc = this.win.parent.document;
> >  
> > -    for (let [id, toolDefinition] of gDevTools.getToolDefinitionMap()) {
> > -      // Prevent multiple entries for the same tool.
> > -      if (!toolDefinition.key || doc.getElementById("key_" + id)) {
> > +    for (let item of Startup.KeyShortcuts) {
> > +      // KeyShortcuts contain tool-specific and global key shortcuts,
> > +      // here we only need to copy shortcut specific to each tool.
> > +      if (!item.toolId) {
> 
> It's a shame we have to duplicate the whole key creation code, but this
> filter makes it difficult to mutualize without complexifying. Let's leave as
> is.

We may implement that from devtools-startup.js?
This would surely add some specific code... may be not that much?
Mostly listening to another more global event listener. I imagine the onKey function would work for everything.
(In reply to Julian Descottes [:jdescottes] from comment #45)
> Comment on attachment 8886178 [details]
> Bug 1359855 - Inline jsonview main module to keep it working before user
> action.
> 
> https://reviewboard.mozilla.org/r/156980/#review162090
> 
> Clearing mostly to look at the destroy.
> 
> ::: devtools/client/devtools-startup.js:190
> (Diff revision 2)
> >          devtoolsFlag = false;
> >        }
> >      };
> >      Services.obs.addObserver(onWindowReady, "browser-delayed-startup-finished");
> > +
> > +    JsonView.initialize();
> 
> Is there a good reason to initialize it here rather than in onWindowReady()?
> If yes, add a comment.

I didn't gave much thoughts about that. Session restore of tabs with JSON may be an issue, but it seems to work even when initializing on delayed-startup.

> 
> ::: devtools/client/devtools-startup.js:521
> (Diff revision 2)
> > +    // Register for messages coming from the child process.
> > +    Services.ppmm.addMessageListener(
> > +      "devtools:jsonview:save", this.onSave);
> > +  },
> > +
> > +  destroy: function () {
> 
> We are no longer calling destroy at any point. We should probably observe
> [quit-application] and run some cleanup code?

I'm not sure there is any real value in cleaning up things on shutdown?
Comment on attachment 8885915 [details]
Bug 1359855 - Prevent loading any DevTools module until users interact with any devtool entrypoint.

https://reviewboard.mozilla.org/r/156698/#review163126

Some comments, looks good but I need to test it a bit more tomorrow.

::: devtools/client/definitions.js:35
(Diff revision 6)
>  loader.lazyImporter(this, "ResponsiveUIManager", "resource://devtools/client/responsivedesign/responsivedesign.jsm");
>  loader.lazyImporter(this, "ScratchpadManager", "resource://devtools/client/scratchpad/scratchpad-manager.jsm");
>  
>  const {LocalizationHelper} = require("devtools/shared/l10n");
>  const L10N = new LocalizationHelper("devtools/client/locales/startup.properties");
> +const L10NKeyShortcuts = new LocalizationHelper("devtools/client/locales/key-shortcuts.properties");

Any reason to avoid using the MultiLocalizationHelper ? This way no need to change anything else in the file.

::: devtools/client/devtools-startup.js:5
(Diff revision 6)
>  /**
>   * This XPCOM component is loaded very early.
>   * It handles command line arguments like -jsconsole, but also ensures starting
>   * core modules like 'devtools-browser.js' that hooks the browser windows
>   * and ensure setting up tools.
>   *
>   * Be careful to lazy load dependencies as much as possible.
>   **/

I totally missed that we had this head comment. Let's just update it to describe the component a bit better:
- responsible for handling cmd line args
- creates devtools key shortcuts, and initializes devtools when any of them is triggered
- listens to events indicating that a devtools menu is displayed and initializes devtools then

::: devtools/client/devtools-startup.js:189
(Diff revision 6)
>      };
> -    Services.obs.addObserver(onStartup, "browser-delayed-startup-finished");
> +    Services.obs.addObserver(onWindowReady, "browser-delayed-startup-finished");
>    },
>  
> +  /**
> +   * Register listener to all possible entry points for Developer Tools.

nit: listener -> listeners

::: devtools/client/devtools-startup.js:191
(Diff revision 6)
>    },
>  
> +  /**
> +   * Register listener to all possible entry points for Developer Tools.
> +   * But instead of implementing the actual actions, defer to DevTools codebase.
> +   * In most case, it only need to call this.initDevTools which handle the rest.

nit: case -> cases, need -> needs, handle -> handles

::: devtools/client/locales/en-US/key-shortcuts.properties:7
(Diff revision 6)
> +# 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/.
> +
> +# LOCALIZATION NOTE (toogleToolbox.commandkey):
> +# Key pressed to open a toolbox with the default panel selected
> +# Used to come from devtools/client/toolbox.dtd

If there is no need to sync the localizations, I would get rid of this last sentence in each localization note

::: devtools/client/locales/en-US/startup.properties
(Diff revision 6)
>  performance.panelLabel=Performance Panel
>  
> -# LOCALIZATION NOTE (performance.commandkey, performance.accesskey)
> -# Used for the menuitem in the tool menu
> -performance.commandkey=VK_F5
> -performance.accesskey=P

I didn't mean we should remove totally the support for accesskey in definitions.js, sorry if I was unclear. browser-menus still seem to expect them to be defined.
Comment on attachment 8886178 [details]
Bug 1359855 - Inline jsonview main module to keep it working before user action.

https://reviewboard.mozilla.org/r/156980/#review163148

::: devtools/client/devtools-startup.js:549
(Diff revision 3)
> +    // Register for messages coming from the child process.
> +    Services.ppmm.addMessageListener(
> +      "devtools:jsonview:save", this.onSave);
> +  },
> +
> +  destroy: function () {

I'm fine with not calling destroy, should we keep the method though?
Attachment #8886178 - Flags: review?(jdescottes) → review+
Comment on attachment 8886181 [details]
Bug 1359855 - Prevent leaking DeveloperToolbar getter on browser windows during DevTools unload.

https://reviewboard.mozilla.org/r/156986/#review163150

::: devtools/client/framework/devtools-browser.js:711
(Diff revision 3)
>      let desc = Object.getOwnPropertyDescriptor(win, "DeveloperToolbar");
>      if (desc && !desc.get) {
>        win.DeveloperToolbar.destroy();
>      }
> +    // Remove the getter to prevent leak report during mochitests
> +    delete win.DeveloperToolbar;

Not sure if my comment was lost but I was just saying that tests didn't seem to fail for me without this changeset. Are we sure it is needed?
Attachment #8886181 - Flags: review?(jdescottes)
(In reply to Julian Descottes [:jdescottes] from comment #62)
> Comment on attachment 8886181 [details]
> Bug 1359855 - Prevent exception about DeveloperToolbar being leaked on top
> level window.
> 
> https://reviewboard.mozilla.org/r/156986/#review163150
> 
> ::: devtools/client/framework/devtools-browser.js:711
> (Diff revision 3)
> >      let desc = Object.getOwnPropertyDescriptor(win, "DeveloperToolbar");
> >      if (desc && !desc.get) {
> >        win.DeveloperToolbar.destroy();
> >      }
> > +    // Remove the getter to prevent leak report during mochitests
> > +    delete win.DeveloperToolbar;
> 
> Not sure if my comment was lost but I was just saying that tests didn't seem
> to fail for me without this changeset. Are we sure it is needed?

That isn't mandatory, but I still think it is a valuable cleanup.
I rephrased the commit message and comment to make it more generic.
Comment on attachment 8886178 [details]
Bug 1359855 - Inline jsonview main module to keep it working before user action.

Jan, I'm r? on the jsonview for two reasons:
1) Are you ok with the inline of main.js into devtools-startup.js?
 (That's to avoid having to load the loader and this module during firefox startup)
2) I removed JsonView.destroy as it looks like it is useless to do during firefox shutdown? It was useful and important when this code was part of the devtools add-on as it prevented having more than one instance of it alive.
Comment on attachment 8886181 [details]
Bug 1359855 - Prevent leaking DeveloperToolbar getter on browser windows during DevTools unload.

https://reviewboard.mozilla.org/r/156986/#review163504

As discussed, r+ for the devtools part, please ask a suitable reviewer for testing/mochitest/browser-test.js
Attachment #8886181 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #49)
> Comment on attachment 8886182 [details]
> Bug 1359855 - Remove assertion for dynamic key registration.
> 
> https://reviewboard.mozilla.org/r/156988/#review162826
> 
> In theory we can support registering new keys for new tools, we just need to
> expose an API on devtools-startup to handle that right? (and keep
> definitions/startup in sync). But I'm ok with pushing that to a follow up
> based on your comment that webextensions can't define keys (let's make sure
> this is true before landing !)

It looks like it doesn't pass any key attribute:
  http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-devtools-panels.js#77-95
Nor does chrome seem to support any key shortcut API directely related to devtools API:
  https://developer.chrome.com/extensions/devtools_panels
I imagine if an add-on want to set a key shortcut, it will use the commands API:
  https://developer.chrome.com/extensions/commands
Which is implemented independantly of devtools APIs.

Luca, could you just confirm you won't depend on devtools codebase to register key shortcuts for addon's panels?
Flags: needinfo?(lgreco)
Blocks: 1381862
Blocks: 1381868
Here is the possible followups:
* bug 1380806, move command line implementation to its own module
* bug 1381862, try to simplify the menus / .properties story
* bug 1381868, try to share more code regarding key shortcut of toolboxes loaded in a window.
I confirm that currently we have no plans to support a "key shortcut" parameter on the "devtools.panels.create" API,
for any other similar kind of shortcut action (e.g. opening an extension browserAction/pageAction popup or a sidebar), we are extending the "actions" supported by the commands API, and so it would be probably better to expose it using a consistent approach.

Also, the panels added by a webextension are "toolbox" specific and by allowing them to specify the "key shortcut" in the "devtools.panels.create" API could potentially allow an extension to use a different shortcut for the same panel registered on different toolboxes, which would be very confusing for the user.
Flags: needinfo?(lgreco)
(In reply to Julian Descottes [:jdescottes] from comment #72)
> Comment on attachment 8886181 [details]
> Bug 1359855 - Prevent leaking DeveloperToolbar getter on browser windows
> during DevTools unload.
> 
> https://reviewboard.mozilla.org/r/156986/#review163504
> 
> As discussed, r+ for the devtools part, please ask a suitable reviewer for
> testing/mochitest/browser-test.js

Oh, you were right about the browser-test.js modification, it isn't useful anymore:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b87dde6e1e3a851bd645dd225a94e0db471b9633
Tests are green without it. I'll remove browser-test modification from this patch queue.

I imagine that's:
  window.addEventListener("mochitest-load", () => this.initDevTools(), { once: true });
which fixes this, by injecting DeveloperToolbar before the mochitest codebase kicks in.
(In reply to Alexandre Poirot [:ochameau] from comment #84)
> (In reply to Julian Descottes [:jdescottes] from comment #72)
> > Comment on attachment 8886181 [details]
> > Bug 1359855 - Prevent leaking DeveloperToolbar getter on browser windows
> > during DevTools unload.
> > 
> > https://reviewboard.mozilla.org/r/156986/#review163504
> > 
> > As discussed, r+ for the devtools part, please ask a suitable reviewer for
> > testing/mochitest/browser-test.js
> 
> Oh, you were right about the browser-test.js modification, it isn't useful
> anymore:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b87dde6e1e3a851bd645dd225a94e0db471b9633
> Tests are green without it. I'll remove browser-test modification from this
> patch queue.
> 
> I imagine that's:
>   window.addEventListener("mochitest-load", () => this.initDevTools(), {
> once: true });
> which fixes this, by injecting DeveloperToolbar before the mochitest
> codebase kicks in.

I'd prefer to not have a different startup sequence for tests vs our live code. After reading the comments here but I'm still not sure why we need this. Why is this needed?
(In reply to Brian Grinstead [:bgrins] from comment #85)
> I'd prefer to not have a different startup sequence for tests vs our live
> code. After reading the comments here but I'm still not sure why we need
> this. Why is this needed?

Spoiler: this is unfortunately complex story.

With this patch, you need one of these actions (command line argument, fire a key shortcut, click on a menuitem) to ensure loading main devtools modules (i.e. devtools/client/framework/devtools and devtools/client/framework/devtools-browser = gDevTools and gDevToolsBrowser).
But most tests don't do any of these actions before running their assertions,
and they really expect gDevTools and gDevToolsBrowser to be up and running!

One thing I tried was registering a shared-head, from absolutely all browser-mochitest,
but got into trouble because of this DeveloperToolbar being set on browser windows *after* mochitests starts running.
Any attribute added to browser.xul window object that isn't removed before the test finishes is reported as a leak.
Then, I tried to call gDevTools.destroy() at the end of each test but that is way too much work.
Too many tests crashes as the destruction isn't complete and finishes asynchronously.

So I added this mochitest-load listener which allows to load gDevTools/gDevToolsBrowser *before* mochitests start running.
That allows to workaround the leak report of DeveloperToolbar attribute (as we add this property before mochitests checks) *and* prevent having to introduce yet another shared-head, which, compared to the existing shared-head, would be really imported from every single browser-mochitest of /devtools/.

I think we can followup by removing this DeveloperToolbar hooked on window object. We may have kept that for compatiblity with add-ons. There is a bunch of legacy code like that we will be able to remove after the merge day.
But I'm quite skeptical about the introduction of yet another shared-head?
Comment on attachment 8885915 [details]
Bug 1359855 - Prevent loading any DevTools module until users interact with any devtool entrypoint.

https://reviewboard.mozilla.org/r/156698/#review163756

Thanks for addressing the comments! r+ on my side. 
Regarding the initDevTools for mochitests, we discussed it before and I think that it currently makes for a nice workaround, but it's worth discussing.

Last week we discussed about potential issues with the DevToolsShim. The Shim will now consider that DevTools are not installed until devtools-browser is loaded.
One of the issues is the "inspect element" context menuitem, but other things will start breaking (addon stuff, restoring scratchpad sessions etc...).
Our tests are hiding the issues since we init devtools on mochitest load.

My proposal would be to support two states in the DevToolsShim: installed and initialized. 
Initialized would be set to true when gDevTools registers itself on the shim. Most methods in the shim should call Startup.initDevTools() if initialized is false.
Regarding installed, it's a bit more complicated. Ideally we would rely on the AddonManager, but we can't do that until DevTools are a system addon. We could simply hardcode it to be always true until we land the devtools as system addon patches.

Another thing we need to review are the events that are observed from within devtools and coming from the m-c. They are also valid entry points for initializing devtools which are not covered yet in this patch.

::: devtools/client/devtools-startup.js:246
(Diff revision 7)
> +    if (key.toolId) {
> +      gDevToolsBrowser.selectToolCommand(window.gBrowser, key.toolId);
> +    } else {
> +      gDevToolsBrowser.onKeyShortcut(window, key.id);
> +    }

It would be nice to limit the number of APIs we use from devtools in this startup component (trying to keep in mind the addon plans). The if() can be moved inside of gDevToolsBrowser.onKeyShortcut(), this way we only need to call onKeyShortcut from here.
Attachment #8885915 - Flags: review?(jdescottes) → review+
Quickly reviewed the events we are listening to in devtools (http://searchfox.org/mozilla-central/search?q=Services.obs.addO&case=false&regexp=false&path=devtools) and I think the only issue is actually with an event I plan to add in Bug 1374735, to open the browser console from extensions code (review: https://reviewboard.mozilla.org/r/151844/diff/3#index_header)

The observer is supposed to be in devtools-browser, which will now be loaded only if devtools are already initialized. I can either keep the event, and support it in devtools-startup, or change my patch in order to use the DevToolsShim to open the browser console (and then reuse the logic explained above)
Depends on: 1382173
Comment on attachment 8886178 [details]
Bug 1359855 - Inline jsonview main module to keep it working before user action.

https://reviewboard.mozilla.org/r/156980/#review164108

(In reply to Alexandre Poirot [:ochameau] from comment #71)
> Comment on attachment 8886178 [details]
> Bug 1359855 - Inline jsonview main module to keep it working before user
> action.
> 
> Jan, I'm r? on the jsonview for two reasons:
> 1) Are you ok with the inline of main.js into devtools-startup.js?
>  (That's to avoid having to load the loader and this module during firefox
> startup)

I'd be happier if the main module is part of the jsonview directory, but
if using a loader is a perf problem than it's an argument I respect.
So, yes, ok for me.

> 2) I removed JsonView.destroy as it looks like it is useless to do during
> firefox shutdown? It was useful and important when this code was part of the
> devtools add-on as it prevented having more than one instance of it alive.

I see, ok, please make a comment in JsonView.initialize() method explaining why
`Services.ppmm.addMessageListener("devtools:jsonview:save", this.onSave);`
...is never removed.

Thanks!
Honza
Attachment #8886178 - Flags: review?(odvarko) → review+
Comment on attachment 8885915 [details]
Bug 1359855 - Prevent loading any DevTools module until users interact with any devtool entrypoint.

Flod, would you mind looking at this patch and confirm everything looks good regarding localization?
We are moving a few strings from one .properties to another with some key name changes, hopefully that would not introduce too much burden on l10n side. If there is anything we can do upfront to help...
Attachment #8885915 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8885915 [details]
Bug 1359855 - Prevent loading any DevTools module until users interact with any devtool entrypoint.

Can't spot anything wrong (thanks for flagging).
Attachment #8885915 - Flags: feedback?(francesco.lodolo) → feedback+
Quick check on the profiler.
devtools-startup.js is still visible on Firefox startup, but only see 17ms in total.
With 12ms spent on creating the xul:key.
https://perfht.ml/2tfim3H

This is within a VM, I see a total cost of 120ms without these patches.
(In reply to Alexandre Poirot [:ochameau] from comment #107)
> Quick check on the profiler.
> devtools-startup.js is still visible on Firefox startup, but only see 17ms
> in total.
> With 12ms spent on creating the xul:key.
> https://perfht.ml/2tfim3H
> 
> This is within a VM, I see a total cost of 120ms without these patches.

Nice improvement :-).

You may want to also look at this in profiles of creating a second window, in that case it's likely to take a larger part of _delayedStartup, as all the code that only runs once won't be in the profile.

Is hookKeyShortcuts something we could delay with an idle callback? Maybe with a relatively short timeout of 1s to ensure it does happen early enough to not miss key presses.
Comment on attachment 8887926 [details]
Bug 1359855 - Prevent loading DevTools when saving session restore data.

https://reviewboard.mozilla.org/r/158830/#review164328
Attachment #8887926 - Flags: review?(dtownsend) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #107)
> Quick check on the profiler.
> devtools-startup.js is still visible on Firefox startup, but only see 17ms
> in total.
> With 12ms spent on creating the xul:key.
> https://perfht.ml/2tfim3H

Interesting. And it looks like most of that time is actually spent in the baseline compiler. The code itself executes quite fast once that's finished.

So, well done.
(In reply to Florian Quèze [:florian] [:flo] from comment #108)
> You may want to also look at this in profiles of creating a second window,
> in that case it's likely to take a larger part of _delayedStartup, as all
> the code that only runs once won't be in the profile.

I see only 3ms for devtools-startup coded fired from delayedStartup on new windows.
Whereas delayedStartup takes between 53 or 100ms (I don't know how to properly read the profile)
https://perfht.ml/2uebtCx

> Is hookKeyShortcuts something we could delay with an idle callback? Maybe
> with a relatively short timeout of 1s to ensure it does happen early enough
> to not miss key presses.

Possibly. Do not hesitate to open followup after this patch is landed if you think there is some valuable win here.
Just dropped the change to browser-test.js, we do not have to whitelist DeveloperToolbar as we inject it early enough thanks to the mochitest-load event listener.

But I had to patch bug 1382173 to prevent loading devtools again when session restore try to save its data.

I got a pretty green try on all platforms and test on the previous patch queue:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20637a86d9035d4f82863afb0499bb49f28e6f01

But here is a new try just just with xpcshell and mochitest and the latest patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=13425474d8a07f9999cc43959608fe6857b2f786
Comment on attachment 8887926 [details]
Bug 1359855 - Prevent loading DevTools when saving session restore data.

https://reviewboard.mozilla.org/r/158830/#review164590

::: devtools/shim/DevToolsShim.jsm:183
(Diff revision 2)
>      if (!this.isInstalled()) {
>        return [];
>      }
>  
>      if (!this.isInitialized()) {
> -      this._initDevTools();
> +      return [];

Sorry about that! If devtools are not initialized, we can assume there are no sessions. Feel free to get rid of the first if{} block in the method, as it is redundant here.
Attachment #8887926 - Flags: review?(jdescottes) → review+
Depends on: 1382661
It looks like we don't need the special case for mochitest in devtools-startup if we get rid of DeveloperToolbar property on top level windows, so I'm going to fix that first in bug 1382661 before proceeding.
Attachment #8886181 - Attachment is obsolete: true
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12b271e113a0
So with bug 1382661 and DeveloperToolbar removal, I was able to remove the special codepath for tests in devtools-startup.js (mochitest-load listener).

Let's proceed and jump to the followups now!
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a46ee323e1e7
Prevent loading any DevTools module until users interact with any devtool entrypoint. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/44726932185a
Fix the developer toggle in customize widgets. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/354080a171dc
Inline jsonview main module to keep it working before user action. r=Honza,jdescottes
https://hg.mozilla.org/integration/autoland/rev/8d68726f1c34
Fix support of per tool key shortcuts in toolboxes opened in a window. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/061f66345448
Update key id reference if tests for responsive design. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/dbd03e7e1bc5
Remove assertion for dynamic key registration. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/2dd30b065030
Prevent loading DevTools when saving session restore data. r=jdescottes,mossop
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=116867370&repo=autoland
Flags: needinfo?(poirot.alex)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/263f0b999ae1
Backed out changeset 2dd30b065030 
https://hg.mozilla.org/integration/autoland/rev/5c5cd7954f5c
Backed out changeset dbd03e7e1bc5 
https://hg.mozilla.org/integration/autoland/rev/e2e44c6b5180
Backed out changeset 061f66345448 
https://hg.mozilla.org/integration/autoland/rev/d25cdb7f60f4
Backed out changeset 8d68726f1c34 
https://hg.mozilla.org/integration/autoland/rev/d80f56061436
Backed out changeset 354080a171dc 
https://hg.mozilla.org/integration/autoland/rev/4c2da74e95d3
Backed out changeset 44726932185a 
https://hg.mozilla.org/integration/autoland/rev/a6b198e415f3
Backed out changeset a46ee323e1e7 for test failures in browser_ext_devtools_inspectedWindow.js
Comment on attachment 8889409 [details]
Bug 1359855 - Automatically initialize DevTools when accessing DevToolsShim.gDevTools.

https://reviewboard.mozilla.org/r/160442/#review165698

LGTM!
Attachment #8889409 - Flags: review?(jdescottes) → review+
Blocks: 1378863
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba72020236fa
Prevent loading any DevTools module until users interact with any devtool entrypoint. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ef8587aca058
Fix the developer toggle in customize widgets. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/469ff13c773d
Inline jsonview main module to keep it working before user action. r=Honza,jdescottes
https://hg.mozilla.org/integration/autoland/rev/eb206956401c
Fix support of per tool key shortcuts in toolboxes opened in a window. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/c379b1dfb233
Update key id reference if tests for responsive design. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/4ec28f94e284
Remove assertion for dynamic key registration. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ff507dd06af9
Prevent loading DevTools when saving session restore data. r=jdescottes,mossop
https://hg.mozilla.org/integration/autoland/rev/1b4dea0fe021
Automatically initialize DevTools when accessing DevToolsShim.gDevTools. r=jdescottes
Flags: needinfo?(poirot.alex)
Iteration: --- → 56.4 - Aug 1
This also landed some performance improvements:

== Change summary for alert #8222 (as of July 24 2017 17:34 UTC) ==

Improvements:

  9%  sessionrestore windows10-64 opt e10s     764.71 -> 697.08
  8%  sessionrestore_no_auto_restore windows10-64 opt e10s855.71 -> 786.75
  5%  sessionrestore windows7-32 opt e10s      803.77 -> 764.50
  5%  sessionrestore_no_auto_restore windows7-32 opt e10s885.88 -> 844.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8222
and a memory imrpovement on osx:
== Change summary for alert #8220 (as of July 24 2017 19:07 UTC) ==

Improvements:

  3%  JS summary osx-10-10 opt      138,862,703.33 -> 135,039,975.83

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8220
Depends on: 1385226
Depends on: 1386724
Depends on: 1386821
Depends on: 1387023
Depends on: 1387359
Depends on: 1389939
Blocks: damp
Product: Firefox → DevTools
Performance Impact: --- → P2
Whiteboard: [reserve-photon-performance][qf:p2] → [reserve-photon-performance]
You need to log in before you can comment on or make changes to this bug.