Alexandre Poirot [:ochameau] Assignee
	Description • 7 years ago

Preliminary work before bug 1361080. We would first start with some code that listen for key shortcuts, adds a menu entry in Tools menu and manage the add-on installation.
Then once any of these hook is fired, bug 1361080 would introduce a nice UI to prompt for add-on installation if the user really expected to use the DevTools.

	Alexandre Poirot [:ochameau] Assignee
	Comment 2 • 7 years ago

So, this patch comes from bug 1361080. It really focuses on the hooks (key shortcuts, menu, command line) and add-on install.
The duplicated devtools-startup.js isn't completely satisfying due to code duplication.
Ideally we would merge it with existing one, but I would like to be careful about not making existing codepath even slower with more things to load...

you can test this patch by patching this file:
diff --git a/browser/confvars.sh b/browser/confvars.sh
index 6fcb839..11e40a9 100755
--- a/browser/confvars.sh
+++ b/browser/confvars.sh
@@ -65,4 +65,4 @@ MOZ_PROFILE_MIGRATOR=1
 MOZ_ADDON_SIGNING=1
 
 # Include the DevTools client, not just the server (which is the default)
-MOZ_DEVTOOLS=all
+MOZ_DEVTOOLS=addon

	Patrick Brosset <:pbro>
	Comment 3 • 7 years ago

DevTools bug triage (filter on CLIMBING SHOES).

	Julian Descottes [:jdescottes]
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8883877 [details]
Bug 1378397 - Introduce shim for managing upcoming DevTools addon.

https://reviewboard.mozilla.org/r/154862/#review160284

Here is a first pass on the code, I can't finish the review right now and I apologize in advance if some of the comments don't make sense :) Feel free to ping me!

::: devtools/client/framework/command-line.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I want to make sure: the reason for not loading this file in the addon's devtools-startup component is to avoid slowing down the startup of devtools?

In this case maybe we could do it the other way around and have devtools-startup.js expose the CommandLine handling logic? Then maybe wrap it in this command-line.js file if needed, or directly have firefox's devtools-startup import the addon's devtools-startup?

I didn't compare the files carefully so I don't know if there are discrepencies that would prevent doing that.

::: devtools/shim/devtools-startup.js:66
(Diff revision 1)
> +    }
> +    return false;
> +  },
> +
> +  // Returns true if the DevTools add-on is installed *and* enabled
> +  get isInstalled() {

I find isInstalled misleading here. 
"installed" doesn't imply "enabled". isEnabled would be less confusing, or isInstalledAndEnabled

::: devtools/shim/devtools-startup.js:101
(Diff revision 1)
> +    // If the add-on is already installed, its own command line component is
> +    // going to take over. Register into Firefox UI and implement the command
> +    // line arguments
> +    if (isInstalled) {
> +      console.log("Add-on already installed");
> +      this.watchAddonUninstall();

all the code paths in this method seem to call this.watchAddonUninstall. 

I would prefer having it in a single place in the method. When I first read this my initial reaction was "why are we only watching uninstall is the addon is already installed ?"

::: devtools/shim/devtools-startup.js:109
(Diff revision 1)
> +
> +    console.log("Auto install enabled: " + this.autoInstallEnabled);
> +    console.log("Has any devtools command line argument: ",
> +      this.hasAnyDevToolsFlag(cmdLine));
> +    console.log("Has devtools ever been opened: " + this.hasDevToolsEverBeenOpened);
> +    if (this.autoInstallEnabled && (this.hasAnyDevToolsFlag(cmdLine) ||

I think autoInstallEnabled should only be checked for "hasDevToolsEvenBeenOpened". 

If a devtools flag was set, then it's the same as if the user pressed a shortcut: there is a clear intent of using devtools, so we should install it, regardless of a previous uninstall.

So (hasFlag || (autoInstall && beenOpened)) ?

::: devtools/shim/devtools-startup.js:119
(Diff revision 1)
> +      if (this.hasAnyDevToolsFlag(cmdLine)) {
> +        // Then once the add-on is installed hand over the nsICommandLine object
> +        // to the addon to trigger the given command line arguments
> +        let { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +        let CommandLine = require("devtools/client/framework/command-line");
> +        CommandLine.handleCommandLine(cmdLine);

I could not test this completely because the addon I use doesn't have command-line yet, so might be wrong.

When we are first installing the addon and then forwarding command line arguments to the command-line module, I think we will have issues with:
- --devtools because it relies on the browser-delayed-startup-finished event
- --wait-for-jsdebugger (not sure to understand the implementation completely, I just think it needs to happen very early in the startup process)

For --devtools we can if (Services.startup.startingUp) to fix this. For the other one maybe we can log something?

::: devtools/shim/devtools-startup.js:136
(Diff revision 1)
> +        if (addon.id != AddonID) {
> +          return;
> +        }
> +        this.isInstalled.then(installed => {
> +          if (installed) {
> +            this.cleanup();

Trying to decide if we should set the autoinstall pref back to true here. Logically it would make sense, but at the same time, I can't see this pref being useful once the user has installed the addon. What do you think, could there be a tricky edge case where the browser restarts with the addon uninstalled even though it was installed before?

::: devtools/shim/devtools-startup.js:158
(Diff revision 1)
> +    listener.onDisabling =
> +    listener.onUninstalling = addon => {
> +      if (addon.id == AddonID) {
> +        console.log("Add-on disabled or uninstalled, disabling auto-install");
> +        Services.prefs.setBoolPref("devtools.addon.auto-install", false);
> +        AddonManager.removeAddonListener(listener);

It could be a good idea to reattach the keys and menu entries? 

Right now the disabling/uninstall of the addon really needs to be improved (disabling doesn't disconnect key handlers, uninstalling doesn't close existing toolboxes etc...). But if we manage to cleanly unload devtools, I think we should restore the browser back to its devtools-less state.

::: devtools/shim/devtools-startup.js:193
(Diff revision 1)
> +          this.hookKeys(subject);
> +          this.hookToolsMenu(subject);
> +          this.hookHamburgerMenu();

Given https://bugzilla.mozilla.org/show_bug.cgi?id=1371337#c3 I think it could be nice if we wrapped this new startup code in an idle callback to accommodate the performance comments we received on our current startup code.

::: devtools/shim/devtools-startup.js:197
(Diff revision 1)
> +        case "browser-delayed-startup-finished":
> +          this.hookKeys(subject);
> +          this.hookToolsMenu(subject);
> +          this.hookHamburgerMenu();
> +          break;
> +        case "browser-inspect-node":

For info, in Bug 1372520, we ended up adding an inspectNode method on the DevToolsShim. It was preferred to using an event during the review and I didn't push back.

We can still fire this event from the DevToolsShim. Another thing we need to update is that the menu entry is hidden now if devtools are not installed. Once the UI shim is ready we will need to reenable the menu item even if devtools are not installed.

::: devtools/shim/devtools-startup.js:254
(Diff revision 1)
> +        },
> +        onInstallFailed(install) {
> +          installFailureHandler(install, "Install failed");
> +        },
> +      };
> +      AddonManager.getInstallForURL(this.addonURL, install => {

Is it ok to install the addon in case the addon was simply disabled ? Reading AddonManager.jsm I couldn't find any API to simply enable addons though.

When following the STRs:
- install addon via install dialog
- disabled addon
- restart FF
- install addon via install dialog

=> I can see the logs for the devtools addon bootstrap twice. But it doesn't seem buggy and after a restart the duplicated logs are gone.

::: devtools/shim/devtools-startup.js:261
(Diff revision 1)
> +        install.install();
> +      }, "application/x-xpinstall");
> +    });
> +  },
> +
> +  get keyShortcuts() {

Getting an exception in the console on browser startup: 

console.error: Devtools-Addon-Install: 
  exception
  Message: TypeError: setting getter-only property "keyShortcuts"
  
Had to define an empty setter to make it go away.

::: devtools/shim/devtools-startup.js:291
(Diff revision 1)
> +
> +      // The 3 following keys are also registered in /client/definitions.js
> +      // and should be synced.
> +
> +      // Key for opening the Inspector
> +      // 'C' on all but kk: K, el: 0, sk: r, eo: I, cy: A

I don't think we need those localization comments anymore now that we have a dedicated properties file.

::: browser/locales/en-US/chrome/browser/devtools/shim.properties:10
(Diff revision 1)
> -JAR_MANIFESTS += ['jar.mn']
> +# LOCALIZATION NOTE (toogleToolbox):
> +# Key pressed to open a toolbox with the default panel selected
> +# Used to come from devtools/client/toolbox.dtd
> +toggleToolbox=I
>  
> -# Register the about:debugging page only for 'addon' and 'all' builds.
> +# LOCALIZATION NOTE (inspector.commandkey=C):

Remove the value from the comment

::: browser/locales/en-US/chrome/browser/devtools/shim.properties:15
(Diff revision 1)
> -    EXTRA_COMPONENTS += [
> -        'aboutdebugging-registration.js',
> +# Used to come from devtools/client/startup.properties
> +inspector.commandkey=C
> -        'aboutdebugging.manifest',
> -    ]
>  
> -XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini']
> +# LOCALIZATION NOTE (cmd.commandkey=C):

Remove the value from the comment

"# LOCALIZATION NOTE (cmd.commandkey):"

::: browser/locales/en-US/chrome/browser/devtools/shim.properties:18
(Diff revision 1)
> -    ]
>  
> -XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini']
> +# LOCALIZATION NOTE (cmd.commandkey=C):
> +# Key pressed to open a toolbox with the web console panel selected
> +# Used to come from devtools/client/startup.properties
> +cmd.commandkey=C

Should be K

::: browser/locales/en-US/chrome/browser/devtools/shim.properties:20
(Diff revision 1)
> -XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini']
> +# LOCALIZATION NOTE (cmd.commandkey=C):
> +# Key pressed to open a toolbox with the web console panel selected
> +# Used to come from devtools/client/startup.properties
> +cmd.commandkey=C
> +
> +# LOCALIZATION NOTE (debugerMenu.commandkey=C):

debugerMenu -> debuggerMenu 
Remove the value from the comment

::: browser/locales/en-US/chrome/browser/devtools/shim.properties:23
(Diff revision 1)
> +cmd.commandkey=C
> +
> +# LOCALIZATION NOTE (debugerMenu.commandkey=C):
> +# Key pressed to open a toolbox with the debugger panel selected
> +# Used to come from devtools/client/startup.properties
> +debuggerMenu.commandkey=C

Should be S

	Alexandre Poirot [:ochameau] Assignee
	Comment 5 • 7 years ago

(In reply to Julian Descottes [:jdescottes] from comment #4)
> Comment on attachment 8883877 [details]
> ::: devtools/client/framework/command-line.js:1
> (Diff revision 1)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> I want to make sure: the reason for not loading this file in the addon's
> devtools-startup component is to avoid slowing down the startup of devtools?

That's the main goal. This logic is only needed if a command line is passed and not before.
But also, it allows to more easily change the implementation of each command over time.

> In this case maybe we could do it the other way around and have
> devtools-startup.js expose the CommandLine handling logic? Then maybe wrap
> it in this command-line.js file if needed, or directly have firefox's
> devtools-startup import the addon's devtools-startup?

I don't quite follow this suggestion.

> ::: devtools/shim/devtools-startup.js:66
> (Diff revision 1)
> > +    }
> > +    return false;
> > +  },
> > +
> > +  // Returns true if the DevTools add-on is installed *and* enabled
> > +  get isInstalled() {
> 
> I find isInstalled misleading here. 
> "installed" doesn't imply "enabled". isEnabled would be less confusing, or
> isInstalledAndEnabled

Renamed to isEnabled.

> ::: devtools/shim/devtools-startup.js:101
> (Diff revision 1)
> > +    // If the add-on is already installed, its own command line component is
> > +    // going to take over. Register into Firefox UI and implement the command
> > +    // line arguments
> > +    if (isInstalled) {
> > +      console.log("Add-on already installed");
> > +      this.watchAddonUninstall();
> 
> all the code paths in this method seem to call this.watchAddonUninstall. 
> 
> I would prefer having it in a single place in the method. When I first read
> this my initial reaction was "why are we only watching uninstall is the
> addon is already installed ?"

Moved to the constructor.

> ::: devtools/shim/devtools-startup.js:109
> (Diff revision 1)
> > +
> > +    console.log("Auto install enabled: " + this.autoInstallEnabled);
> > +    console.log("Has any devtools command line argument: ",
> > +      this.hasAnyDevToolsFlag(cmdLine));
> > +    console.log("Has devtools ever been opened: " + this.hasDevToolsEverBeenOpened);
> > +    if (this.autoInstallEnabled && (this.hasAnyDevToolsFlag(cmdLine) ||
> 
> I think autoInstallEnabled should only be checked for
> "hasDevToolsEvenBeenOpened". 
> 
> If a devtools flag was set, then it's the same as if the user pressed a
> shortcut: there is a clear intent of using devtools, so we should install
> it, regardless of a previous uninstall.

The difference is that when pressing a shortcut, you have a UI shim prompt.
It won't install it automagically, you will still have to accept by clicking on the install button.

> ::: devtools/shim/devtools-startup.js:119
> (Diff revision 1)
> > +      if (this.hasAnyDevToolsFlag(cmdLine)) {
> > +        // Then once the add-on is installed hand over the nsICommandLine object
> > +        // to the addon to trigger the given command line arguments
> > +        let { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> > +        let CommandLine = require("devtools/client/framework/command-line");
> > +        CommandLine.handleCommandLine(cmdLine);
> 
> I could not test this completely because the addon I use doesn't have
> command-line yet, so might be wrong.
> 
> When we are first installing the addon and then forwarding command line
> arguments to the command-line module, I think we will have issues with:
> - --devtools because it relies on the browser-delayed-startup-finished event
> - --wait-for-jsdebugger (not sure to understand the implementation
> completely, I just think it needs to happen very early in the startup
> process)
> 
> For --devtools we can if (Services.startup.startingUp) to fix this. For the
> other one maybe we can log something?

When the add-on is already installed it will work as before, the timings shouldn't change.
But you were right, on first run, --devtools wasn't working.
It is harder to make --wait-for-jsdebugger work on first run, I would require 
specifics into devtools-startup. We should followup on this if we want it to work before during add-on installation.

> ::: devtools/shim/devtools-startup.js:136
> (Diff revision 1)
> > +        if (addon.id != AddonID) {
> > +          return;
> > +        }
> > +        this.isInstalled.then(installed => {
> > +          if (installed) {
> > +            this.cleanup();
> 
> Trying to decide if we should set the autoinstall pref back to true here.
> Logically it would make sense, but at the same time, I can't see this pref
> being useful once the user has installed the addon. What do you think, could
> there be a tricky edge case where the browser restarts with the addon
> uninstalled even though it was installed before?

Uninstalled, no. Disabled, yes, for compatiblity reasons or something.
But I don't see the value of ensuring it is true here.

About this pref, I'm wondering if we should simplify it and set it to false
right after the first auto-install. We wouldn't have to wait for the first
uninstall to set it to false...

> ::: devtools/shim/devtools-startup.js:158
> (Diff revision 1)
> > +    listener.onDisabling =
> > +    listener.onUninstalling = addon => {
> > +      if (addon.id == AddonID) {
> > +        console.log("Add-on disabled or uninstalled, disabling auto-install");
> > +        Services.prefs.setBoolPref("devtools.addon.auto-install", false);
> > +        AddonManager.removeAddonListener(listener);
> 
> It could be a good idea to reattach the keys and menu entries? 
> 
> Right now the disabling/uninstall of the addon really needs to be improved
> (disabling doesn't disconnect key handlers, uninstalling doesn't close
> existing toolboxes etc...). But if we manage to cleanly unload devtools, I
> think we should restore the browser back to its devtools-less state.

Yes, let's wait for cleaning up the removal on the add-on state first.

> ::: devtools/shim/devtools-startup.js:193
> (Diff revision 1)
> > +          this.hookKeys(subject);
> > +          this.hookToolsMenu(subject);
> > +          this.hookHamburgerMenu();
> 
> Given https://bugzilla.mozilla.org/show_bug.cgi?id=1371337#c3 I think it
> could be nice if we wrapped this new startup code in an idle callback to
> accommodate the performance comments we received on our current startup code.

Done.

> ::: devtools/shim/devtools-startup.js:197
> (Diff revision 1)
> > +        case "browser-delayed-startup-finished":
> > +          this.hookKeys(subject);
> > +          this.hookToolsMenu(subject);
> > +          this.hookHamburgerMenu();
> > +          break;
> > +        case "browser-inspect-node":
> 
> For info, in Bug 1372520, we ended up adding an inspectNode method on the
> DevToolsShim. It was preferred to using an event during the review and I
> didn't push back.
> 
> We can still fire this event from the DevToolsShim. Another thing we need to
> update is that the menu entry is hidden now if devtools are not installed.
> Once the UI shim is ready we will need to reenable the menu item even if
> devtools are not installed.

Let's figure out the context menu in a followup.

> ::: devtools/shim/devtools-startup.js:254
> (Diff revision 1)
> > +        },
> > +        onInstallFailed(install) {
> > +          installFailureHandler(install, "Install failed");
> > +        },
> > +      };
> > +      AddonManager.getInstallForURL(this.addonURL, install => {
> 
> Is it ok to install the addon in case the addon was simply disabled ?
> Reading AddonManager.jsm I couldn't find any API to simply enable addons
> though.

You can do addon.userDisabled = false to re-enable it,
I added coded in install() method to do that.

> ::: devtools/shim/devtools-startup.js:261
> (Diff revision 1)
> > +        install.install();
> > +      }, "application/x-xpinstall");
> > +    });
> > +  },
> > +
> > +  get keyShortcuts() {
> 
> Getting an exception in the console on browser startup: 
> 
> console.error: Devtools-Addon-Install: 
>   exception
>   Message: TypeError: setting getter-only property "keyShortcuts"
>   
> Had to define an empty setter to make it go away.

Hum, I'm pretty sure I fixed that in a branch somewhere, I pushed the wrong HEAD...
Fixed by using a defineLazyGetter.

> ::: devtools/shim/devtools-startup.js:291
> (Diff revision 1)
> > +
> > +      // The 3 following keys are also registered in /client/definitions.js
> > +      // and should be synced.
> > +
> > +      // Key for opening the Inspector
> > +      // 'C' on all but kk: K, el: 0, sk: r, eo: I, cy: A
> 
> I don't think we need those localization comments anymore now that we have a
> dedicated properties file.

Removed.

> ::: browser/locales/en-US/chrome/browser/devtools/shim.properties:10

Fixed!

	Alexandre Poirot [:ochameau] Assignee
	Comment 8 • 7 years ago

Pushed some fixes related to issues around browser-delayed-startup-finished listening.
Services.startup.startingUp was misleading.

	Julian Descottes [:jdescottes]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8883877 [details]
Bug 1378397 - Introduce shim for managing upcoming DevTools addon.

https://reviewboard.mozilla.org/r/154862/#review161732

Some comments and I want to discuss a topic: we have been discussing about a new devtools-startup to address startup performance issues. 
This is of course overlapping both with your patch and with my patch. We can land them in whatever order we want but I think we need to agree on the next steps for the devtools-startup component before moving forward.

Let's discuss this tomorrow!

::: devtools/shim/devtools-startup.js:19
(Diff revision 3)
> +"use strict";
> +
> +const { interfaces: Ci, utils: Cu } = Components;
> +
> +const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});

Would it make any sense to lazyload Services?

::: devtools/shim/devtools-startup.js:30
(Diff revision 3)
> +XPCOMUtils.defineLazyGetter(this, "console", () => {
> +  let {ConsoleAPI} = Cu.import("resource://gre/modules/Console.jsm", {});
> +  return new ConsoleAPI({
> +    maxLogLevelPref: "devtools.addon.install-log",
> +    prefix: "Devtools-Addon-Install",
> +  });
> +});

Since we are focusing on performances for startup code, did you measure the impact of using this? That's importing a module+reading a pref just for logging.

::: devtools/shim/devtools-startup.js:48
(Diff revision 3)
> +  "devtools",
> +  "start-debugger-server",
> +];
> +
> +// ID defined in add-on's install.rdf file
> +const AddonID = "devtools@mozilla.org";

nit: AddonID -> ADDON_ID

::: devtools/shim/devtools-startup.js:237
(Diff revision 3)
> +    CustomizableUI.removeWidgetFromArea("developer-button");
> +    CustomizableUI.destroyWidget("developer-button");

We are no longer adding anything to CustomizableUI, the only thing this would do would be to delete the button added by the real devtools startup?

::: devtools/shim/devtools-startup.js:275
(Diff revision 3)
> +    });
> +    if (addon) {
> +      if (addon.userDisabled) {
> +        addon.userDisabled = false;
> +      }
> +      return;

eslint: consistent return -> return Promise.resolve()

::: devtools/shim/devtools-startup.js:299
(Diff revision 3)
> +          let progress = install.maxProgress == -1 ? -1 :
> +            install.progress / install.maxProgress;
> +          console.log("Progress: " + progress);
> +        },
> +
> +        onInstallEnded({addon}) {

eslint: addon already declared

::: devtools/shim/devtools-startup.js:400
(Diff revision 3)
> +
> +  hookHamburgerMenu(window) {
> +    let { document } = window;
> +    // Replace the existing entry which displays the Web Developer menu entries
> +    // by a new one which will only open the install dialog
> +    let oldBtn = document.getElementById("appMenu-developer-button");

"developer-button" is the button that we are adding to the customizableUI and "appMenu-developer-button" is the new menu entry for devtools that comes with photon. 

It is risky to hijack the menu like that without using a proper API. What if the id changes? Same for the classname, I can imagine other devs being quite surprised if they start adding other classes and see the devtools button having a different style for no reason.

We could do the same as what we do for customizable UI: by default they don't create any devtools button but give us an API to to it.

We do not have to have a clean solution right now, but I think it is a must have before we enable addon updates.

::: devtools/shim/devtools-startup.js:405
(Diff revision 3)
> +    let oldBtn = document.getElementById("appMenu-developer-button");
> +    let newBtn = document.createElement("toolbarbutton");
> +    newBtn.className = "subviewbutton";
> +    newBtn.setAttribute("label", oldBtn.getAttribute("label"));
> +    newBtn.setAttribute("oncommand", "PanelUI.hide()");
> +    newBtn.addEventListener("command", this.showInstallDialog.bind(this, "hamburger", null));

nit: this.showInstallDialog.bind(this, "hamburger", null)
 => () => this.showInstallDialog("hamburger")
 
Easier to read + no need to protect the second argument. Same comment for the similar uses of bind in the file.

::: devtools/shim/devtools-startup.js:406
(Diff revision 3)
> +    let newBtn = document.createElement("toolbarbutton");
> +    newBtn.className = "subviewbutton";
> +    newBtn.setAttribute("label", oldBtn.getAttribute("label"));
> +    newBtn.setAttribute("oncommand", "PanelUI.hide()");
> +    newBtn.addEventListener("command", this.showInstallDialog.bind(this, "hamburger", null));
> +    newBtn.oldBtn = oldBtn;

We should be restoring oldBtn in cleanup()

::: devtools/shim/devtools-startup.js:426
(Diff revision 3)
> +      let { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +      let { gDevToolsBrowser } = require("devtools/client/framework/devtools-browser");
> +      if (reason == "menu" || reason == "hamburger" ||
> +          (reason == "key" &&
> +           (options.id == "toogleToolbox" || options.id == "toogleToolboxF12"))) {
> +        gDevToolsBrowser.toggleToolboxCommand(win.gBrowser);

This is a method from devtools. We should be careful with non devtools code calling devtools apis and consider using/extending the devtools-shim. 

Documentation for this method in devtools-browser states 'This function is for the benefit of Tools:DevToolbox in browser/base/content/browser-sets.inc and should not be used outside of there'.

If we use it here, we should at least update the comment.

If we want to try and use the shim here, we might have to wait on the devtools-shim devtools-registered event instead of just waiting for the addon install.

::: devtools/shim/devtools-startup.js:428
(Diff revision 3)
> +      if (reason == "menu" || reason == "hamburger" ||
> +          (reason == "key" &&
> +           (options.id == "toogleToolbox" || options.id == "toogleToolboxF12"))) {
> +        gDevToolsBrowser.toggleToolboxCommand(win.gBrowser);
> +      } else if (reason == "key") {
> +        gDevToolsBrowser.selectToolCommand(win.gBrowser, options.id);

Same comment about using devtools internal API from m-c code.

::: browser/locales/en-US/chrome/browser/devtools/shim.properties
(Diff revision 3)
> -# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-

HG thinks this file was created by duplicating a moz.build. If this can be cleaned up that would be nice.

::: devtools/shim/moz.build:18
(Diff revision 3)
> +if CONFIG['MOZ_DEVTOOLS'] == 'addon':
> +    EXTRA_COMPONENTS += [
> +        'devtools-startup.js',
> +        'devtools-startup.manifest',
> +    ]

On one hand it's great to only bundle this when needed to avoid slowing down nightly for no reason, but that means this component will only actually get tested on builds with devtools='addon'.

If I quickly think about the hack we are doing with the hamburger menu. Imagine the responsible team changes the id of the button 1 week before a merge to beta. In this timeframe, potentially we don't do any QA on addon builds (maybe we just released a version). This goes to beta with a devtools-startup.js which tries to get a button with id "appMenu-developer-button" which no longer exists.

I would feel much more confident if we could end up with a single devtools-startup component, that would be able to either install the addon or simply initialize devtools depending on the context. 

As long as enabling addon updates is not on the horizon, it is ok to hide this new code behind the [addon] flag.

Let us discuss this

::: devtools/shim/preferences.js:10
(Diff revision 3)
> +/* global pref */
> +
> +// DevTools add-on is automatically installed on startup if:
> +// - they were opened once,
> +// - any DevTools command line argument is passed,
> +// This pref allow to disable that.

nit: allow -> allows

	Alexandre Poirot [:ochameau] Assignee
	Comment 12 • 7 years ago

I pushed a new patch which mostly rebase on top of bug 1359855.
I still need to address a couple of comments but the patch changed in various ways as we are using only one devtools-startup.js file.