Closed Bug 1749032 Opened 3 years ago Closed 3 years ago

Consumers should not call createInstance on nsISound, and instead should be using it as a service

Categories

(Toolkit :: General, task)

Desktop
All
task

Tracking

()

RESOLVED FIXED
98 Branch
Tracking Status
firefox97 --- wontfix
firefox98 --- fixed

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...

Flags: needinfo?(masayuki)

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.

Flags: needinfo?(masayuki)

(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.

Summary: Some consumers are calling createInstance on nsISound while others use it as a service → Consumers should not call createInstance on nsISound, and instead should be using it as a service

(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 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?

I think so.

Type: defect → task
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8f02bfc9e294 fix JS consumers of nsISound to access it as a service, r=mtigley https://hg.mozilla.org/integration/autoland/rev/72b87587ee43 fix C++ consumers of nsISound to access it as a service, r=NeilDeakin
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: