Closed Bug 1286356 Opened 4 years ago Closed 4 years ago

event-emitter.js requires Console.jsm

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file)

Bug 1275330 added a require of Console.jsm to event-emitter.js.
This has to be lifted for de-chrome-ification.
Flags: qe-verify-
Whiteboard: [devtools-html] → [devtools-html] [triage]
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
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.
Attachment #8773421 - Flags: review?(gtatum) → review-
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/
Attachment #8773421 - Flags: review- → review?(gtatum)
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.
Attachment #8773421 - Flags: review?(gtatum) → review-
(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 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/
Attachment #8773421 - Flags: review- → review?(gtatum)
Comment on attachment 8773421 [details]
Bug 1286356 - isolate import of Console in event-emitter;

https://reviewboard.mozilla.org/r/66132/#review63614
Attachment #8773421 - Flags: review?(gtatum) → review+
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46de97426f7b
isolate import of Console in event-emitter; r=gregtatum
https://hg.mozilla.org/mozilla-central/rev/46de97426f7b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.