Closed Bug 1042642 Opened 6 years ago Closed 1 year ago

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

Categories

(DevTools :: Framework, defect, P3)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ejpbruel, Assigned: yulia)

References

Details

Attachments

(6 files)

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: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1378922
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
Blocks: 1494796

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
Keywords: leave-open
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: 3 years ago1 year ago
Flags: needinfo?(ystartsev)
Resolution: --- → FIXED
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.