Consumers should not call createInstance on nsISound, and instead should be using it as a service
Categories
(Toolkit :: General, task)
Tracking
()
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
Attachments
(2 files)
We should make a decision on how we want this to behave and then be consistent in how we call it.
All the JS consumers use createInstance
and so do some C++ consumers, but other C++ consumers use getService
.
Masayuki, looks like you were one of the last persons to review code to do with nsISound - do you have any intuitions as to what the "right" path is? I assume service, but I'm not sure...
Comment 1•3 years ago
|
||
Yes, it should be used as a service because sound resource/API is not treated as multiple things, and all concrete classes derived from nsISound
are stateless. However, I don't know when the user is in non-main thread.
Assignee | ||
Comment 2•3 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 12/27-1/4) from comment #1)
Yes, it should be used as a service because sound resource/API is not treated as multiple things, and all concrete classes derived from
nsISound
are stateless. However, I don't know when the user is in non-main thread.
All the JS uses will for sure be mainthread-only. The C++ createInstance calls are all in either XUL menu frame code (so mainthread) and typeaheadfind (also mainthread). So this should be straightforward to fix.
It's perhaps worth noting that some of the existing code has comments complaining that it has to hang on to the nsISound
instance to avoid the sound not being played - https://searchfox.org/mozilla-central/rev/d25b03e77bc6bc3d9c86165b83e0b512700b0462/toolkit/components/typeaheadfind/nsTypeAheadFind.h#103-105 . ISTM that if we consistently use getService
, this problem is likely to go away?
AIUI there is no way to enforce using getService, but hopefully if we fix existing callers nobody will cargo-cult the current bad patterns.
Comment 3•3 years ago
|
||
(In reply to :Gijs (he/him) from comment #2)
(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 12/27-1/4) from comment #1)
Yes, it should be used as a service because sound resource/API is not treated as multiple things, and all concrete classes derived from
nsISound
are stateless. However, I don't know when the user is in non-main thread.
It's perhaps worth noting that some of the existing code has comments complaining that it has to hang on to thensISound
instance to avoid the sound not being played - https://searchfox.org/mozilla-central/rev/d25b03e77bc6bc3d9c86165b83e0b512700b0462/toolkit/components/typeaheadfind/nsTypeAheadFind.h#103-105 . ISTM that if we consistently usegetService
, this problem is likely to go away?
I think so.
Updated•3 years ago
|
Assignee | ||
Comment 4•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 5•3 years ago
|
||
Depends on D136481
Comment 7•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8f02bfc9e294
https://hg.mozilla.org/mozilla-central/rev/72b87587ee43
Updated•3 years ago
|
Description
•