Closed Bug 1042642 Opened 10 years ago Closed 5 years ago

Get rid of eventSource in devtools/shared/client/*

Categories

(DevTools :: Framework, defect, P3)

Product:
DevTools
Component:
Framework
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: ejpbruel, Assigned: yulia)

References

Details

Attachments

(6 files)

Bug 1042642 - Remove eventSource from EnvironmentClient; r=ochameau,jdescottes
(deleted), text/x-phabricator-request
Details
Bug 1042642 - do not use DebuggerClient for functions that should only take ThreadClient; r=ochameau,jdescottes
(deleted), text/x-phabricator-request
Details
Bug 1042642 - use EventEmitter instead of eventSource on the DebuggerClient; r=ochameau,jdescottes
(deleted), text/x-phabricator-request
Details
Bug 1042642 - Use EventEmitter instead of EventSource for the threadClient; r=ochameau,jdescottes
(deleted), text/x-phabricator-request
Details
Bug 1042642 - Stop using the DebuggerClient instance to listen for ThreadClient events in tests; r=ochameau,jdescottes
(deleted), text/x-phabricator-request
Details
Bug 1042642 - Do not pass the threadClient to EnvironmentClient during instantiation in tests; r=jdescottes
(deleted), text/x-phabricator-request
Details
The sendActorEvent API from bug 797621 expects clients to be EventTargets. This means the API is broken for some of the existing clients, since they use eventSource instead, which is not API compatible with EventTarget. This didn't surface before because none of the existing clients in dbg-client.jsm have an non-empty events array. However, adding an event to any of those clients will cause the API to break, because they don't have the required emit function from EventTarget (instead, they have notify, which is the eventSource equivalent). As a quick fix, I'll be filing a patch in bug 1035206 that changes eventSource.notify to eventSource.emit. This will solve the immediate problem, but as a long term solution I suggest that we get rid of the eventSource API completely, and replace it with EventTarget. This will reduce code duplication, but will probably require some consumers of the existing clients to be updated.
Blocks: dbg-client
Mentor: ejpbruel
We should use toolkit/devtools/event-emitter.js instead.
Mentor: ejpbruel
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [nosdk]
The file discussed here has been moved to devtools/shared/client/main, which is already tracked by 1378922. Closing as duplicate.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Whiteboard: [nosdk]
I believe this bug is separate from bug 1378922. The files in question now live in devtools/shared/client: https://searchfox.org/mozilla-central/search?q=eventSource&case=true&regexp=false&path=devtools%2Fshared%2Fclient `eventSource` and all its usages should be replaced by event emitter instead.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Get rid of eventSource in dbg-client.jsm → Get rid of eventSource in devtools/shared/client/*
Product: Firefox → DevTools
Priority: P2 → P3
Assignee: nobody → ystartsev

a couple of notes here:

EnvironmentClient doesn't seem to use eventSource

ThreadClient conversion to protocol.js front needs ThreadClient to be moved to EventEmitter (which is why I picked this up)

DebuggerClient seems to be used in tests in places where ThreadClient should be use, examples: https://searchfox.org/mozilla-central/search?q=client.addListener(%22paused&case=false&regexp=false&path=devtools

After reviewing how the EnvironmentClient is used, it looks like the use of eventSource
might be some cruft from the past. Here is the try run:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=242251058&revision=df4bb52f188f79b8006e8c40401e5af2258493ce

with the exception of whatever is going on Window 7 (which appeares also on central), it looks like
things are working as expected. The environment client will eventually have the event emitter, once
it is moved to being a front.

adding @nchevobbe as a subscriber, as this touches a dependancy of the scratchpad.

Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/72e61b1971a3 Remove eventSource from EnvironmentClient; r=jdescottes
Status: REOPENED → ASSIGNED
Component: Debugger → Framework

turns out we were incorrectly passing the threadClient to the environmentClient while
instantiating in one of the tests. It worked before because the threadClient exposed the API surface
of the debugger-client.

Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/453945168753 do not use DebuggerClient for functions that should only take ThreadClient; r=jdescottes https://hg.mozilla.org/integration/autoland/rev/cef2f6228b9b use EventEmitter instead of eventSource on the DebuggerClient; r=jdescottes https://hg.mozilla.org/integration/autoland/rev/035e95bfd41c Use EventEmitter instead of EventSource for the threadClient; r=jdescottes https://hg.mozilla.org/integration/autoland/rev/84e5d70c6b5e Stop using the DebuggerClient instance to listen for ThreadClient events in tests; r=jdescottes https://hg.mozilla.org/integration/autoland/rev/daf1de2ade3b Do not pass the threadClient to EnvironmentClient during instantiation in tests; r=jdescottes

Does this still need to remain open?

Flags: needinfo?(ystartsev)

no, this is now done.

Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 5 years ago
Flags: needinfo?(ystartsev)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: