Closed Bug 1382604 Opened 2 years ago Closed 2 years ago

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

Categories

(DevTools :: Scratchpad, enhancement, P2)

enhancement

Tracking

(firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: zer0, Assigned: nchevobbe)

References

Details

Attachments

(1 file)

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.
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
Flags: qe-verify-
Priority: -- → P2
No longer blocks: 1381542
Blocks: 1384546
Whiteboard: [nosdk]
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.
(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!
(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.
Component: Developer Tools → Developer Tools: Scratchpad
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
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.
Attachment #8956047 - Flags: review?(jryans) → review+
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
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4cc7620edb4f
Remove old-event-emitter usage from scratchpad; r=jryans.
https://hg.mozilla.org/mozilla-central/rev/4cc7620edb4f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.