browser/base/content/test/performance/browser_startup.js indicates that the following scripts are loaded before opening the first window: chrome://devtools-shim/content/DevToolsShim.jsm

Pinging Julian on this (could you please assign a priority to this bug too?)

Not sure if it makes a big difference, but when I test locally the file is actually logged as follows: > INFO modules loaded before handling user events: chrome://devtools-shim/content/DevToolsShim.jsm ("before handling user events" vs "before opening the first window") The DevToolsShim is a class that exposes some DevTools APIs so mozilla central. If DevTools are installed, it forwards calls to DevTools, otherwise it either holds the call or throws, depending on the API used. To do that, the DevToolsShim needs to know if DevTools are available, without depending on a devtools path (we don't want any m-c code to check for chrome://devtools... or resource://devtools... as the paths might change in the future). Right now the approach is to let DevTools (ie gDevTools from devtools/client/framework/devtools.js) register itself on the DevToolsShim. In the current setup, where DevTools are initialized by default, this triggers the load of the DevToolsShim: - on "browser-delayed-startup-finished" - devtools-startup loads devtools-browser - devtools-browser loads devtools - devtools loads DevToolsShim to register itself Note that this code path won't be executed when DevTools are extracted to an addon, if the addon is not installed. Alternatively, instead of calling directly DevToolsShim from devtools.js, I suppose we could use Services.obs events to communicate between them. That means at least 3 events, 2 fired from devtools.js to register and unregister itself, and one from DevToolsShim in order to check if devtools are already loaded. Ehsan: What is the priority of this issue? Do we want the added complexity, knowing that it should not affect release users? (provided we successfully move devtools to an addon of course)

(In reply to Julian Descottes [:jdescottes] from comment #2) > In the current setup, where DevTools are initialized by default, this > triggers the load of the DevToolsShim: > - on "browser-delayed-startup-finished" > - devtools-startup loads devtools-browser > - devtools-browser loads devtools > - devtools loads DevToolsShim to register itself Do you need this to be triggered once for each browser window (that's what "browser-delayed-startup-finished" does) or once after startup (if so you want the "sessionstore-windows-restored" notification, which fires once after we are done restoring all windows). It would also be nice to use an idle callback so that we can start processing user events before your initialization code runs. See also bug 1353548 and bug 1359855.

"Do you need this to be triggered once for each browser window or once" ? Looking at the code: > let onStartup = window => { > Services.obs.removeObserver(onStartup, > "browser-delayed-startup-finished"); > // Ensure loading core module once firefox is ready > this.initDevTools(); > > if (devtoolsFlag) { > this.handleDevToolsFlag(window); > } > }; > Services.obs.addObserver(onStartup, "browser-delayed-startup-finished"); (http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/devtools/client/devtools-startup.js#51-61) This is in the handle() method devtools-startup which is command-line-handler component. I believe this means we only need to do it once (and we effectively only do it once since the observer is removed right after). I can try to rely on the "sessionstore-windows-restored", but I'm not familiar enough with the DevTools startup sequence to know if this can trigger regressions or not. Same for using an idle callback, I'll try but not sure about the impacts. bgrins: any insights about this? do you think changing this (event+idle callback) could be problematic?

(In reply to Julian Descottes [:jdescottes] from comment #4) > > This is in the handle() method devtools-startup which is > command-line-handler component. I believe this means we only need to do it > once (and we effectively only do it once since the observer is removed right > after). Actually we need a window in case the "--devtools" flag was passed to initialize a toolbox right away. I guess we could still swap the event to "sessionstore-windows-restored" and find the window for the current tab. But unless there is a real incentive to swap events here I think we should keep using "browser-delayed-startup-finished". Try push switching to requestIdleCallback: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f775acc153820be86077f039cbcb0dce3ec8fa0

(for the record, try push also switching to sessionstore-windows-restored: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d997e118c4107de671b7308ab6f7f186d6d7e4)

(In reply to Julian Descottes [:jdescottes] from comment #2) > Ehsan: What is the priority of this issue? Do we want the added complexity, > knowing that it should not affect release users? (provided we successfully > move devtools to an addon of course) If this will in fact not impact most of our users on the release channel, then it doesn't concern me all that much at this point. :-)

(In reply to Julian Descottes [:jdescottes] from comment #4) > "Do you need this to be triggered once for each browser window or once" ? > > Looking at the code: > > > let onStartup = window => { > > Services.obs.removeObserver(onStartup, > > "browser-delayed-startup-finished"); > > // Ensure loading core module once firefox is ready > > this.initDevTools(); > > > > if (devtoolsFlag) { > > this.handleDevToolsFlag(window); > > } > > }; > > Services.obs.addObserver(onStartup, "browser-delayed-startup-finished"); > > (http://searchfox.org/mozilla-central/rev/ > c49a70b53f67dd5550eec8a08793805f2aca8d42/devtools/client/devtools-startup. > js#51-61) > > This is in the handle() method devtools-startup which is > command-line-handler component. I believe this means we only need to do it > once (and we effectively only do it once since the observer is removed right > after). > > I can try to rely on the "sessionstore-windows-restored", but I'm not > familiar enough with the DevTools startup sequence to know if this can > trigger regressions or not. Same for using an idle callback, I'll try but > not sure about the impacts. > > bgrins: any insights about this? do you think changing this (event+idle > callback) could be problematic? The only concern with delaying it (either by using a different event or a idle callback) is that initDevTools is the first thing that loads gDevToolsBrowser (client/framework/devtools-browser.js). I don't see anything that should cause breakage there if it's delayed, but I do see a couple potential issues: * It adds the menu items, which would lead to menus / keyboard shortcuts not working until it loads * It adds the wrench toolbar button, which could cause some movement on the top toolbar as it gets added in after the fact I don't think either of those would be so bad if we just did an idle callback. I'm not sure how long "sessionstore-windows-restored" takes and as you say that would complicate the code a little bit when the devtools command line flag is passed in, so I'd generally be in favor of the idle callback if all is green.

DevTools bug triage (filter on CLIMBING SHOES).

This should be closed as duplicate of Bug 1359855 Now that devtools are not loaded on startup, they also don't load the devtools shim.

Sure, thanks!