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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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.

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: bug.patch

Comment 2

[Andrea Marchesini [:baku]]

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.

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Baku, 
Do you mean I should have two separate static functions. 
one is called SpeakerManagerService* GetOrCreateSpeakerManagerservice()
Another is SpeakerManagerService* GetOrCreateSpeakerManager() ?

Comment 4

[Andrea Marchesini [:baku]]

Yes, 2 static functions:

SpeakerManagerService* GetOrCreateSpeakerManagerservice()
SpeakerManagerService* GetSpeakerManagerservice() <- This can return null. (write a comment)

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: bug.patch

Thanks baku, 
Here is the new one. 
BTW, should I modify the audiochannel?

Comment 6

[Andrea Marchesini [:baku]]

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.

Comment 7

[Andrea Marchesini [:baku]]

> BTW, should I modify the audiochannel?

What do you mean?

Comment 8

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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

Comment 9

[Andrea Marchesini [:baku]]

> > > BTW, should I modify the audiochannel?

If you write a patch, I'll be happy to review it!

Comment 10

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch

check-in patch, carry reviewer, 
try result: 
https://tbpl.mozilla.org/?tree=Try&rev=b040e4a2e417

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ac8948dcc1ef

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac8948dcc1ef