1286356 - event-emitter.js requires Console.jsm

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

Bug 1275330 added a require of Console.jsm to event-emitter.js.
This has to be lifted for de-chrome-ification.

Comment 1

[Tom Tromey :tromey]

Attachment: Bug 1286356 - isolate import of Console in event-emitter;

Review commit: https://reviewboard.mozilla.org/r/66132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66132/

Comment 2

[Greg Tatum [:gregtatum]]

Comment on attachment 8773421 [details]
Bug 1286356 - isolate import of Console in event-emitter;

https://reviewboard.mozilla.org/r/66132/#review63204

The only issue I see is the globally-scoped console, everything else looks fine.

::: devtools/shared/event-emitter.js:10
(Diff revision 1)
>  "use strict";
>  
>  (function (factory) {
>    if (this.module && module.id.indexOf("event-emitter") >= 0) {
> +    if (isWorker) {
> +      this.console = {

From how I'm understanding this code, right here you are setting a global console object. In the existing code on line 47 the console is scoped to the factory function's frame.

It seems like it should be something like


```js
let console = isWorker
  ? { error: () => {} }
  : Cu.import( ... )
```

And then pass it into the factory function.

```js
factory.call(this, require, this, { exports: this }, console);
```

::: devtools/shared/event-emitter.js:28
(Diff revision 1)
>      // Bug 1259045: This module is loaded early in firefox startup as a JSM,
>      // but it doesn't depends on any real module. We can save a few cycles
>      // and bytes by not loading Loader.jsm.
>      let require = function (module) {
> -      const Cu = Components.utils;
>        switch (module) {

I think it would be nice while we're here to add a comment about how this is a mixed-privilege space where we can have Cu.import. I'm not sure on the ultimate migration path if this will all be content code or not.

::: devtools/shared/event-emitter.js:45
(Diff revision 1)
>      };
>      factory.call(this, require, this, { exports: this });
>      this.EXPORTED_SYMBOLS = ["EventEmitter"];
>    }
>  }).call(this, function (require, exports, module) {
>    let EventEmitter = this.EventEmitter = function () {};

And then another comment here about no-privileged code here, that it must be injected.

Comment 3

[Tom Tromey :tromey]

Comment on attachment 8773421 [details]
Bug 1286356 - isolate import of Console in event-emitter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66132/diff/1-2/

Comment 4

[Greg Tatum [:gregtatum]]

Comment on attachment 8773421 [details]
Bug 1286356 - isolate import of Console in event-emitter;

https://reviewboard.mozilla.org/r/66132/#review63280

::: devtools/shared/event-emitter.js:11
(Diff revision 2)
>  
>  (function (factory) {
> +  // This file can be loaded in several different ways.  It can be
> +  // require()d, either from the main thread or from a worker thread;
> +  // or it can be imported via Cu.import.  These different forms
> +  // explain some of the hairness of this code.

nit: s/hairness/hairiness/

::: devtools/shared/event-emitter.js:22
(Diff revision 2)
> +  // plus the lack of |console| in workers, results in some gyrations
> +  // in the definition of |console|.
>    if (this.module && module.id.indexOf("event-emitter") >= 0) {
> +    let console;
> +    if (isWorker) {
> +      this.console = {

Hmm... this will still create a global console object instead of a locally scoped one, as `this` refers to the global object, not the local lexical scope.

Comment 5

[Tom Tromey :tromey]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #4)

> > +      this.console = {
> 
> Hmm... this will still create a global console object instead of a locally
> scoped one, as `this` refers to the global object, not the local lexical
> scope.

Dang it, I thought I changed that.
Thanks for catching this.

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8773421 [details]
Bug 1286356 - isolate import of Console in event-emitter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66132/diff/2-3/

Comment 7

[Greg Tatum [:gregtatum]]

Comment on attachment 8773421 [details]
Bug 1286356 - isolate import of Console in event-emitter;

https://reviewboard.mozilla.org/r/66132/#review63614

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46de97426f7b
isolate import of Console in event-emitter; r=gregtatum

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46de97426f7b