882551 - [b2g-bluetooth] Remove observer class for Bluetooth*Manager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Gina Yeh [:gyeh] [:ginayeh]]

Since the structure of Bluetooth profile managers has been changed, we could remove those observer classes in each profile, i.e., BluetoothHfpManagerObserver, BluetoothOppManagerObserver, BluetoothA2dpManagerObserver.

As reported in bug 798035, Bluetooth*Managers used to inherit both nsISupports and RefCounted<>, so we need to separate the observer part from profile manager itself.

Now that the structure changes, we can just let BluetoothProfileManagerBase inherit nsIObserver and implement virtual function Observe() in each profile manager.

Comment 1

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 0(v1): BluetoothProfileManagerBase inherits nsIObserver

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Remove BluetoothHfpManagerObserver

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 2(v1): Remove BluetoothOppManagerObserver

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 3(v1): Remove BluetoothA2dpManagerObserver

Comment 5

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 4(v1): Change return type of internal functions

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

Separate into several patches.

Patch 0: Let BluetoothProfileManagerBase inherit nsIObserver

Patch 1/2/3: Remove Bluetooth*ManagerObserver class

Patch 4: Change return type of internal functions and also rename observer type for consistency.

Comment 7

[Gina Yeh [:gyeh] [:ginayeh]]

Review requests were send to Eric, Blake, and Kyle.

Please help to review patches if you have time. Or cancel the review request would be fine. Thanks :)

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 761920 [details] [diff] [review]
Patch 1(v1):  Remove BluetoothHfpManagerObserver

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

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +202,5 @@
>  
>    MOZ_ASSERT(false, "BluetoothHfpManager got unexpected topic!");
>    return NS_ERROR_UNEXPECTED;
> +
> +  return NS_OK;

We're never going to hit his NS_OK?

Comment 9

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 761920 [details] [diff] [review]
Patch 1(v1):  Remove BluetoothHfpManagerObserver

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

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +202,5 @@
>  
>    MOZ_ASSERT(false, "BluetoothHfpManager got unexpected topic!");
>    return NS_ERROR_UNEXPECTED;
> +
> +  return NS_OK;

Yes. Let me remove it! Thanks.

Comment 10

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v2): Remove BluetoothHfpManagerObserver

Remove extra lines in BluetoothHfpManager::Observe()

Comment 11

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 4(v2): Change return type of internal functions

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 762446 [details] [diff] [review]
Patch 1(v2):  Remove BluetoothHfpManagerObserver

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

One non-nit.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +340,5 @@
> +  nsresult rv =
> +    obs->AddObserver(this, MOZSETTINGS_CHANGED_ID, false) &&
> +    obs->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false) &&
> +    obs->AddObserver(this, MOBILE_CONNECTION_ICCINFO_CHANGED, false) &&
> +    obs->AddObserver(this, MOBILE_CONNECTION_VOICE_CHANGED, false);

obs->AddObserver returns an nsresult and it isn't valid to use && to combine nsresults. This should be written as:

bool success =
  NS_SUCCEEDED(obj->AddObserver(...)) &&
  NS_SUCCEEDED(obj->AddObserver(...)) &&
  ...;
if (!success) { ...

@@ +392,5 @@
> +  nsresult rv =
> +    obs->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID) &&
> +    obs->RemoveObserver(this, MOZSETTINGS_CHANGED_ID) &&
> +    obs->RemoveObserver(this, MOBILE_CONNECTION_ICCINFO_CHANGED) &&
> +    obs->RemoveObserver(this, MOBILE_CONNECTION_VOICE_CHANGED);

Ditto.

Comment 13

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Blake Kaplan (:mrbkap) from comment #12)
> @@ +340,5 @@
> > +  nsresult rv =
> > +    obs->AddObserver(this, MOZSETTINGS_CHANGED_ID, false) &&
> > +    obs->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false) &&
> > +    obs->AddObserver(this, MOBILE_CONNECTION_ICCINFO_CHANGED, false) &&
> > +    obs->AddObserver(this, MOBILE_CONNECTION_VOICE_CHANGED, false);
> 
> obs->AddObserver returns an nsresult and it isn't valid to use && to combine
> nsresults. This should be written as:
> 
> bool success =
>   NS_SUCCEEDED(obj->AddObserver(...)) &&
>   NS_SUCCEEDED(obj->AddObserver(...)) &&
>   ...;
> if (!success) { ...

Oops! I'll update patch soon. Thanks, mrbkap.

Comment 14

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v3): Remove BluetoothHfpManagerObserver, r=qdot

Comment 15

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 4(v2): Change return type of internal functions

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 763964 [details] [diff] [review]
Patch 1(v3):  Remove BluetoothHfpManagerObserver, r=qdot

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

Looks good.

Comment 17

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 763964 [details] [diff] [review]
Patch 1(v3):  Remove BluetoothHfpManagerObserver, r=qdot

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

r=me with nit addressed.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +385,5 @@
>  
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  NS_ENSURE_TRUE_VOID(obs);
> +
> +  if (NS_FAILED(obs->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) || 

nit: trailing whitespace

Comment 18

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 763965 [details] [diff] [review]
Patch 4(v2): Change return type of internal functions

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

Looks good. Thanks.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +503,4 @@
>  
>    JS::Rooted<JS::Value> val(cx);
> +  if (!JS_ParseJSON(cx, aData.BeginReading(), aData.Length(), &val) ||
> +      !val.isObject()) {

I think it would be better if we can have error log if the condition doesn't match. So how about:

NS_ENSURE_TRUE_VOID(JS_ParseJSON(cx, aData.BeginReading(), aData.Length(), &val)); 
NS_ENSURE_TRUE_VOID(val.isObject());

(Maybe not all condition checks in this function are suitable for this pattern. It's up to you.)

Comment 19

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] (6/25 ~ 28 @ Shanghai, GSMA MAE) from comment #18)
> Comment on attachment 763965 [details] [diff] [review]
> Patch 4(v2): Change return type of internal functions
> 
> Review of attachment 763965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Thanks.
> 
> ::: dom/bluetooth/BluetoothHfpManager.cpp
> @@ +503,4 @@
> >  
> >    JS::Rooted<JS::Value> val(cx);
> > +  if (!JS_ParseJSON(cx, aData.BeginReading(), aData.Length(), &val) ||
> > +      !val.isObject()) {
> 
> I think it would be better if we can have error log if the condition doesn't
> match. So how about:
> 
> NS_ENSURE_TRUE_VOID(JS_ParseJSON(cx, aData.BeginReading(), aData.Length(),
> &val)); 
> NS_ENSURE_TRUE_VOID(val.isObject());
> 
> (Maybe not all condition checks in this function are suitable for this
> pattern. It's up to you.)

That would be great! Let me update the final patch.

Comment 20

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1: BluetoothProfileManagerBase inherits nsIObserver, r=qdot, r=mrbkap, r=echou

Comment 21

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 2: Remove BluetoothHfpManagerObserver, r=qdot, r=mrbkap, r=echou

Comment 22

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 3: Remove BluetoothOppManagerObserver, r=qdot, r=mrbkap, r=echou

Comment 23

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 4: Remove BluetoothA2dpManagerObserver, r=qdot, r=mrbkap, r=echou

Comment 24

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 5: Change return type of internal functions, r=qdot, r=mrbkap, r=echou

Comment 25

[Gina Yeh [:gyeh] [:ginayeh]]

try:
https://tbpl.mozilla.org/?tree=Try&rev=c56ef90d15e8
https://tbpl.mozilla.org/?tree=Try&rev=4df90b2f51d1

Comment 26

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 5: Change return type of internal functions, r=qdot, r=mrbkap, r=echou

try:

https://tbpl.mozilla.org/?tree=Try&rev=c8caf1a8e3ad
https://tbpl.mozilla.org/?tree=Try&rev=58215f7ca933

Comment 27

[Gina Yeh [:gyeh] [:ginayeh]]

http://hg.mozilla.org/projects/birch/rev/1b143479d465
http://hg.mozilla.org/projects/birch/rev/8b0e07d1c8b8
http://hg.mozilla.org/projects/birch/rev/64c24d06d429
http://hg.mozilla.org/projects/birch/rev/84c69071626e
http://hg.mozilla.org/projects/birch/rev/e90c2c614ed9

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e90c2c614ed9
https://hg.mozilla.org/mozilla-central/rev/84c69071626e
https://hg.mozilla.org/mozilla-central/rev/64c24d06d429
https://hg.mozilla.org/mozilla-central/rev/8b0e07d1c8b8
https://hg.mozilla.org/mozilla-central/rev/1b143479d465