811569 - [b2g-bluetooth] Bluetooth cleanup

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

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

Attachment: Patch 1(v1): Remove unused files

Comment 2

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

Attachment: Patch 2(v1): Remove redundant events/attributes and add comments in idl

No propertychange event in adapter and device. Also, since agent events like onrequestconfirmation is sent by system message, remove those from adapter.

Comment 3

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

Attachment: Patch 2(v1): Remove redundant events/attributes and add comments in idl

Comment 4

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

Attachment: Patch 2(v1): Remove redundant events/attributes and add comments in idl

Comment 5

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

Attachment: Patch 3(v1): Support array of nsString in SetJsObject

Comment 6

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

Attachment: Patch 4(v1): Cleanup for DOMRequest related checking and error handling

Comment 7

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

Attachment: Patch 4(v1): Cleanup for DOMRequest related checking and do_GetService checking

Comment 8

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

Attachment: Patch 2(v1): Remove redundant events/attributes and add comments in idl

Comment 9

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

Attachment: Patch 5(v1): Remove asynchronous function GetProperty

Comment 10

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

Attachment: Patch 6(v1): No return value for Connect()

Comment 11

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

Attachment: Patch 6(v1): No return value for Connect()

Comment 12

[Doug Turner (:dougt)]

We won't block on this.  If this is safe, please request uplift on the patches.

Comment 13

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

Comment on attachment 681318 [details] [diff] [review]
Patch 1(v1): Remove unused files

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

r=me with nits addressed.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +266,3 @@
>  BluetoothAdapter::Notify(const BluetoothSignal& aData)
>  {
>    InfallibleTArray<BluetoothNamedValue> arr;

Nit: this can be removed, right?

@@ +309,5 @@
>  
> +    NS_ASSERTION(arr.Length() == 1,
> +                 "Got more than one property in a change message!");
> +    BluetoothNamedValue property = arr[0];
> +    SetPropertyByValue(property);

Nit: it seems like we could just use arr[0] directly

::: dom/bluetooth/BluetoothDevice.cpp
@@ +182,3 @@
>                   "PropertyChanged: Invalid value type");
> +    InfallibleTArray<BluetoothNamedValue> arr =
> +      v.get_ArrayOfBluetoothNamedValue();

Question: should we use const InfallibleTArray<BluetoothNamedValue>& to catch returned value of v.get_ArrayOfBluetoothNamedValue()?

Same question in BluetoothAdapter.cpp

@@ +185,5 @@
>  
> +    NS_ASSERTION(arr.Length() == 1,
> +                 "Got more than one property in a change message!");
> +    BluetoothNamedValue property = arr[0];
> +    SetPropertyByValue(property);

Nit: it seems like we could just use arr[0] directly

Comment 14

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

Attachment: Patch 1: Remove unused files, r=echou

Comment 15

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

Comment on attachment 681331 [details] [diff] [review]
Patch 3(v1): Support array of nsString in SetJsObject

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

Looks good, some nits need to be fixed, though.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +243,3 @@
>  
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, nullptr);

Nit: In order to save a memory allocation, I would suggest to call NS_ENSURE_TRUE(bs, nullptr) before creating an adapter object.

::: dom/bluetooth/BluetoothDevice.cpp
@@ +159,3 @@
>  
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, nullptr);

Nit: In order to save a memory allocation, I would suggest to call NS_ENSURE_TRUE(bs, nullptr) before creating an adapter object.

::: dom/bluetooth/BluetoothUtils.cpp
@@ +15,3 @@
>  #include "nsISystemMessagesInternal.h"
>  #include "nsTArray.h"
> +#include "nsTArrayHelpers.h"

Nit: alphabetical order, please.

@@ +23,3 @@
>  mozilla::dom::bluetooth::SetJsObject(JSContext* aContext,
> +                                     const BluetoothValue& aValue,
> +                                     JSObject* aObj)

Curious: why changing the order of the last two parameters? :P

@@ +26,2 @@
>  {
> +  MOZ_ASSERT(aContext && aObj);

check aValue as well?

@@ +29,5 @@
> +  if (aValue.type() == BluetoothValue::TArrayOfnsString) {
> +    nsTArray<nsString> sourceArray = aValue.get_ArrayOfnsString();
> +    if (NS_FAILED(nsTArrayToJSArray(aContext,
> +                                    sourceArray,
> +                                    &aObj))) {

Nit: one line should be enough for this if-statement

@@ +32,5 @@
> +                                    sourceArray,
> +                                    &aObj))) {
> +#ifdef DEBUG
> +      NS_WARNING("Cannot set JS UUIDs object!");
> +#endif

no need to use DEBUG flag on NS_WARNING, here and below.

@@ +38,4 @@
>      }
>  
> +    return NS_OK;
> +  } else  if (aValue.type() == BluetoothValue::TArrayOfBluetoothNamedValue) {

Nit: an additional whitespace between else and if

@@ +81,5 @@
> +
> +#ifdef DEBUG
> +  NS_WARNING("Not handle the type of BluetoothValue!");
> +#endif
> +  return NS_ERROR_FAILURE;

I may add an "else" to handle exception and return NS_OK at bottom, then you don't have to call return NS_OK in each block.

...
} else {
  NS_WARNING("Not handle the type of BluetoothValue!");
  return NS_ERROR_FAILURE;
}

return NS_OK;

::: dom/bluetooth/BluetoothUtils.h
@@ +10,5 @@
>  #include "BluetoothCommon.h"
>  
>  class JSContext;
>  class JSObject;
> +class nsIScriptContext;

It seems that we don't need this forward declaration.

@@ +20,2 @@
>  
> +nsresult

Since this is an internal use function, returning bool should be fine.

Comment 16

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

Attachment: Final patch, Patch 1: Remove unused files, r=echou

Comment 17

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

Attachment: Final patch, Patch 2: Support array of nsString in SetJsObject, r=echou

Comment 18

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

Comment on attachment 681335 [details] [diff] [review]
Patch 4(v1): Cleanup for DOMRequest related checking and do_GetService checking

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

r=me with nit addressed.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +107,5 @@
>        return false;
>      }
>  
> +    rv =
> +      nsTArrayToJSArray(sc->GetNativeContext(), devices, &JsDevices);

Question: why wrapping this? It doesn't seem to exceed 80 chars.

@@ +328,2 @@
>    nsCOMPtr<nsIDOMDOMRequest> req;
> +  PrepareDOMRequest(GetOwner(), getter_AddRefs(req));

This could fail. We should check the returned value. Here and below.

@@ +416,1 @@
>    }    

nit: trailing whitespace

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +895,5 @@
>  
>  void
>  BluetoothHfpManager::OnConnectSuccess()
>  {
> +

super-nit: extra blank line

@@ +944,5 @@
>  
>  void
>  BluetoothHfpManager::OnDisconnect()
>  {
> +

super-nit: extra blank line

@@ +945,5 @@
>  void
>  BluetoothHfpManager::OnDisconnect()
>  {
> +
> +  // When we close a connected socket, then restart listening again and 

super-nit: trailing whitespace

::: dom/bluetooth/BluetoothReplyRunnable.cpp
@@ +39,1 @@
>    

super-nit: additional blanks

@@ +46,5 @@
>  BluetoothReplyRunnable::FireErrorString()
>  {
>    nsCOMPtr<nsIDOMRequestService> rs =
>      do_GetService("@mozilla.org/dom/dom-request-service;1");
> +  NS_ENSURE_TRUE(rs, NS_ERROR_FAILURE);  

nit: trailing whitespace

@@ +51,1 @@
>    

super-nit: additional blanks

Comment 19

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

Comment on attachment 681340 [details] [diff] [review]
Patch 5(v1): Remove asynchronous function GetProperty

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

Looks good! However many parts of code have been removed already, so please remember to revise your patch for current codebase.

::: dom/bluetooth/BluetoothService.cpp
@@ +274,5 @@
>  BluetoothService::Cleanup()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  mBluetoothSignalObserverTable.Clear();

We don't clear the table here since it will be executed in UnregisterAllSignalHandlers()

::: dom/bluetooth/BluetoothService.h
@@ +388,3 @@
>  
>  #ifdef DEBUG
>    bool mLastRequestedEnable;

I think we can remove this variable since no one is using it.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +429,5 @@
>        LOG("%s: Invalid arguments for RequestConfirmation() method", __FUNCTION__);
>        errorStr.AssignLiteral("Invalid arguments for RequestConfirmation() method");
>      } else {
>        parameters.AppendElement(BluetoothNamedValue(
> +                                 NS_LITERAL_STRING("path"),

Nice catch!

Comment 20

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

Attachment: Final patch, Patch 3: Cleanup for DOMRequest related checking and do_GetService checking, r=echou

Comment 21

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

Attachment: Final patch, Patch 4: Remove asynchronous function GetProperty, r=echou

Comment 22

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

Attachment: Final patch, Patch 1: Remove unused files, r=echou

Comment 23

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

Attachment: Final patch, Patch 2: Support array of nsString in SetJsObject, r=echou

Comment 24

[Mozilla RelEng Bot]

Try run for b838c17e9b35 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b838c17e9b35
Results (out of 19 total builds):
    success: 17
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b838c17e9b35

Comment 25

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

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

Comment 26

[Mozilla RelEng Bot]

Try run for 4b75f2e4f6ce is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4b75f2e4f6ce
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4b75f2e4f6ce

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d02aa8c36b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/80e9c08418ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/55ec088a4aba
https://hg.mozilla.org/integration/mozilla-inbound/rev/84706f3c1abe

Comment 28

[Peter Van der Beken [:peterv]]

Comment on attachment 707011 [details] [diff] [review]
Final patch, Patch 2: Support array of nsString in SetJsObject, r=echou

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

::: dom/bluetooth/BluetoothUtils.cpp
@@ +65,5 @@
> +      if (!JS_SetProperty(aContext, aObj,
> +                          NS_ConvertUTF16toUTF8(arr[i].name()).get(),
> +                          &val)) {
> +        NS_WARNING("Failed to set property");
> +        return NS_ERROR_FAILURE;

You're returning a nsresult from a function with a bool return type, I don't think this does what you want it to.

Comment 29

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

Oops! Thanks for reminding, Peter. :)

I'll file another bug to fix it soon.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d02aa8c36b2
https://hg.mozilla.org/mozilla-central/rev/80e9c08418ec
https://hg.mozilla.org/mozilla-central/rev/55ec088a4aba
https://hg.mozilla.org/mozilla-central/rev/84706f3c1abe

Comment 31

[Peter Van der Beken [:peterv]]

(In reply to Gina Yeh from comment #29)
> I'll file another bug to fix it soon.

Could you mention the bug number here?

Comment 32

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

(In reply to Peter Van der Beken [:peterv] from comment #31)
> (In reply to Gina Yeh from comment #29)
> > I'll file another bug to fix it soon.
> 
> Could you mention the bug number here?

I think it's Bug 835740