In order to help running tools in a tab like debugger, inspector and netmonitor, we should stop using Cu.import to import devtools modules. event-emitter is one such example. Now that this module has been copied to toolkit in order, we can remove the boilerplate and always load it via require(). It would also help bug 1384527, by removing an occurence of Promise.jsm and bug 1381542 by removing this hard to maintain compatility code.

Comment on attachment 8892488 [details] Bug 1386299 - Stop importing event-emitter as a JSM. https://reviewboard.mozilla.org/r/163460/#review169128 That's a great cleanup, thanks! We can probably get rid of devtools/client/inspector/webpack/rewrite-event-emitter.js, which was explicitly ignoring all the beginning of the event-emitter file for some reason. Given that inspector in launchpad is broken at the moment, don't do it right now since we have no way to test it. ::: devtools/client/shared/widgets/AbstractTreeItem.jsm:15 (Diff revision 1) > const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {}); > const { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm"); > const { ViewHelpers } = require("devtools/client/shared/widgets/view-helpers"); > const { KeyCodes } = require("devtools/client/shared/keycodes"); > > -XPCOMUtils.defineLazyModuleGetter(this, "EventEmitter", > +const EventEmitter = require("devtools/shared/event-emitter"); Could use loader.lazyRequireGetter to be consistent with the previous lazyModuleGetter. For the record the difference is that now the module is loaded as soon as you open the performance tool. With lazy loading it is only fetched when you have a perf recording and display the call tree. ::: devtools/shared/event-emitter.js (Diff revision 1) > - return Cu.import("resource://gre/modules/Promise.jsm", {}).Promise.defer; > - case "Services": > - return Cu.import("resource://gre/modules/Services.jsm", {}).Services; > - case "devtools/shared/platform/stack": { > - let obj = {}; > - Cu.import("resource://devtools/shared/platform/chrome/stack.js", obj); This was the only spot loading devtools/shared/platform/chrome/stack.js via Cu.import. This file has a similar JSM boilerplate as the event-emitter. Does it mean we can clean it up now too as a follow up? ::: devtools/shared/event-emitter.js:7 (Diff revision 1) > - let EventEmitter = this.EventEmitter = function () {}; > +let EventEmitter = this.EventEmitter = function () {}; > - module.exports = EventEmitter; > +module.exports = EventEmitter; We usually require dependencies + initialize cosnt first. These two lines should be moved right before EventEmitter.decorate. (not related to your change, but since the file looks less funky now, let's make it consistent with the rest of the codebase) ::: devtools/shared/event-emitter.js:10 (Diff revision 1) > - // After this point the code may not use Cu.import, and should only > - // require() modules that are "clean-for-content". > - let EventEmitter = this.EventEmitter = function () {}; > +let EventEmitter = this.EventEmitter = function () {}; > - module.exports = EventEmitter; > +module.exports = EventEmitter; > > - // See comment in JSM module boilerplate when adding a new dependency. > +// See comment in JSM module boilerplate when adding a new dependency. Remove this comment

(In reply to Julian Descottes [:jdescottes] from comment #2) > Comment on attachment 8892488 [details] > Bug 1386299 - Stop importing event-emitter as a JSM. > > https://reviewboard.mozilla.org/r/163460/#review169128 > > That's a great cleanup, thanks! > > We can probably get rid of > devtools/client/inspector/webpack/rewrite-event-emitter.js, which was > explicitly ignoring all the beginning of the event-emitter file for some > reason. Given that inspector in launchpad is broken at the moment, don't do > it right now since we have no way to test it. Oh, yes I forgot about that. Tom already mentioned that file. > This was the only spot loading devtools/shared/platform/chrome/stack.js via > Cu.import. > > This file has a similar JSM boilerplate as the event-emitter. Does it mean > we can clean it up now too as a follow up? Yes, and we should also convert all these jsm to regular modules. (like devtools/client/shared/widgets/*.jsm) (see bug 1182194) I did that a long time ago for the server, but this is still a work to be done in client/shared.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c7f4ecbc3658 Stop importing event-emitter as a JSM. r=jdescottes

Backed out for failing toolkit/components/extensions/test/browser/browser_ext_management_themes.js: https://hg.mozilla.org/integration/autoland/rev/7105d0571b3184589464a86fc8dca8609191407e Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c7f4ecbc365808e75fd0d80f14cd28a6b339930c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120656065&repo=autoland [task 2017-08-03T09:38:32.043668Z] 09:38:32 INFO - Console message: [JavaScript Error: "ReferenceError: require is not defined" {file: "resource://devtools/shared/event-emitter.js" line: 7}] [task 2017-08-03T09:38:32.045038Z] 09:38:32 INFO - Console message: [JavaScript Error: "ReferenceError: EventEmitter is not defined" {file: "chrome://extensions/content/ext-management.js" line: 97}] [task 2017-08-03T09:38:32.047619Z] 09:38:32 INFO - Console message: [JavaScript Error: "ReferenceError: EventEmitter is not defined" {file: "chrome://extensions/content/ext-management.js" line: 97}] [task 2017-08-03T09:38:32.048989Z] 09:38:32 INFO - Console message: [JavaScript Error: "ReferenceError: EventEmitter is not defined" {file: "chrome://extensions/content/ext-management.js" line: 97}] [task 2017-08-03T09:38:32.050756Z] 09:38:32 INFO - Buffered messages finished [task 2017-08-03T09:38:32.052440Z] 09:38:32 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_management_themes.js | Test timed out - [task 2017-08-03T09:38:32.055110Z] 09:38:32 INFO - Not taking screenshot here: see the one that was previously logged [task 2017-08-03T09:38:32.059187Z] 09:38:32 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/browser/browser_ext_management_themes.js | no tasks awaiting on messages - Got ["onInstalled"], expected [] [task 2017-08-03T09:38:32.061156Z] 09:38:32 INFO - Stack trace: [task 2017-08-03T09:38:32.065414Z] 09:38:32 INFO - chrome://mochikit/content/browser-test.js:test_is:1002 [task 2017-08-03T09:38:32.070242Z] 09:38:32 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:31 [task 2017-08-03T09:38:32.072094Z] 09:38:32 INFO - chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest<:409 [task 2017-08-03T09:38:32.073787Z] 09:38:32 INFO - timeoutFn@chrome://mochikit/content/browser-test.js:893:9 [task 2017-08-03T09:38:32.075619Z] 09:38:32 INFO - setTimeout handler*Tester_execTest@chrome://mochikit/content/browser-test.js:855:9 [task 2017-08-03T09:38:32.077252Z] 09:38:32 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:689:7 [task 2017-08-03T09:38:32.079034Z] 09:38:32 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

I forgot about the event-emitter usage from WebExtension codebase, we should get rid of this one in favor of toolkit's EventEmitter.jsm

Comment on attachment 8893393 [details] Bug 1386299 - Make WebExtension use toolkit's EventEmitter instead of DevTools one. https://reviewboard.mozilla.org/r/164512/#review169772

This time with a green try with browser mochitests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0728c771d33&selectedJob=120730688

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b63ee28352e4 Make WebExtension use toolkit's EventEmitter instead of DevTools one. r=rpl https://hg.mozilla.org/integration/autoland/rev/8b64ca81f61a Stop importing event-emitter as a JSM. r=jdescottes

https://hg.mozilla.org/mozilla-central/rev/b63ee28352e4 https://hg.mozilla.org/mozilla-central/rev/8b64ca81f61a