Unify DevTools' EventEmitter and sdk/event/*

RESOLVED FIXED in Firefox 57

Status

P1
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: zer0, Assigned: zer0)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [nosdk] )

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox 56
Flags: qe-verify-
(Assignee)

Updated

a year ago
Depends on: 1382575
(Assignee)

Updated

a year ago
Depends on: 1382576
(Assignee)

Updated

a year ago
Depends on: 1382577
(Assignee)

Updated

a year ago
Depends on: 1382578
(Assignee)

Updated

a year ago
Depends on: 1382580
(Assignee)

Updated

a year ago
Depends on: 1382581
(Assignee)

Updated

a year ago
Depends on: 1382582
(Assignee)

Updated

a year ago
Depends on: 1382583
(Assignee)

Updated

a year ago
Depends on: 1382584
(Assignee)

Updated

a year ago
Depends on: 1382585
(Assignee)

Updated

a year ago
Depends on: 1382599
(Assignee)

Updated

a year ago
Depends on: 1382600
(Assignee)

Updated

a year ago
Depends on: 1382601
(Assignee)

Updated

a year ago
Depends on: 1382602
(Assignee)

Updated

a year ago
Depends on: 1382603
(Assignee)

Updated

a year ago
Depends on: 1382604
(Assignee)

Updated

a year ago
Depends on: 1382605
(Assignee)

Updated

a year ago
Depends on: 1382606
(Assignee)

Updated

a year ago
Depends on: 1382607
(Assignee)

Updated

a year ago
Depends on: 1382609
For information, we recently duplicated devtools' event emitter to /toolkit in Bug 1356231, for the gofaster projet. There was non devtools code relying on our event-emitter, so the goal was to have a copy that would stay in mozilla-central after devtools move out.

This change means that the two copies of event emitter will have different APIs now. Not a big deal, but I think it's good to be aware of that. 

http://searchfox.org/mozilla-central/source/toolkit/modules/EventEmitter.jsm
It is fine diverging, but Kris comments over bug 952653 were meaningful. So modifications made to event-emitter API/module should be reviewed accordingly.

Otherwise I would like to raise something important here in term of project management.
Unification is a "nice to have".
Regarding SDK removal, we only have to refactor the modules using SDK event emitters:
http://searchfox.org/mozilla-central/search?q=sdk%2Fevent&case=false&regexp=false&path=devtools%2F
which is mostly related to protocoljs events and related tests.

We shouldn't delay SDK removal for an optional significant refactoring!
(Assignee)

Comment 3

a year ago
That's right, the unification was a way to reduce the entropy of our event system, addressing the Kris' comments, and speed up the process of SDK removal.

However, fixing the usage of the "old" Event Emitter in the process is definitely not part of SDK removal; and it's definitely slow us down; so I'm going to propose the new Event Emitter patch for landing in our code base, while we're temporary keeping the old Event Emitter as well. In that way, we can focusing on replacing the SDK modules with the new EventEmitter (that is compatible), and we can move from the old ones to the new ones in a following step.
The cons, we're going to have temporary three different EventEmitter in our codebase, but I think we can live with that, and it's better IMVHO than just move the SDK modules into devtools (since they're also a performance concern).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8890127 - Attachment is obsolete: true
Attachment #8890127 - Flags: review?(poirot.alex)
(Assignee)

Updated

a year ago
Attachment #8890129 - Attachment is obsolete: true
Attachment #8890129 - Flags: review?(poirot.alex)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
Comment on attachment 8890128 [details]
Bug 1381542 - renamed "devtools/shared/event-emitter" in "devtools/shared/old-event-emitter";

https://reviewboard.mozilla.org/r/161206/#review166724

I haven't looked into each modification, I just assume your script to replace the require path is correct.
So a green try would be enough.

Feel free to land this patch first, independantly, as it may easily conflict over time and I will surely have comments for the other patches.
Attachment #8890128 - Flags: review?(poirot.alex) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8890128 [details]
Bug 1381542 - renamed "devtools/shared/event-emitter" in "devtools/shared/old-event-emitter";

https://reviewboard.mozilla.org/r/161206/#review166730

Note that if you land it first, you would need to modify devtools/shared/tests/mochitest/test_eventemitter_basic.html to refer to old-event-emitter.
(Assignee)

Updated

a year ago
See Also: → bug 1384546

Comment 12

a year ago
mozreview-review
Comment on attachment 8890303 [details]
Bug 1381542 - refactored EventEmitter with old SDK event/core capabilities;

https://reviewboard.mozilla.org/r/161430/#review166740

Thanks, it is looking great! I have a couple of comments before proceeding.

It would be useful to confirm that test_eventemitter_basic.html works as-is.
It is fine removing it and replace it with test_eventemitter_basic.js (That's great seeing it become an xpcshell test!), but it is important to verify that the new event emitter implementation pass previous tests as-is.
Have you tried running it locally before removing it?

What is your plan next to address concerns about shapes and inheritance:
  https://bugzilla.mozilla.org/show_bug.cgi?id=952653#c20
Do you plan to avoid using EventEmitter.decorate and refactor to classes in some places?
Or do you plan to expose EventEmitter.inherit to do not have to depend on ES6 classes before improving things?

And so, we have three APIs in this module:
* EventEmitter.decorate
* EventEmitter static methods on/once/off/emit
* inheritance
It looks like you would like to deprecate the static methods which related to the SDK version.
Should we mention that in the comments that only one usage is suggested?
Do you plan to refactor the deprecated usages during the NoSDK project?

::: devtools/shared/event-emitter.js:60
(Diff revision 1)
>      this.EXPORTED_SYMBOLS = ["EventEmitter"];
>    }
>  }).call(this, function (require, exports, module, console) {
>    // ⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠⚠
>    // After this point the code may not use Cu.import, and should only
>    // require() modules that are "clean-for-content".

It is unclear what bug is going to land first,
but note that in bug 1384527, I'm converting event-emitter to a commonjs module only. So it won't be used as a jsm anymore and I'm dropping the whole module boiterplate.

::: devtools/shared/event-emitter.js:128
(Diff revision 1)
>  
> -    /**
> -     * Remove a previously-registered event listener.  Works for events
> -     * registered with either on or once.
> -     *
> -     * @param string event
> +        if (listenersForType) {
> +          if (listenersForType.has(listener)) {
> +            listenersForType.delete(listener);
> +          } else {
> +            for (let value of listenersForType.values()) {

This block is far from being obvious.
It would be great to comment that we are cleaning up special listeners for "once".

Also, I know you just refactored and kept the same namings, but what do you think about renaming "originalListener" to "onceEventListener" or "onceOriginalListener" ?

::: devtools/shared/event-emitter.js:131
(Diff revision 1)
> -     * @param function listener
> +                listenersForType.delete(value);
> -     *        The listener to remove.
> -     */
> -    off(event, listener) {
> -      if (!this._eventEmitterListeners) {
> -        return;
> +                return;

Using `break;` would be better here as you never use `return` is any of the other if/else branches.

::: devtools/shared/event-emitter.js:138
(Diff revision 1)
> -      }
> +            }
> -    },
> +          }
> +        }
> +      } else if (length === 2) {
> +        if (events.has(type)) {
> +          events.get(type).clear();

nit: do you think it is any useful as we unreference it in the next line with the `delete`?

::: devtools/shared/event-emitter.js:144
(Diff revision 1)
> +          events.delete(type);
> +        }
> +      } else if (length === 1) {
> +        for (let listeners of events.values()) {
> +          listeners.clear();
> +          events.delete(type);

* `type` will be undefined here
* same as previous block, is it any useful to clear the Sets?
* Wouldn't it be enough to replace the for loop with:
delete target[eventListeners];
or events.clear();

::: devtools/shared/event-emitter.js:171
(Diff revision 1)
> +          resolve(first);
> +        };
> +
> +        handler[originalListener] = listener;
> +        EventEmitter.on(target, type, handler);
> +      });

nit: This is a subtle API behavior comment...
There may be value in adding 
  if (typeof listener !== "function") {
    throw new Error(BAD_LISTENER);
  }
before creating the promise.

With current patch, EventEmitter.on will _throw_ if you pass a wrong listener, whereas EventEmitter.once will return a rejected promise. It may be more coherent to have them both throw?

::: devtools/shared/event-emitter.js:195
(Diff revision 1)
>          }
>  
>          // If listeners were removed during emission, make sure the
>          // event handler we're going to fire wasn't removed.
> -        if (originalListeners === this._eventEmitterListeners.get(event) ||
> -          this._eventEmitterListeners.get(event).some(l => l === listener)) {
> +        if (target[eventListeners].get(type) &&
> +          target[eventListeners].get(type).has(listener)) {

Small optimizations:
let events = target[eventListeners];
if (!events)
  break;
let listeners = events.get(type);
if (listeners && listeners.has(listener)) {
  listener.call(...);
}

::: devtools/shared/event-emitter.js:229
(Diff revision 1)
> +          return listenersForType.size;
> +        }
>        }
>  
> -      let description = describeNthCaller(2);
> +      return 0;
> +    }

I do not see that being used anywhere in /devtools/?
Not EventEmitter.count, not events.count.

::: devtools/shared/tests/unit/test_eventemitter_basic.js:16
(Diff revision 1)
> +  method in target && typeof target[method] === "function";
> +
> +const createNewEmitter = () => new EventEmitter();
> +const decorateObject = () => EventEmitter.decorate({});
> +
> +let getEventEmitter = createNewEmitter;

You should comment about what you do at the end of this file. This is used to run all the tests with createNewEmitter and decorateObject.

::: devtools/shared/tests/unit/test_eventemitter_basic.js:28
(Diff revision 1)
> +    ok(emitter, "We have an event emitter");
> +    ok(hasMethod(emitter, "on") &&
> +        hasMethod(emitter, "off") &&
> +        hasMethod(emitter, "once") &&
> +        !hasMethod(emitter, "decorate") &&
> +        !hasMethod(emitter, "count"),

This is an indentation issue here.

::: devtools/shared/tests/unit/test_eventemitter_basic.js:185
(Diff revision 1)
> +  }
> +};
> +
> +const run = (tests) => (async function () {
> +  for (let name of Object.keys(tests)) {
> +    do_print(name);

It really looks like you are missing SDK-like tests with this helper.
It would be as good with `tests` being an array of unnamed functions and calling do_print with the test descriptions:

const TESTS = [
 function () {
   do_print("test EventEmitter creation");
   ...
 },
 ...
];

And the helper would only be:
for (let test of tests) {
  await test();
}

::: devtools/shared/tests/unit/test_eventemitter_basic.js:187
(Diff revision 1)
> +
> +const run = (tests) => (async function () {
> +  for (let name of Object.keys(tests)) {
> +    do_print(name);
> +    if (tests[name].length === 1) {
> +      await (new Promise(resolve => tests[name](resolve)));

Instead of having this special case here, couldn't you make "test emitting events" and "test Kill it while emitting" be async functions or return promises?

::: devtools/shared/tests/unit/test_eventemitter_static.js:249
(Diff revision 1)
> +      await (new Promise(resolve => tests[name](resolve)));
> +    } else {
> +      await tests[name]();
> +    }
> +  }
> +});

In this test you really don't need such helper:

add_task(function () {
  do_print("test add a listener");
  ...
})

add_task(function () {
  do_print("test that listener is unique per type");
  ...
})

I know it may look more verbose but it is really painful to have various ways to define/read tests.
Attachment #8890303 - Flags: review?(poirot.alex) → review+

Comment 13

a year ago
mozreview-review
Comment on attachment 8890303 [details]
Bug 1381542 - refactored EventEmitter with old SDK event/core capabilities;

https://reviewboard.mozilla.org/r/161430/#review166770

Sorry, I meant to reset the flag, I would like to have another look at this.
Also, you would have to modify the web extension files before proceeding in order to make them use the toolkit event emitter URL.
Attachment #8890303 - Flags: review+
(Assignee)

Comment 14

a year ago
mozreview-review-reply
Comment on attachment 8890303 [details]
Bug 1381542 - refactored EventEmitter with old SDK event/core capabilities;

https://reviewboard.mozilla.org/r/161430/#review166740

Thanks for the review!

About test_eventemitter_basic.html: likely won't work as is, since we have a breaking change, and it's the main reason why we're landing this in parallel to the current e-e, and renaming the current in "old" in the other patch.

About the concerns you mention: that's exactly why we're suggesting to subclass EventEmitter instead of decorate or using `on` / `once` on non event emitter objects; see: https://github.com/devtools-html/snippets-for-removing-the-sdk#events
This new EventEmitter already address almost all issues that Kris mentioned for the old event emitter, and the old sdk event core (the weakmap, the pipe, the `*` special event).
Ideally, I would like to remove at least `decorate`, but both the static (generics) method and the decorate are actually complementary to subclass the event emitter:

1. `decorate` works as mixin: where single inheritance is not enough.
2. generis are useful as building blocks for API. If, for instance, we want have a cleaner "event target" (https://developer.mozilla.org/en-US/docs/Web/API/EventTarget) API that do not emit per sé. Or, if we want to have services that dispatch events, but they're not listen to anything.

So, in the NoSDK we're using subclassing as first choice whenever is possible, but if it's not possible (or it take too much time) we're not doing that. So I'm expecting at the end of the NoSDK project that we're still have the static method for instance, and maybe some usage of `decorate`. My hope is that we will be able at least to reduce the usage of `decorate` using subclassing, but we'll see at the end. And, if that's happen, then I'm more than keen to remove such method – since it means we can avoid the mixin.

> It is unclear what bug is going to land first,
> but note that in bug 1384527, I'm converting event-emitter to a commonjs module only. So it won't be used as a jsm anymore and I'm dropping the whole module boiterplate.

I kept the boilerplate since I thought it would be still used in content, where we do not have access to chrome API.
If you're positive that we do not have any place like that currently in our codebase, I can remove that from this patch too.

Actually, if that is the case, the patch where we're renaming the current event emitter in "old-event-emitter" can be redone to use the toolkit fork instead.

> Using `break;` would be better here as you never use `return` is any of the other if/else branches.

I actually use it at the beginning of the function, but if you prefer I remove from here it's not a problem.

> nit: do you think it is any useful as we unreference it in the next line with the `delete`?

This code was inherited from SDK, and kept since I wanted just to force the removal of any reference even before the `delete`. But if you think it's better without I will remove it, I don't mind.

> * `type` will be undefined here
> * same as previous block, is it any useful to clear the Sets?
> * Wouldn't it be enough to replace the for loop with:
> delete target[eventListeners];
> or events.clear();

- thanks for the catch!
- I will remove it if think there is no harm in that
- Same as above, I can definitely use `events.clear()` is we leave everything to the `gc` I guess, so if you think it's better that way I can do that. But I can't `delete` the `eventListeners` since it would change the shape of the JS object.

> nit: This is a subtle API behavior comment...
> There may be value in adding 
>   if (typeof listener !== "function") {
>     throw new Error(BAD_LISTENER);
>   }
> before creating the promise.
> 
> With current patch, EventEmitter.on will _throw_ if you pass a wrong listener, whereas EventEmitter.once will return a rejected promise. It may be more coherent to have them both throw?

We can't do that, since `once` can be called without actually a listener – this is something the old/toolkit EventEmitter does:
```js
await object.once("data-received");
```
It's a perfectly valid code. If you want, I can `throw` only if a listener is given, and it's a valid type. However, we can't reuse the `BAD_LISTENER` if we keep the predicate function (see https://github.com/devtools-html/snippets-for-removing-the-sdk#experimental-features), we'll have a `BAD_LISTENER_OR_PREDICATE` kind of error.

> I do not see that being used anywhere in /devtools/?
> Not EventEmitter.count, not events.count.

`count` is something it wasn't in `EventEmitter` before, in fact it's a method that is only static and it's not inherited by the mixin or subclasses directly. It was – and it can – be useful mostly for assertion in tests – in fact, I used count for that purpose – without expose the symbol for listeners.

> This is an indentation issue here.

Could you be more specific? I don't see it. The line 24's indentation is 4 spaces, all the following lines (25, 26, 27, 28) are 8 spaces.

> It really looks like you are missing SDK-like tests with this helper.
> It would be as good with `tests` being an array of unnamed functions and calling do_print with the test descriptions:
> 
> const TESTS = [
>  function () {
>    do_print("test EventEmitter creation");
>    ...
>  },
>  ...
> ];
> 
> And the helper would only be:
> for (let test of tests) {
>   await test();
> }

I don't actually miss the "SDK-like" tests, but they were based on CommonJS specifications, and since for our tests (at least the XPCShell ones) is "highly reccomended" to use the assertion methods provided by the CommonJS Unit Testing specification version 1.1, it felt natural to me using the same approach used for CommonJS tests.

Also, I used this patch as working example to show to Patrick because I want to suggest this way as standard way / preferable way to write tests. He's agreed that currently our tests are a mess and we ended up to write any time a small helper, and this approach removes a lot of boilerplate code, and it's minimalistic enough, plus it's connected well to `add_task` functions and any functions / helper that uses async function / promises.

The reason why the `run` function is local is because I didn't want yet to modify in our head.js files before shown. It will be another bug that would take care of that, it's not the scope of this one.

> Instead of having this special case here, couldn't you make "test emitting events" and "test Kill it while emitting" be async functions or return promises?

It's generic exactly for the reason decribed above. I found useful to have sync tests function, async tests function (where the end of the function is the end of the test) and a function where actually you have a `done` function to call. This is cover basically all our needs in all our tests.

> In this test you really don't need such helper:
> 
> add_task(function () {
>   do_print("test add a listener");
>   ...
> })
> 
> add_task(function () {
>   do_print("test that listener is unique per type");
>   ...
> })
> 
> I know it may look more verbose but it is really painful to have various ways to define/read tests.

As above. The idea is having this as new simple way to write tests. Currently our tests are harder to read and maintain compare to a CommonJS unit test approach, and there is no really a standard way to write them.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8890128 - Attachment is obsolete: true
(Assignee)

Comment 17

a year ago
Comment on attachment 8890303 [details]
Bug 1381542 - refactored EventEmitter with old SDK event/core capabilities;

Removing the flag since I pushed the wrong changeset; I'll add it again once I fixed the mess on my local machine.

Also, I had to merge the two patch together since a problem with rebasing; but I kept separate the new event-emitter patch from the renaming the current event emitter.
Attachment #8890303 - Flags: review?(poirot.alex)
Depends on: 1386299

Comment 18

a year ago
mozreview-review
Comment on attachment 8890304 [details]
Bug 1381542 - renamed "devtools/shared/event-emitter" in "devtools/shared/old-event-emitter";

https://reviewboard.mozilla.org/r/161432/#review168816

I already r+ that patch, but I would now suggest landing such change tomorrow, after the merge happened.

Note that you could also land the web extension modification first, in a dedicated revision.

Please ensure having a _rebased_ green try before landing, and do not wait between the try results and landing it. This is the typical patch that bitrot easily.

Do not wait for the review of the new implementation to proceed with this rename.
Attachment #8890304 - Flags: review?(poirot.alex) → review+
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #14)
> Comment on attachment 8890303 [details]
> Bug 1381542 - refactored EventEmitter with old SDK event/core capabilities;
>
> So, in the NoSDK we're using subclassing as first choice whenever is
> possible, but if it's not possible (or it take too much time) we're not
> doing that. So I'm expecting at the end of the NoSDK project that we're
> still have the static method for instance, and maybe some usage of
> `decorate`. My hope is that we will be able at least to reduce the usage of
> `decorate` using subclassing, but we'll see at the end. And, if that's
> happen, then I'm more than keen to remove such method – since it means we
> can avoid the mixin.

Thanks for detailing your refactoring plan,
it help understanding where we are going with this API!

> I kept the boilerplate since I thought it would be still used in content,
> where we do not have access to chrome API.
> If you're positive that we do not have any place like that currently in our
> codebase, I can remove that from this patch too.

I'll try to get bug 1386299 landed tomorrow, after the merge.
So you should be able to rebase your patch soon after and get rid of the compatiblity code.

> Actually, if that is the case, the patch where we're renaming the current
> event emitter in "old-event-emitter" can be redone to use the toolkit fork
> instead.

The toolkit fork only support being loaded as a JSM, Cu.import(.../event-emitter.js).
So it would mean rewriting all require into Cu.import which is something we don't want
as it would break loading tools in a tab.

> > nit: do you think it is any useful as we unreference it in the next line with the `delete`?
> 
> This code was inherited from SDK, and kept since I wanted just to force the
> removal of any reference even before the `delete`. But if you think it's
> better without I will remove it, I don't mind.

Unreferencing the whole Map/Set is enough.
Calling clear looks like a useless operation that we can avoid.
 
> > nit: This is a subtle API behavior comment...
> > There may be value in adding 
> >   if (typeof listener !== "function") {
> >     throw new Error(BAD_LISTENER);
> >   }
> > before creating the promise.
> > 
> > With current patch, EventEmitter.on will _throw_ if you pass a wrong listener, whereas EventEmitter.once will return a rejected promise. It may be more coherent to have them both throw?
> 
> We can't do that, since `once` can be called without actually a listener –
> this is something the old/toolkit EventEmitter does:
> ```js
> await object.once("data-received");
> ```
> It's a perfectly valid code. If you want, I can `throw` only if a listener
> is given, and it's a valid type. However, we can't reuse the `BAD_LISTENER`
> if we keep the predicate function (see
> https://github.com/devtools-html/snippets-for-removing-the-sdk#experimental-
> features), we'll have a `BAD_LISTENER_OR_PREDICATE` kind of error.

Ah, I missed this usecase. Then I'm not sure it is worth adding a dedicated check for that.
As I'm not sure passing a argument that isn't a function nor non-falsy value will happen much.

> 
> > I do not see that being used anywhere in /devtools/?
> > Not EventEmitter.count, not events.count.
> 
> `count` is something it wasn't in `EventEmitter` before, in fact it's a
> method that is only static and it's not inherited by the mixin or subclasses
> directly. It was – and it can – be useful mostly for assertion in tests – in
> fact, I used count for that purpose – without expose the symbol for
> listeners.

I would keep that for a distinct bug with example of usages.
Let's first stick to what we have to support.
Same applies to predicate feature, can you please keep that for a dedicated bug?

> > This is an indentation issue here.
> 
> Could you be more specific? I don't see it. The line 24's indentation is 4
> spaces, all the following lines (25, 26, 27, 28) are 8 spaces.

It should either be 6 for 25..38 as we indent by 2,
or 7 to align with the first argument offset from line 24.

> > It really looks like you are missing SDK-like tests with this helper.
> > It would be as good with `tests` being an array of unnamed functions and calling do_print with the test descriptions:
> > 
> > const TESTS = [
> >  function () {
> >    do_print("test EventEmitter creation");
> >    ...
> >  },
> >  ...
> > ];
> > 
> > And the helper would only be:
> > for (let test of tests) {
> >   await test();
> > }
> 
> I don't actually miss the "SDK-like" tests, but they were based on CommonJS
> specifications, and since for our tests (at least the XPCShell ones) is
> "highly reccomended" to use the assertion methods provided by the CommonJS
> Unit Testing specification version 1.1, it felt natural to me using the same
> approach used for CommonJS tests.
> 
> Also, I used this patch as working example to show to Patrick because I want
> to suggest this way as standard way / preferable way to write tests. He's
> agreed that currently our tests are a mess and we ended up to write any time
> a small helper, and this approach removes a lot of boilerplate code, and
> it's minimalistic enough, plus it's connected well to `add_task` functions
> and any functions / helper that uses async function / promises.

You should focus. The event-emitter refactoring is already quite big.
This doesn't help trying to mix this refactoring with a test refactoring.
Without a plan to actually rewrite the other tests, and ensure someone will spend some time on it,
it will only add yet another test flavor.

Could you do that independently in a dedicated bug where we could drive a discussion specific to tests?
Because I'm pretty confident there is much more to do to really have an impact.
We could sync up with bgrins for example and improve all mozilla-central tests
by contributing directly to mochitest/xpcshell test helpers.
If not, instead of introducing these helpers and copy pasting them in each tests,
we can plan to put that in a shared test file.
But that isn't obvious either as we don't have any file shared by all of our tests.
Target Milestone: Firefox 56 → Firefox 57
quick update: the tree is on Firefox 57 since yesterday, so you can move forward with all this.
bug 1386299 is in autoland, if that sticks you should be able to remove the boilerplate soon.
Duplicate of this bug: 859373
(In reply to Alexandre Poirot [:ochameau] from comment #19)
> > I don't actually miss the "SDK-like" tests, but they were based on CommonJS
> > specifications, and since for our tests (at least the XPCShell ones) is
> > "highly reccomended" to use the assertion methods provided by the CommonJS
> > Unit Testing specification version 1.1, it felt natural to me using the same
> > approach used for CommonJS tests.
> > 
> > Also, I used this patch as working example to show to Patrick because I want
> > to suggest this way as standard way / preferable way to write tests. He's
> > agreed that currently our tests are a mess and we ended up to write any time
> > a small helper, and this approach removes a lot of boilerplate code, and
> > it's minimalistic enough, plus it's connected well to `add_task` functions
> > and any functions / helper that uses async function / promises.
> 
> You should focus. The event-emitter refactoring is already quite big.
> This doesn't help trying to mix this refactoring with a test refactoring.
> Without a plan to actually rewrite the other tests, and ensure someone will
> spend some time on it,
> it will only add yet another test flavor.
> 
> Could you do that independently in a dedicated bug where we could drive a
> discussion specific to tests?
> Because I'm pretty confident there is much more to do to really have an
> impact.
> We could sync up with bgrins for example and improve all mozilla-central
> tests
> by contributing directly to mochitest/xpcshell test helpers.
> If not, instead of introducing these helpers and copy pasting them in each
> tests,
> we can plan to put that in a shared test file.
> But that isn't obvious either as we don't have any file shared by all of our
> tests.

Matteo and I discussed this and agreed that the ideally we would contribute these features into the add_task implementations. We'll spin that off into a separate thread / bug.

In the meantime, this helper seems fine for these two tests, it's not really bigger or more complicated than other one-off test helpers used throughout this directory (none of which are shared and many rely on run_test). And at least for test_eventemitter_basic.js we need some kind of helper/runner thing to wrap add_task since those tests get run twice. I agree we shouldn't add scope to this bug by trying to share it, but I don't have a problem with landing these tests with the helper at the bottom of each one.
I'm not saying we should not have any helper, just that we can have a simplier one, which doesn't make the test look any special.

See comment 12 review:
>  const TESTS = [
>   function () {
>     do_print("test EventEmitter creation");
>     ...
>   },
>   ...
>  ];
>
>  And the helper would only be:
>    for (let test of tests) {
>      await test();
>    }

In current patch, devtools/shared/tests/unit/test_eventemitter_basic.js looks just weird.

> const TESTS = {
>   "test EventEmitter creation"() {
>     ...
>   },
>   "test emitting events"(done) {
>   ...

We keep saying our codebase is not unified. I'm being picky here to help stay unified.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

a year ago
This is the try build for the latest renamed patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0399de8fd69c7295bf85caa1911d8e74844ffb11

The oranges seems intermittent not related to this bug.

Comment 27

a year ago
mozreview-review
Comment on attachment 8890303 [details]
Bug 1381542 - refactored EventEmitter with old SDK event/core capabilities;

https://reviewboard.mozilla.org/r/161430/#review172286

You should keep devtools/shared/tests/mochitest/test_eventemitter_basic.html as it now tests old-event-emitter, and remove it only once we remove old-event-emitter.js.

And some comments I already made:
- I would suggest landing these two patches independently. The first one, renaming to old-event-emitter, could already have been landed.
- keep EventEmitter.count and especially the handler feature for later, with a real debate on its purpose/value. But as it doesn't introduce too much churn in the implementation we can always revisit that later.

::: devtools/shared/event-emitter.js:27
(Diff revision 3)
> +   *
> +   * @param {Object} target
> +   *    Event target object.
> +   * @param {String} type
> +   *    The type of event.
> +   * @param {Function} listener

s/Function/Function|Object/

::: devtools/shared/event-emitter.js:57
(Diff revision 3)
> +   * event `target`.
> +   * @param {Object} target
> +   *    The event target object.
> +   * @param {String} [type]
> +   *    The type of event.
> +   * @param {Function} [listener]

s/Function/Function|Object/

::: devtools/shared/event-emitter.js:90
(Diff revision 3)
> +        // So we iterate all the listeners to check if any of them is a wrapper to
> +        // the `listener` given.
> +        for (let value of listenersForType.values()) {
> +          if (onceOriginalListener in value && (
> +            value[onceOriginalListener] === listener ||
> +            value[onceOriginalListener].when === listener

Update the comment to also mention the "handler" scenario. here you only mention "once".

::: devtools/shared/event-emitter.js:173
(Diff revision 3)
> +      }
> +
> +      // If listeners were removed during emission, make sure the
> +      // event handler we're going to fire wasn't removed.
> +      if (target[eventListeners].get(type) &&
> +          target[eventListeners].get(type).has(listener)) {

As said in previous review, you should optimize this code:
1) Cache `target[eventListeners]`
  let events = target[eventListeners];
  if (!events) {
    break;
  }
2) Cache `target[eventListeners].get(type)`
  let listeners = events.get(type);
  if (listeners && listeners.has(listener)) {

::: devtools/shared/event-emitter.js:252
(Diff revision 3)
> +}
> +
> +module.exports = EventEmitter;
> +
> +const isEventHandler = (listener) =>
> +  listener && handler in listener && typeof listener[handler] === "function";

I think you can safely get rid of `handler in listener` as the `typeof listener[handler] === "function"` should be enough.

::: devtools/shared/tests/unit/test_eventemitter_basic.js:20
(Diff revision 3)
> + *
> + * 1. Plain functions are synchronous tests.
> + * 2. methods with `async` keyword are asynchronous tests.
> + * 3. methods with `done` as argument are asynchronous tests (`done` needs to be called to
> + *    finish the test).
> + */

Thanks for the comment, it makes it much clear.

::: devtools/shared/tests/unit/test_eventemitter_basic.js:31
(Diff revision 3)
> +    ok(emitter, "We have an event emitter");
> +    ok(hasMethod(emitter, "on") &&
> +        hasMethod(emitter, "off") &&
> +        hasMethod(emitter, "once") &&
> +        !hasMethod(emitter, "decorate") &&
> +        !hasMethod(emitter, "count"),

So unless mozreview is buggy, there really is an indentation issue here.
We use 2 space indentations. Line 27 is with 4 spaces, 28 should be at:
 * 6 spaces to just have one additional indentation,
 * 7, so match the first argument of line 27.
Attachment #8890303 - Flags: review?(poirot.alex) → review+

Comment 28

a year ago
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58f678547059
renamed "devtools/shared/event-emitter" in "devtools/shared/old-event-emitter"; r=ochameau
I had to back this out because it was conflicting with incoming changes from mozilla-central: https://hg.mozilla.org/integration/mozilla-inbound/rev/45e8dfa1d016708c00cf1e969addd9cb5d2fe817 

Feel free to rebase and reland whenever.
Flags: needinfo?(zer0)

Comment 30

a year ago
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6544c0f5a489
renamed "devtools/shared/event-emitter" in "devtools/shared/old-event-emitter"; r=ochameau
(Assignee)

Comment 31

a year ago
(In reply to Wes Kocher (:KWierso) from comment #29)
> I had to back this out because it was conflicting with incoming changes from
> mozilla-central:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 45e8dfa1d016708c00cf1e969addd9cb5d2fe817 
> 
> Feel free to rebase and reland whenever.

Thanks, I pushed again; let's cross the fingers. :)
Flags: needinfo?(zer0)

Comment 32

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6544c0f5a489
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED

Comment 34

a year ago
Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4fb5e7082d8
refactored EventEmitter with old SDK event/core capabilities; r=ochameau
(Assignee)

Comment 35

a year ago
mozreview-review-reply
Comment on attachment 8890303 [details]
Bug 1381542 - refactored EventEmitter with old SDK event/core capabilities;

https://reviewboard.mozilla.org/r/161430/#review172286

Since the new xpcshell `test_eventemitter_basic.js` has the same tests of `test_eventemitter_basic.html`, I just create a new xpcshell called `test_old_eventemitter_basic.js` for testing the legacy event emitter; and removed the `test_eventemitter_basic.html`. The only part I changed, of course, is about the emitting arguments.

About the rest:

- I tried to land this before, but apparently MozReview doesn't permit to land separate commits – can trigger a try build per commit, but cannot land a single commit of multiple. So I tried to push manually to inbound, but since I changed my workflow (from hg to git-cinnabar, from git-cinnabar to git-cinnabar + github) it tooks a while, something wasn't working.
- I wasn't sure from your comment if we could land and keep them or not; but since your comments below (where you suggest to add for listener both `Function` and `Object`) I decided to keep for the time being and landing as is. We can avoid to use those features until the proper debate (however, `count` is already used in tests). Sorry but it wasn't clear to me that you wanted also this two removed: it was clear for the "predicate" feature, and I took it off, but not quite for the `handler` and `count`, my apologies.

> Update the comment to also mention the "handler" scenario. here you only mention "once".

Not sure what you mean, since a listener (it doesn't really matter if it's an object or a function) is handled in the same way, here. But thanks for highlighting this line! It was a left-over from the removal of the predicate feature, so it has to be removed too.

> I think you can safely get rid of `handler in listener` as the `typeof listener[handler] === "function"` should be enough.

We're in strict mode, it means if I remove that check I would got a `ReferenceError` warning in the test output (see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Undefined_prop ); I would prefer to avoid that – firefox tests' output are already full of warnings that we don't need, and makes the reading more difficult.

> So unless mozreview is buggy, there really is an indentation issue here.
> We use 2 space indentations. Line 27 is with 4 spaces, 28 should be at:
>  * 6 spaces to just have one additional indentation,
>  * 7, so match the first argument of line 27.

Ah, now I understand what you mean: you're talking about the block from line 28 to line 31.

We indent as this also in other part of our codebase (basically multiply of 2 spaces); if it's annoying you I will fix it but we should probably specify that in our code guidelines if you feel strong about it.

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.