Closed Bug 1382580 Opened 3 years ago Closed 2 years ago

Remove old-event-emitter, and fix old debugger folder

Categories

(DevTools :: Debugger, enhancement, P2)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: zer0, Assigned: yulia)

References

Details

Attachments

(4 files)

Failing tests:

devtools/client/debugger/test/mochitest/browser_dbg_aaa_run_first_leaktest.js
devtools/client/debugger/test/mochitest/browser_dbg_addon-console.js
devtools/client/debugger/test/mochitest/browser_dbg_addon-modules-unpacked.js
devtools/client/debugger/test/mochitest/browser_dbg_addon-modules.js
devtools/client/debugger/test/mochitest/browser_dbg_addon-panels.js
devtools/client/debugger/test/mochitest/browser_dbg_addon-sources.js
devtools/client/debugger/test/mochitest/browser_dbg_addon-workers-dbg-enabled.js
devtools/client/debugger/test/mochitest/browser_dbg_auto-pretty-print-01.js
devtools/client/debugger/test/mochitest/browser_dbg_auto-pretty-print-02.js
devtools/client/debugger/test/mochitest/browser_dbg_auto-pretty-print-03.js
devtools/client/debugger/test/mochitest/browser_dbg_blackboxing-01.js
devtools/client/debugger/test/mochitest/browser_dbg_blackboxing-02.js
devtools/client/debugger/test/mochitest/browser_dbg_blackboxing-03.js
devtools/client/debugger/test/mochitest/browser_dbg_blackboxing-04.js
devtools/client/debugger/test/mochitest/browser_dbg_blackboxing-05.js
devtools/client/debugger/test/mochitest/browser_dbg_blackboxing-06.js
devtools/client/debugger/test/mochitest/browser_dbg_blackboxing-07.js
devtools/client/debugger/test/mochitest/browser_dbg_breadcrumbs-access.js
devtools/client/debugger/test/mochitest/browser_dbg_break-in-anon.js
devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-02.js
devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-03.js
devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-04.js
devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-05.js
devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-06.js
devtools/client/debugger/test/mochitest/browser_dbg_break-on-dom-08.js
devtools/client/debugger/test/mochitest/browser_dbg_break-on-next-console.js
devtools/client/debugger/test/mochitest/browser_dbg_break-on-next.js
devtools/client/debugger/test/mochitest/browser_dbg_break-unselected.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-actual-location.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-actual-location2.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-break-on-last-line-of-script-on-reload.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-button-01.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-button-02.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-condition-thrown-message.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-contextmenu-add.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-contextmenu.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-disabled-reload.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-editor.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-eval.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-highlight.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-new-script.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-other-tabs.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-pane.js
devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-reload.js
devtools/client/debugger/test/mochitest/browser_dbg_bug-896139.js
devtools/client/debugger/test/mochitest/browser_dbg_chrome-create.js
devtools/client/debugger/test/mochitest/browser_dbg_on-pause-highlight.js
devtools/client/debugger/test/mochitest/browser_dbg_optimized-out-vars.js
devtools/client/debugger/test/mochitest/browser_dbg_panel-size.js
devtools/client/debugger/test/mochitest/browser_dbg_pause-exceptions-01.js
devtools/client/debugger/test/mochitest/browser_dbg_pause-exceptions-02.js
devtools/client/debugger/test/mochitest/browser_dbg_pause-no-step.js
devtools/client/debugger/test/mochitest/browser_dbg_pause-resume.js
devtools/client/debugger/test/mochitest/browser_dbg_pause-warning.js
devtools/client/debugger/test/mochitest/browser_dbg_paused-keybindings.js
devtools/client/debugger/test/mochitest/browser_dbg_post-page.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-01.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-02.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-03.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-04.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-05.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-06.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-07.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-08.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-09.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-10.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-11.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-12.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-13.js
devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-on-paused.js
devtools/client/debugger/test/mochitest/browser_dbg_progress-listener-bug.js
devtools/client/debugger/test/mochitest/browser_dbg_promises-allocation-stack.js
devtools/client/debugger/test/mochitest/browser_dbg_promises-fulfillment-stack.js
devtools/client/debugger/test/mochitest/browser_dbg_promises-rejection-stack.js
devtools/client/debugger/test/mochitest/browser_dbg_reload-preferred-script-02.js
devtools/client/debugger/test/mochitest/browser_dbg_reload-preferred-script-03.js
devtools/client/debugger/test/mochitest/browser_dbg_reload-same-script.js
devtools/client/debugger/test/mochitest/browser_dbg_scripts-switching-01.js
devtools/client/debugger/test/mochitest/browser_dbg_scripts-switching-02.js
devtools/client/debugger/test/mochitest/browser_dbg_scripts-switching-03.js
devtools/client/debugger/test/mochitest/browser_dbg_search-autofill-identifier.js
devtools/client/debugger/test/mochitest/browser_dbg_search-basic-01.js
devtools/client/debugger/test/mochitest/browser_dbg_search-basic-02.js
devtools/client/debugger/test/mochitest/browser_dbg_search-basic-03.js
devtools/client/debugger/test/mochitest/browser_dbg_search-basic-04.js
devtools/client/debugger/test/mochitest/browser_dbg_search-global-01.js
devtools/client/debugger/test/mochitest/browser_dbg_search-global-02.js
devtools/client/debugger/test/mochitest/browser_dbg_search-global-03.js
devtools/client/debugger/test/mochitest/browser_dbg_search-global-04.js
devtools/client/debugger/test/mochitest/browser_dbg_search-global-05.js
devtools/client/debugger/test/mochitest/browser_dbg_search-global-06.js
devtools/client/debugger/test/mochitest/browser_dbg_search-popup-jank.js
devtools/client/debugger/test/mochitest/browser_dbg_search-sources-01.js
devtools/client/debugger/test/mochitest/browser_dbg_search-sources-02.js
devtools/client/debugger/test/mochitest/browser_dbg_search-sources-03.js
devtools/client/debugger/test/mochitest/browser_dbg_searchbox-help-popup-01.js
devtools/client/debugger/test/mochitest/browser_dbg_searchbox-help-popup-02.js
devtools/client/debugger/test/mochitest/browser_dbg_source-maps-01.js
devtools/client/debugger/test/mochitest/browser_dbg_source-maps-02.js
devtools/client/debugger/test/mochitest/browser_dbg_source-maps-03.js
devtools/client/debugger/test/mochitest/browser_dbg_source-maps-04.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-bookmarklet.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-cache.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-contextmenu-01.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-contextmenu-02.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-eval-02.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-iframe-reload.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-keybindings.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-labels.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-large.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-sorting.js
devtools/client/debugger/test/mochitest/browser_dbg_sources-webext-contentscript.js
devtools/client/debugger/test/mochitest/browser_dbg_stack-01.js
devtools/client/debugger/test/mochitest/browser_dbg_stack-02.js
devtools/client/debugger/test/mochitest/browser_dbg_stack-03.js
devtools/client/debugger/test/mochitest/browser_dbg_stack-04.js
devtools/client/debugger/test/mochitest/browser_dbg_stack-05.js
devtools/client/debugger/test/mochitest/browser_dbg_stack-06.js
devtools/client/debugger/test/mochitest/browser_dbg_stack-07.js
devtools/client/debugger/test/mochitest/browser_dbg_stack-contextmenu-01.js
devtools/client/debugger/test/mochitest/browser_dbg_stack-contextmenu-02.js
devtools/client/debugger/test/mochitest/browser_dbg_step-out.js
devtools/client/debugger/test/mochitest/browser_dbg_terminate-on-tab-close.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-01.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-02.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-03.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-04.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-05.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-06.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-07.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-08.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-edit-cancel.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-edit-click.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-edit-getset-01.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-edit-getset-02.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-edit-value-01.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-edit-value-02.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-edit-watch.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-filter-01.js
devtools/client/debugger/test/mochitest/browser_dbg_variables-view-filter-02.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 event type:

  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 and edge cases, 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]
Component: Developer Tools → Developer Tools: Debugger
These tests are related to the old debugger, perhaps we should set this bug to WONTFIX?
(In reply to Yulia Startsev [:yulia] from comment #2)
> These tests are related to the old debugger, perhaps we should set this bug
> to WONTFIX?
Agreed. The time it would take to fix them all is probably no worth it considering we're switching completely to the new debugger fairly soon.
How much time do you think it would take to fix them anyway? Just curious.
hard to tell; for a single panel i think it would take an afternoon, maybe a day or two
I dont think we will be fixing this, since removing this code is a higher priority
Since we are not going to fix this, let's move the old-event-emitter.js file over the debugger folder so tests can still be run.
This should be done once there's no other consumers of the old-event-emitter.
Summary: Fix 140 tests failures on devtools/client/debugger due the EventEmitter refactoring → Move old-event-emitter to old debugger folder
Assignee: nobody → ystartsev
Now that Bug 1450982 is resolved, this bug is the last piece of work that needs to happen in mozilla-central.
Could we do the remaining cleanup work here ?

- Alias in webconsole webpack config [1]. It can be removed
- Mention in getting started doc [2]. Should be replaced with the new event emitter path
- old-event-emitter tests [3] & [4]. Can be removed since the new event emitter has unit tests.


If you think it doesn't fit here, we can create a separate bug for that.

[1] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/devtools/client/webconsole/webpack.config.js#99
[2] https://searchfox.org/mozilla-central/source/devtools/docs/getting-started/development-profiles.md#33
[3] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/devtools/shared/tests/unit/test_old_eventemitter_basic.js
[4] https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/devtools/shared/tests/mochitest/test_eventemitter_basic.html
Sure, lets just do that work as part of this -- it seems related to moving the old event emitter anyway.
Assignee: ystartsev → nobody
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
I took a look at the event emitter in the old debugger, and there were very few mentions of it. I managed to clean up the failing tests very quickly. I propose that we remove it from the old debugger, and just delete the old event emitter. Maybe this will make it easier to delete?
Comment on attachment 8972837 [details]
Bug 1382580 - remove old-event-emitter from old debugger;

https://reviewboard.mozilla.org/r/241406/#review247224

Oh, that seems pretty easy, thanks for giving this a shot yulia.
Do you think we should simply delete the old file here, or do it in another bug ? (I do have a patch ready for it, so maybe another bug is fine).

::: commit-message-87bd4:1
(Diff revision 1)
> +Bug 1382580 - remove old-event-emitter from old debugger

could you add "; r=nchevobbe." at the end please ?
Attachment #8972837 - Flags: review+
I am running the tests now to see if everything comes out ok, but so far it looks promising. https://treeherder.mozilla.org/#/jobs?repo=try&revision=86e6408cf002bb975be13771f07ac2c4ca8ef01c

And sure! will make the update
Comment on attachment 8972894 [details]
Bug 1382580 - Replace mention of the old event emitter with the new one in documentation;

https://reviewboard.mozilla.org/r/241438/#review247278
Attachment #8972894 - Flags: review?(nchevobbe) → review+
Comment on attachment 8972895 [details]
Bug 1382580 - Remove old-event-emitter alias from webconsole;

https://reviewboard.mozilla.org/r/241440/#review247282
Attachment #8972895 - Flags: review?(nchevobbe) → review+
Comment on attachment 8972896 [details]
Bug 1382580 - Delete old event emitter;

https://reviewboard.mozilla.org/r/241442/#review247284

Thanks for doing this yulia :)
Farewell old-event-emitter !
Attachment #8972896 - Flags: review?(nchevobbe) → review+
Summary: Move old-event-emitter to old debugger folder → Remove old-event-emitter, and fix old debugger folder
Assignee: nchevobbe → ystartsev
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2fc5c1baf4d3
remove old-event-emitter from old debugger; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/31d43eae6c14
Replace mention of the old event emitter with the new one in documentation; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/398457c41176
Remove old-event-emitter alias from webconsole; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/2c55003986b3
Delete old event emitter;  r=nchevobbe
Duplicate of this bug: 1384546
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.