Closed Bug 882551 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gyeh, Assigned: gyeh)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(5 files, 10 obsolete files)

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
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.
No longer blocks: b2g-bluetooth-a2dp
No longer depends on: 872907
Whiteboard: [fixed-in-birch]
Assignee: nobody → gyeh
Attachment #761919 - Flags: review?(mrbkap)
Attachment #761919 - Flags: review?(kyle)
Attachment #761919 - Flags: review?(echou)
Attachment #761920 - Flags: review?(mrbkap)
Attachment #761920 - Flags: review?(kyle)
Attachment #761920 - Flags: review?(echou)
Attachment #761921 - Flags: review?(mrbkap)
Attachment #761921 - Flags: review?(kyle)
Attachment #761921 - Flags: review?(echou)
Attachment #761922 - Flags: review?(mrbkap)
Attachment #761922 - Flags: review?(kyle)
Attachment #761922 - Flags: review?(echou)
Attachment #761923 - Flags: review?(echou)
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.
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 :)
Attachment #761919 - Flags: review?(kyle) → review+
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-
Attachment #761922 - Flags: review?(kyle) → review+
Attachment #761921 - Flags: review?(kyle) → review+
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.
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)
Attachment #761923 - Attachment is obsolete: true
Attachment #761923 - Flags: review?(echou)
Attachment #762447 - Flags: review?(echou)
Attachment #762446 - Flags: review?(kyle) → review+
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)
(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.
Attachment #762446 - Attachment is obsolete: true
Attachment #762446 - Flags: review?(echou)
Attachment #763964 - Flags: review?(mrbkap)
Attachment #763964 - Flags: review?(echou)
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+
(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.
You need to log in before you can comment on or make changes to this bug.