Open Bug 1380806 Opened 2 years ago Updated Last year

Pull out devtools command line implementation into its own module

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox57 fix-optional)

Tracking Status
firefox57 --- fix-optional

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file)

devtools/client/devtools-startup.js is the main entry point for devtools.
It is loaded very early during Firefox started.
Lazyness of it is important and we should try to load only what is necessary over time.
Bug 1359855 is going to delay even more any module loading until it is really needed. After that, this file is going to grow and there is a significant part, related to command line argument implementation that could be stripped in its own module as it is only useful for people passing a devtools command.

Doing that would also help modifying the implementation of command line arguments from the devtools add-on.
Assignee: nobody → poirot.alex
Comment on attachment 8886387 [details]
Bug 1380806 - Move DevTools command line implementation to its own module.

https://reviewboard.mozilla.org/r/157138/#review171636

Thanks for the cleanup! A few comments but I don't think another review round is necessary here.

::: devtools/client/framework/command-line.js:14
(Diff revision 3)
> -      id: "scratchpad",
> -      shortcut: Bundle.GetStringFromName("scratchpad.commandkey"),
> -      modifiers: "shift"
> -    },
>  
> -    // The following keys are also registered in /client/definitions.js
> +module.exports = {

Add a comment here to describe the role of the file.

::: devtools/client/framework/command-line.js:16
(Diff revision 3)
> -   * Flag that indicates if the developer toggle was already added to customizableUI.
> -   */
> -  developerToggleCreated: false,
> -
> -  handle: function (cmdLine) {
>      let consoleFlag = cmdLine.handleFlag("jsconsole", false);

It could be helpful here to mention that any new flag should also be added to the `const DevToolsFlags` defined in devtools-startup.js

::: devtools/client/framework/command-line.js:41
(Diff revision 3)
> -    // Only top level Firefox Windows fire a browser-delayed-startup-finished event
>      let onWindowReady = window => {
> -      this.hookWindow(window);
> +			Services.obs.removeObserver(onWindowReady, "browser-delayed-startup-finished");
> -
> -      if (devtoolsFlag) {
> -        this.handleDevToolsFlag(window);
> +		        this.handleDevToolsFlag(window);

eslint: tabs

::: devtools/client/framework/command-line.js:41
(Diff revision 3)
> -    // Only top level Firefox Windows fire a browser-delayed-startup-finished event
>      let onWindowReady = window => {
> -      this.hookWindow(window);
> +			Services.obs.removeObserver(onWindowReady, "browser-delayed-startup-finished");
> -
> -      if (devtoolsFlag) {
> -        this.handleDevToolsFlag(window);
> +		        this.handleDevToolsFlag(window);

missing `if (devtoolsFlag)`. But it's great that we no longer have to set devtoolsFlag to false, that felt really hacky.

::: devtools/client/framework/command-line.js:192
(Diff revision 3)
> -        }
> -      });
> -    }
> -  }
>  };
>  

nit: remove extra new line at the end of this file
Attachment #8886387 - Flags: review?(jdescottes) → review+
Priority: -- → P3
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.