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

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gyeh, Assigned: gyeh)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(5 attachments, 10 obsolete attachments)

901 bytes, patch
Details | Diff | Splinter Review
8.23 KB, patch
Details | Diff | Splinter Review
4.56 KB, patch
Details | Diff | Splinter Review
3.97 KB, patch
Details | Diff | Splinter Review
17.88 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
No longer blocks: 807758
(Assignee)

Updated

6 years ago
No longer depends on: 872907
(Assignee)

Updated

6 years ago
Whiteboard: [fixed-in-birch]
(Assignee)

Comment 1

6 years ago
Created attachment 761919 [details] [diff] [review]
Patch 0(v1): BluetoothProfileManagerBase inherits nsIObserver
Assignee: nobody → gyeh
(Assignee)

Comment 2

6 years ago
Created attachment 761920 [details] [diff] [review]
Patch 1(v1):  Remove BluetoothHfpManagerObserver
(Assignee)

Comment 3

6 years ago
Created attachment 761921 [details] [diff] [review]
Patch 2(v1): Remove BluetoothOppManagerObserver
(Assignee)

Comment 4

6 years ago
Created attachment 761922 [details] [diff] [review]
Patch 3(v1): Remove BluetoothA2dpManagerObserver
(Assignee)

Comment 5

6 years ago
Created attachment 761923 [details] [diff] [review]
Patch 4(v1): Change return type of internal functions
(Assignee)

Updated

6 years ago
Attachment #761919 - Flags: review?(mrbkap)
Attachment #761919 - Flags: review?(kyle)
Attachment #761919 - Flags: review?(echou)
(Assignee)

Updated

6 years ago
Attachment #761920 - Flags: review?(mrbkap)
Attachment #761920 - Flags: review?(kyle)
Attachment #761920 - Flags: review?(echou)
(Assignee)

Updated

6 years ago
Attachment #761921 - Flags: review?(mrbkap)
Attachment #761921 - Flags: review?(kyle)
Attachment #761921 - Flags: review?(echou)
(Assignee)

Updated

6 years ago
Attachment #761922 - Flags: review?(mrbkap)
Attachment #761922 - Flags: review?(kyle)
Attachment #761922 - Flags: review?(echou)
(Assignee)

Updated

6 years ago
Attachment #761923 - Flags: review?(echou)
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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 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?
Attachment #761920 - Flags: review?(kyle) → review-
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
Created attachment 762446 [details] [diff] [review]
Patch 1(v2):  Remove BluetoothHfpManagerObserver

Remove extra lines in BluetoothHfpManager::Observe()
Attachment #761920 - Attachment is obsolete: true
Attachment #761920 - Flags: review?(mrbkap)
Attachment #761920 - Flags: review?(echou)
Attachment #762446 - Flags: review?(mrbkap)
Attachment #762446 - Flags: review?(kyle)
Attachment #762446 - Flags: review?(echou)
(Assignee)

Comment 11

6 years ago
Created attachment 762447 [details] [diff] [review]
Patch 4(v2): Change return type of internal functions
Attachment #761923 - Attachment is obsolete: true
Attachment #761923 - Flags: review?(echou)
Attachment #762447 - Flags: review?(echou)
Attachment #761919 - Flags: review?(mrbkap) → review+
Attachment #761921 - Flags: review?(mrbkap) → review+
Attachment #761922 - Flags: review?(mrbkap) → review+
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.
Attachment #762446 - Flags: review?(mrbkap)
(Assignee)

Comment 13

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
Created attachment 763964 [details] [diff] [review]
Patch 1(v3):  Remove BluetoothHfpManagerObserver, r=qdot
Attachment #762446 - Attachment is obsolete: true
Attachment #762446 - Flags: review?(echou)
Attachment #763964 - Flags: review?(mrbkap)
Attachment #763964 - Flags: review?(echou)
(Assignee)

Comment 15

6 years ago
Created attachment 763965 [details] [diff] [review]
Patch 4(v2): Change return type of internal functions
Attachment #762447 - Attachment is obsolete: true
Attachment #762447 - Flags: review?(echou)
Attachment #763965 - Flags: review?(echou)
Comment on attachment 763964 [details] [diff] [review]
Patch 1(v3):  Remove BluetoothHfpManagerObserver, r=qdot

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

Looks good.
Attachment #763964 - Flags: review?(mrbkap) → review+
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
Attachment #763964 - Flags: review?(echou) → review+
Attachment #761919 - Flags: review?(echou) → review+
Attachment #761921 - Flags: review?(echou) → review+
Attachment #761922 - Flags: review?(echou) → review+
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.)
Attachment #763965 - Flags: review?(echou) → review+
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
Created attachment 764625 [details] [diff] [review]
Patch 1: BluetoothProfileManagerBase inherits nsIObserver, r=qdot, r=mrbkap, r=echou
Attachment #761919 - Attachment is obsolete: true
(Assignee)

Comment 21

6 years ago
Created attachment 764627 [details] [diff] [review]
Patch 2: Remove BluetoothHfpManagerObserver, r=qdot, r=mrbkap, r=echou
Attachment #763964 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
Created attachment 764629 [details] [diff] [review]
Patch 3: Remove BluetoothOppManagerObserver, r=qdot, r=mrbkap, r=echou
Attachment #761921 - Attachment is obsolete: true
(Assignee)

Comment 23

6 years ago
Created attachment 764630 [details] [diff] [review]
Patch 4: Remove BluetoothA2dpManagerObserver, r=qdot, r=mrbkap, r=echou
Attachment #761922 - Attachment is obsolete: true
(Assignee)

Comment 24

6 years ago
Created attachment 764631 [details] [diff] [review]
Patch 5: Change return type of internal functions, r=qdot, r=mrbkap, r=echou
Attachment #763965 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
Created attachment 764644 [details] [diff] [review]
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
Attachment #764631 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.