Closed Bug 1245462 Opened 5 years ago Closed 5 years ago

Remove usages of gDevTools from /devtools/

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

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
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.
Attachment #8717374 - Flags: review?(jryans)
Here is the complex scenarios, similar to the browser toolbox issue I hit in previous bug.
While working on this, I couldn't resist cleaning up some require/import...
Attachment #8717376 - Flags: review?(jryans)
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!
Attachment #8717377 - Flags: review?(jryans)
Try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=00c06dcd5628
Green except for bug 1246692's patch included in this run.
Attachment #8717375 - Flags: review?(jryans)
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?
Attachment #8717374 - Flags: review?(jryans) → review-
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?
Attachment #8717376 - Flags: review?(jryans) → review-
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.
Attachment #8717375 - Flags: review?(jryans) → review+
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.
Attachment #8717374 - Flags: review- → review+
Attachment #8717375 - Attachment is obsolete: true
Good catch!

What do you think about merging all these patches before landing?
Attachment #8717977 - Flags: review?(jryans)
Attachment #8717376 - Attachment is obsolete: true
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.
Attachment #8717977 - Flags: review?(jryans) → review+
Blocks: 1247270
Blocks: 1247858
Depends on: 1249049
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.