1382604 - Fix 1 tests failures on devtools/client/scratchpad due the EventEmitter refactoring

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, enhancement, P2)

Description

[Matteo Ferretti [:zer0] [:matteo]]

Failing tests:

devtools/client/scratchpad/test/browser_scratchpad_wrong_window_focus.js

The refactoring is currently only on:

https://github.com/zer0/gecko/tree/event-emitter-1381542

We need to address the test failures before land this patch in m-c.

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

Here the original try build with the failures:

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

Those failures are most likely due the breaking change in how the `EventEmitter` emits event.

Previously, the first argument was the type event:

  myEmitter.on("custom-event", (eventType, message) => { ... });

Now the first argument is the message:

  myEmitter.on("custom-event", (message) => { ... });

In the majority of the scenario the `eventType` is ignored by our code, so we should just remove it from the function's signature.

For more details, see: https://github.com/devtools-html/snippets-for-removing-the-sdk/#events

Comment 2

[Patrick Brosset <:pbro>]

Scratchpad uses the sidebar shared widget, so converting scratchpad to the new event-emitter also requires converting the sidebar to the new event emitter which, in turn, probably requires other changes.

Digging into this a bit more, the inspector does not use this shared sidebar anymore, it uses another component (React I think).
The only other thing that uses the shared sidebar is the console's jsterm, but with the recent rewrite of the console, I'm not even sure this code is used anymore.

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #2)
> The only other thing that uses the shared sidebar is the console's jsterm,
> but with the recent rewrite of the console, I'm not even sure this code is
> used anymore.
Digged into this with Nicolas: the old console still uses jsterm and its sidebar functionality. And the old console is still used in the browser console.
The new console also uses jsterm, but not its sidebar. And we plan to rewrite jsterm in React anyway.

So, as things stand now, when we migrate the browser console to use the new front-end (done in bug 1362023) then this shared sidebar module will only be used in scratchpad anymore, which will make it easier to migrate scratchpad to the new event emitter!

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset <:pbro> from comment #3)
> (In reply to Patrick Brosset <:pbro> from comment #2)
> > The only other thing that uses the shared sidebar is the console's jsterm,
> > but with the recent rewrite of the console, I'm not even sure this code is
> > used anymore.
> Digged into this with Nicolas: the old console still uses jsterm and its
> sidebar functionality. And the old console is still used in the browser
> console.
> The new console also uses jsterm, but not its sidebar. And we plan to
> rewrite jsterm in React anyway.
> 
> So, as things stand now, when we migrate the browser console to use the new
> front-end (done in bug 1362023) then this shared sidebar module will only be
> used in scratchpad anymore, which will make it easier to migrate scratchpad
> to the new event emitter!

Not sure the timeline for this bug but removing the old webconsole code is at least a couple months off. And based on Comment 1 this looks like an easy fix so I wouldn't want to block the event emitter removal on old webconsole code removal.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1382604 - Remove old-event-emitter usage from scratchpad; .

Usage was removed from devtools/client/framework/sidebar as well since it emits
events with the parent component.

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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

TRY seems okay https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac742d0089215f0317b8c2a64662a196e3243816

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8956047 [details]
Bug 1382604 - Remove old-event-emitter usage from scratchpad; .

https://reviewboard.mozilla.org/r/224986/#review230998

Thanks for working on this! :)

::: devtools/client/scratchpad/scratchpad-panel.js:9
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
>  const {Cu} = require("chrome");
> -const EventEmitter = require("devtools/shared/old-event-emitter");
> +const EventEmitter = require("devtools/shared/event-emitter");

Is the toolbox actually listening for the `ready` / `destroyed` events from panels?  I looked for a bit, but I couldn't find it...

Anyway, this isn't something to fix here...  It just seems like potentially dead code.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8956047 [details]
Bug 1382604 - Remove old-event-emitter usage from scratchpad; .

https://reviewboard.mozilla.org/r/224986/#review230998

> Is the toolbox actually listening for the `ready` / `destroyed` events from panels?  I looked for a bit, but I couldn't find it...
> 
> Anyway, this isn't something to fix here...  It just seems like potentially dead code.

thanks for the review, I'll file a bug for that. This looks like something we could not do

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cc7620edb4f
Remove old-event-emitter usage from scratchpad; r=jryans.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1443182

Comment 11

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4cc7620edb4f