1248600 - Remove manual call to gDevToolsBrowser.registerBrowserWindow/forgetBrowserWindow from /browser/

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

See bug 1248348.
I'm trying to remove the last devtools bits from /browser/,
while ensuring the hot reload addon is able to actually reload everything without hacks.

Here is the two usages to be removed:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1298
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1298

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Removing these calls isn't completely obvious,
but also, I feel like these hardcoded calls in middle of Firefox load codepath
is some historical leftover...
I don't see why devtools would have to be called like that
and not listen for some generic events.

First, I'm making the devtools command line handler more important.
This XPCOM component is running very early during process startup and should be
the only one thing being loaded by default.
It will become our main entry point.
It defines what has to be loaded and when.
If we want to load things more lazily, we have to start from here.

This module now listen for browser-delayed-startup-finished, even if no command line
argument is given. Then, it loads devtools-browser.js, which will watch top level browser.xul windows.
I think, this move is going to help me find better way to coordinate devtools-browser.js and main.js.

I started added comment on top of modules in order to help understand what each of these do.

Then, I tweaked gDevToolsBrowser in order to iterate over existing windows
and listen for new one to come.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8719846 [details] [diff] [review]
patch v1

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

::: devtools/client/devtools-clhandler.js
@@ +6,5 @@
>     bug 1242893 is fixed */
>  /* globals BrowserToolboxProcess */
>  
> +/**
> + * This XPCOM component is loaded very early.

I think this makes sense overall.  But this file is no longer about just command line args, so it needs a new name.  Maybe `startup.js`?

Also `devtoolsCommandlineHandler` could use a more generic name, now that it also does general startup too.

@@ +51,5 @@
>      if (debuggerServerFlag) {
>        this.handleDebuggerServerFlag(cmdLine, debuggerServerFlag);
>      }
> +
> +    let onStartup = function (window) {

Nit: remove space "n ("

::: devtools/client/framework/devtools-browser.js
@@ +4,5 @@
>  
>  "use strict";
>  
> +/**
> + * This is the main module loaded in Firefox desktop

Nit: Please wrap to 80 chars, seems wrapped randomly.

@@ +335,5 @@
>     * @param {XULDocument} doc
>     *        The document to which menuitems and handlers are to be added
>     */
>    // Used by browser.js
>    registerBrowserWindow: function DT_registerBrowserWindow(win) {

Remove "used by" comment and add "_" prefix since it's internal only now.

@@ +765,5 @@
>     *
>     * @param  {XULWindow} win
>     *         The window containing the menu entry
>     */
>    forgetBrowserWindow: function DT_forgetBrowserWindow(win) {

Add "_" prefix since it's internal only now

Comment 3

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6981eb6ccad1

Comment 4

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=190192d49b57

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Renamed to devtools-start.js / DevtoolsStartup.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8720785 [details] [diff] [review]
patch v2

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

Please double-check browser toolbox to make sure it's still working.

::: devtools/client/devtools-startup.js
@@ +24,5 @@
>  ];
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm");
>  
> +function DevtoolsStartup() {}

Nit: DevToolsStartup

::: devtools/client/framework/devtools-browser.js
@@ +9,5 @@
> + * windows and coordinates devtools around each window.
> + *
> + * This module is loaded lazily by devtools-clhandler.js, once the first
> + * browser window is ready (i.e. fired browser-delayed-startup-finished event)
> + *

Nit: remove blank line

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

We now have a patch checking that the browser toolbox still works :)
But I gave it a try manually.

Comment 8

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/3b17489c9d9835fd44cd9e7a818d9f91da5870b0
Bug 1248600 - Remove registerBrowserWindow/forgetBrowserWindow calls from /browser/. r=jryans

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for ESLint failures: https://treeherder.mozilla.org/logviewer.html#?job_id=7448336&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/8ff368f8075c

Comment 10

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4106f12d3e3

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Fixed eslint.

Happy to learn the hard way that eslint is now running on CI!
And now ./mach eslint --setup works correctly locally.

Comment 12

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/ba186717174aec9677487aa47caa84fc1430e334
Bug 1248600 - Remove registerBrowserWindow/forgetBrowserWindow calls from /browser/. r=jryans

Comment 13

[Carsten Book [:Tomcat]]

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