1245462 - Remove usages of gDevTools from /devtools/

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

Bug 1188405 is about to split gDevTools.jsm into modules. This is going to help reloading them once the loader reloads.
gDevTools.jsm will stay to keep add-ons working, but we should remove usage of it in our codebase. Instead, we should require the relevant module:
- devtools/client/framework/devtools for gDevTools
- devtools/client/framework/devtools-browser for gDevToolsBrowser

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: DevTools is not longer exposed by gDevTools.jsm

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Replace usages of gDevTools.jsm by module imports - v1

Main patch. Convert all Cu.import/require to just require(devtools) or require(devtools-browser).
I removed some lazy getter here and there as devtools is loaded anyway.

I included here only the more obvious replacements and kept more complex scenario for next patches.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Non naive replacements of gDevTools.jsm - v1

Here is the complex scenarios, similar to the browser toolbox issue I hit in previous bug.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Various require/import cleanups - v1

While working on this, I couldn't resist cleaning up some require/import...

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Explicitely import gDevTools - v1

toolbox.js was using Cu.import(gDevTools) without second argument
and ended up injecting gDevTools into all next modules being loaded.
Hopefully, fonts.js seems to be the only one relying on this!

Comment 6

[Alexandre Poirot [:ochameau]]

Try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=00c06dcd5628
Green except for bug 1246692's patch included in this run.

Comment 7

[Alexandre Poirot [:ochameau]]

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

Comment 8

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

Comment on attachment 8717374 [details] [diff] [review]
Replace usages of gDevTools.jsm by module imports - v1

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

::: devtools/client/debugger/debugger-commands.js
@@ +7,5 @@
>  "use strict";
>  
>  const { Cc, Ci, Cu } = require("chrome");
>  const l10n = require("gcli/l10n");
> +const { gDevTools } = require("devtools/client/framework/devtools");

Should it be lazy?

::: devtools/client/framework/toolbox-options.js
@@ +8,5 @@
>  const Services = require("Services");
>  const promise = require("promise");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
> +const {gDevTools} = require("devtools/client/framework/devtools");

Should it be lazy?

::: devtools/client/inspector/rules/rules.js
@@ +23,5 @@
>  const {RuleEditor} =
>        require("devtools/client/inspector/rules/views/rule-editor");
>  const {createChild, promiseWarn} =
>        require("devtools/client/inspector/shared/utils");
> +const {gDevTools} = require("devtools/client/framework/devtools");

Should it be lazy?

::: devtools/client/shared/autocomplete-popup.js
@@ +8,5 @@
>  const {Cc, Ci, Cu} = require("chrome");
>  const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>  
>  loader.lazyImporter(this, "Services", "resource://gre/modules/Services.jsm");
> +const {gDevTools} = require("devtools/client/framework/devtools");

Should it be lazy?

::: devtools/client/shared/theme.js
@@ +11,5 @@
>  
>  const { Ci, Cu } = require("chrome");
>  const { NetUtil } = Cu.import("resource://gre/modules/NetUtil.jsm", {});
>  loader.lazyRequireGetter(this, "Services");
> +const { gDevTools } = require("devtools/client/framework/devtools");

Should it be lazy?

::: devtools/client/shared/view-source.js
@@ +8,3 @@
>  loader.lazyImporter(this, "Task", "resource://gre/modules/Task.jsm");
>  
> +var {gDevTools} = require("devtools/client/framework/devtools");

Should it be lazy?

::: devtools/server/actors/csscoverage.js
@@ +11,5 @@
>  
>  const events = require("sdk/event/core");
>  const protocol = require("devtools/server/protocol");
>  const { method, custom, RetVal, Arg } = protocol;
> +const {gDevTools} = require("devtools/client/framework/devtools");

Should it be lazy?

::: devtools/shared/gcli/commands/calllog.js
@@ +6,5 @@
>  
>  const { Cc, Ci, Cu } = require("chrome");
>  const l10n = require("gcli/l10n");
>  const gcli = require("gcli/index");
> +const { gDevTools } = require("devtools/client/framework/devtools");

Should it be lazy?

::: devtools/shared/gcli/commands/csscoverage.js
@@ +5,5 @@
>  "use strict";
>  
>  const { Cc, Ci } = require("chrome");
>  
> +const { gDevTools } = require("devtools/client/framework/devtools");

Should it be lazy?

Comment 9

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

Comment on attachment 8717376 [details] [diff] [review]
Various require/import cleanups - v1

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

::: devtools/client/responsivedesign/responsivedesign.jsm
@@ -11,5 @@
> -Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> -Cu.import("resource://devtools/client/framework/gDevTools.jsm");
> -Cu.import("resource://devtools/shared/event-emitter.js");
> -Cu.import("resource://devtools/client/shared/widgets/ViewHelpers.jsm");
> -XPCOMUtils.defineLazyModuleGetter(this, "SystemAppProxy",

This is still used in `buildPhoneUI`

::: devtools/client/webconsole/hudservice.js
@@ +20,4 @@
>  loader.lazyRequireGetter(this, "gDevTools", "devtools/client/framework/devtools", true);
>  loader.lazyRequireGetter(this, "DebuggerServer", "devtools/server/main", true);
>  loader.lazyRequireGetter(this, "DebuggerClient", "devtools/shared/client/main", true);
> +loader.lazyRequireGetter(this, "showDoorhanger", "devtools/client/shared/doorhanger");

Needs `true` arg as well?

Comment 10

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

Comment on attachment 8717375 [details] [diff] [review]
Non naive replacements of gDevTools.jsm - v1

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

::: devtools/client/devtools-clhandler.js
@@ +54,5 @@
> +      // Ensure loading main devtools module that hooks up into browser UI
> +      // and initialize all devtools machinery.
> +      // browser.xul or main top-level document used to load this module,
> +      // but this code may be called without/before it.
> +      require("devtools/client/framework/devtools-browser");

Mention bug 1247203 here as well.

Comment 11

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

Comment on attachment 8717374 [details] [diff] [review]
Replace usages of gDevTools.jsm by module imports - v1

Agreed on IRC laziness is overall, since the module should generally be loaded in every case anyway.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Non naive replacements of gDevTools.jsm - v2

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Various require/import cleanups - v2

Good catch!

What do you think about merging all these patches before landing?

Comment 14

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

Comment on attachment 8717977 [details] [diff] [review]
Various require/import cleanups - v2

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

I am fine with landing small commits personally, but do what makes sense to you.

Comment 15

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/6889447e1634ec55711e5ffec38fe8c74068fef2
Bug 1245462 - Replace usages of gDevTools.jsm by module imports. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/03788fdd62056997891df07af06766a7ec3f1f52
Bug 1245462 - Cleanup various require/import in devtools. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/94a25d092e44055eba9c830913d43b8a4f91d898
Bug 1245462 - DevTools is not longer exposed by gDevTools.jsm. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/96951f405d9e7abb431aaeb72ffa25498a0b6ffb
Bug 1245462 - Explicitely import gDevTools r=jryans

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6889447e1634
https://hg.mozilla.org/mozilla-central/rev/03788fdd6205
https://hg.mozilla.org/mozilla-central/rev/94a25d092e44
https://hg.mozilla.org/mozilla-central/rev/96951f405d9e