Closed Bug 1054219 Opened 5 years ago Closed 5 years ago

[B2G][SpeakerManager] Avoid to create unnecessary speakerManager instance on child process

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S3 (29aug)

People

(Reporter: rlin, Assigned: rlin)

Details

Attachments

(1 file, 2 obsolete files)

When parent process try to notify child process through 
http://lxr.mozilla.org/mozilla-central/source/dom/speakermanager/SpeakerManagerService.cpp#124

122   ContentParent::GetAll(children);
123   for (uint32_t i = 0; i < children.Length(); i++) {
124     unused << children[i]->SendSpeakerManagerNotify();
125   }

Then call the 
940         SpeakerManagerService::GetSpeakerManagerService();
If the child process didn't have the SpeakerManagerChild instance before, it would create the new one.
Attached patch bug.patch (obsolete) — Splinter Review
Attachment #8473602 - Flags: review?(amarchesini)
Assignee: nobody → rlin
Component: AudioChannel → General
Comment on attachment 8473602 [details] [diff] [review]
bug.patch

Review of attachment 8473602 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentParent.cpp
@@ +3141,1 @@
>      if (service) {

Use GetOrCreateSpeakerManagerService(). This cannot fail. MOZ_ASSERT(service).

@@ +3155,1 @@
>      if (service) {

read above.

::: dom/speakermanager/SpeakerManagerService.h
@@ +23,5 @@
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIOBSERVER
>  
> +  static SpeakerManagerService* GetSpeakerManagerService(bool aForceCreate);

Implement:

static SpeakerManagerService* GetOrCreateSpeakerManagerservice()

and remove this bool aForceCreate.
Attachment #8473602 - Flags: review?(amarchesini) → review-
Hi Baku, 
Do you mean I should have two separate static functions. 
one is called SpeakerManagerService* GetOrCreateSpeakerManagerservice()
Another is SpeakerManagerService* GetOrCreateSpeakerManager() ?
Flags: needinfo?(amarchesini)
Yes, 2 static functions:

SpeakerManagerService* GetOrCreateSpeakerManagerservice()
SpeakerManagerService* GetSpeakerManagerservice() <- This can return null. (write a comment)
Flags: needinfo?(amarchesini)
Attached patch bug.patch (obsolete) — Splinter Review
Thanks baku, 
Here is the new one. 
BTW, should I modify the audiochannel?
Attachment #8473602 - Attachment is obsolete: true
Attachment #8474947 - Flags: review?(amarchesini)
Comment on attachment 8474947 [details] [diff] [review]
bug.patch

Review of attachment 8474947 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe write a comment in ContentChild.cpp saying why the SpeakerManager can be null and you don't need to create a new instance.

::: dom/speakermanager/SpeakerManager.cpp
@@ +31,1 @@
>    if (service) {

remove this if() and put a MOZ_ASSERT(service) everywhere.

::: dom/speakermanager/SpeakerManagerService.cpp
@@ +40,5 @@
>    }
>  
>    // Create new instance, register, return
>    nsRefPtr<SpeakerManagerService> service = new SpeakerManagerService();
>    NS_ENSURE_TRUE(service, nullptr);

'new' doesn't fail.

::: dom/speakermanager/SpeakerManagerServiceChild.cpp
@@ +32,5 @@
>    }
>  
>    // Create new instance, register, return
>    nsRefPtr<SpeakerManagerServiceChild> service = new SpeakerManagerServiceChild();
>    NS_ENSURE_TRUE(service, nullptr);

'new' doesn't fail.
Attachment #8474947 - Flags: review?(amarchesini) → review+
> BTW, should I modify the audiochannel?

What do you mean?
(In reply to Andrea Marchesini (:baku) from comment #7)
> > BTW, should I modify the audiochannel?
> 
> What do you mean?

This code-snap comes from 
http://lxr.mozilla.org/mozilla-release/source/dom/audiochannel/AudioChannelService.cpp#607
I think this one should also occur on AudioChannel API.
> > > BTW, should I modify the audiochannel?

If you write a patch, I'll be happy to review it!
Attachment #8474947 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ac8948dcc1ef
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
You need to log in before you can comment on or make changes to this bug.