Closed
Bug 1382604
Opened 4 years ago
Closed 3 years ago
Fix 1 tests failures on devtools/client/scratchpad due the EventEmitter refactoring
Categories
(DevTools Graveyard :: Scratchpad, enhancement, P2)
DevTools Graveyard
Scratchpad
Tracking
(firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 fixed)
RESOLVED
FIXED
Firefox 60
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.
Reporter | ||
Comment 1•4 years ago
|
||
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
Updated•4 years ago
|
Flags: qe-verify-
Priority: -- → P2
Reporter | ||
Updated•4 years ago
|
Whiteboard: [nosdk]
Comment 2•3 years ago
|
||
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•3 years ago
|
||
(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•3 years ago
|
||
(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.
Updated•3 years ago
|
status-firefox57:
--- → fix-optional
Updated•3 years ago
|
Blocks: dt-polish-debt
Updated•3 years ago
|
No longer blocks: dt-polish-debt
Updated•3 years ago
|
Component: Developer Tools → Developer Tools: Scratchpad
Assignee | ||
Updated•3 years ago
|
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•3 years ago
|
||
TRY seems okay https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac742d0089215f0317b8c2a64662a196e3243816
Comment 7•3 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 8•3 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 10•3 years ago
|
||
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1443182
![]() |
||
Comment 11•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4cc7620edb4f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•3 years ago
|
status-firefox58:
--- → wontfix
status-firefox59:
--- → wontfix
Updated•3 years ago
|
Product: Firefox → DevTools
Updated•1 year ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•