Closed Bug 1248600 Opened 6 years ago Closed 6 years ago

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


(DevTools :: General, defect)

Not set


(firefox47 fixed)

Firefox 47
Tracking Status
firefox47 --- fixed


(Reporter: ochameau, Assigned: ochameau)




(1 file, 3 obsolete files)

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:
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #8719846 - Flags: review?(jryans)
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
Attachment #8719846 - Flags: review?(jryans) → feedback+
Attached patch patch v2 (obsolete) — Splinter Review
Renamed to devtools-start.js / DevtoolsStartup.
Attachment #8720785 - Flags: review?(jryans)
Attachment #8719846 - Attachment is obsolete: true
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
Attachment #8720785 - Flags: review?(jryans) → review+
Attached patch patch v3 (obsolete) — Splinter Review
We now have a patch checking that the browser toolbox still works :)
But I gave it a try manually.
Attachment #8721984 - Flags: review+
Attachment #8720785 - Attachment is obsolete: true
Bug 1248600 - Remove registerBrowserWindow/forgetBrowserWindow calls from /browser/. r=jryans
Attached patch patch v4Splinter Review
Fixed eslint.

Happy to learn the hard way that eslint is now running on CI!
And now ./mach eslint --setup works correctly locally.
Attachment #8722409 - Flags: review+
Attachment #8721984 - Attachment is obsolete: true
Bug 1248600 - Remove registerBrowserWindow/forgetBrowserWindow calls from /browser/. r=jryans
Flags: needinfo?(poirot.alex)
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Blocks: 1259045
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.