834590 - [b2g-bluetooth] Cache adapter path in BluetoothService

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

After getting default adapter path, we cache its value in BluetoothService, so no need to pass adapter path in many function calls. Getter and setter for mAdapterPath are also implemented in BluetoothService.

For the hash table of distributing signals, key format is changed as follows:
- Key of BluetoothManager: KEY_MANAGER
- Key of BluetoothAdapter: KEY_ADAPTER
- Key of BluetoothDevice: address only. E.g. xx:xx:xx:xx:xx:xx
- Key of Local/Remote agent: KEY_LOCAL_AGENT/KEY_REMOTE_AGENT

Comment 1

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

Attachment: Patch 1(v1): Cache adapter path in BluetoothService

Comment 2

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

Comment on attachment 706937 [details] [diff] [review]
Patch 1(v1): Cache adapter path in BluetoothService

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

The overall idea looks good, some places still need to be revised, though. Please send me another review once the patch is done. Thanks.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +147,5 @@
>  {
>    BluetoothService* bs = BluetoothService::Get();
>    // We can be null on shutdown, where this might happen
>    if (bs) {
> +    bs->UnregisterBluetoothSignalHandler(NS_ConvertUTF8toUTF16(KEY_ADAPTER), this);

nit: NS_LITERAL_STRING(KEY_ADAPTER) would be better.

@@ +254,5 @@
>    }
>  
>    nsRefPtr<BluetoothAdapter> adapter = new BluetoothAdapter(aOwner, aValue);
>  
> +  bs->RegisterBluetoothSignalHandler(NS_ConvertUTF8toUTF16(KEY_ADAPTER), adapter);

nit: NS_LITERAL_STRING(KEY_ADAPTER) would be better.

::: dom/bluetooth/BluetoothCommon.h
@@ +20,5 @@
>  
> +#define KEY_LOCAL_AGENT  "/B2G/bluetooth/agent"
> +#define KEY_REMOTE_AGENT "/B2G/bluetooth/remote_device_agent"
> +#define KEY_MANAGER "/B2G/bluetooth/manager"
> +#define KEY_ADAPTER "/B2G/bluetooth/adapter"

nit: please make them align with each other

::: dom/bluetooth/BluetoothPropertyContainer.cpp
@@ +60,4 @@
>    }
>  
>    nsRefPtr<BluetoothReplyRunnable> task = new BluetoothVoidReplyRunnable(req);
>    

super-nit: extra whitespaces

@@ +65,2 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>    

super-nit: extra whitespaces

::: dom/bluetooth/BluetoothService.cpp
@@ +431,5 @@
>      unused << childActors[index]->SendEnabled(aEnabled);
>    }
>  
>    if (aEnabled) {
> +

super-nit: extra line

@@ +442,3 @@
>      while (iter.HasMore()) {
>        RegisterBluetoothSignalHandler(
> +        NS_ConvertUTF8toUTF16(KEY_MANAGER),

nit: NS_LITERAL_STRING(KEY_MANAGER) would be better.

::: dom/bluetooth/BluetoothService.h
@@ +297,5 @@
>      return mEnabled;
>    }
>  
> +  nsString
> +  GetAdapterPath() const

We usually pass an nsAString& into getter to get the string back.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +174,5 @@
>      nsString errorStr;
>  
> +    BluetoothService* bs = BluetoothService::Get();
> +    NS_ENSURE_TRUE(bs, NS_ERROR_FAILURE);
> +    nsString adapterPath = bs->GetAdapterPath();

If the interface of GetAdapterPath is changed, this needs to be modified, too. (Here and below)

@@ +673,1 @@
>          LOG("Agent already registered, still returning true");

You should use BT_WARNING instead since bug 834551 has been landed.

@@ +2488,5 @@
>    nsString errorStr;
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE_VOID(bs);
> +  nsString adapterPath = bs->GetAdapterPath();

nit: using nsAutoString would be better.

Comment 3

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

Attachment: Patch 1(v2): Cache adapter path in BluetoothService

V2 patch with nits addressed. Also replacing nsString with nsAutoString.

Comment 4

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

Comment on attachment 709551 [details] [diff] [review]
Patch 1(v2): Cache adapter path in BluetoothService

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

Looks good. Due to this change may affect basic Bluetooth functionality, please make sure enough tests have been already done with DEBUG build before checking in. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2489,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Must be called from main thread!");
>  
>    BluetoothValue v;
> +  nsAutoString errorStr;

If the error message may be longer than 64 characters, please use nsString instead of nsAutoString.

@@ +2601,2 @@
>      BluetoothValue v;
> +    nsAutoString errorStr;

nit: please move this declaration into below if-block since there is the only place using this variable.

@@ +2647,5 @@
>      MOZ_ASSERT(!NS_IsMainThread());
>  
>      int channel = GetDeviceServiceChannel(mObjectPath, mServiceUUID, 0x0004);
>      BluetoothValue v;
> +    nsAutoString errorStr;

nit: please move this declaration into below if-block since there is the only place using this variable.

Comment 5

[Mozilla RelEng Bot]

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

Comment 6

[Mozilla RelEng Bot]

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

Comment 7

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

Attachment: Patch 1(v3): Cache adapter path in a static variable in BluetoothDBusService

Since the adapter path is only used in BluetoothDBusService for communicating with dbus and bluez, we can just make it file-scoped and cache its value in a static variable.

Comment 8

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

Attachment: Patch 1(v3): Cache adapter path in a static variable in BluetoothDBusService

Comment 9

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

Comment on attachment 715986 [details] [diff] [review]
Patch 1(v3): Cache adapter path in a static variable in BluetoothDBusService

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

Overall looks good! r=me with nits addressed. I think you may need to rebase on top of current codebase since it's been updated, so please make sure this patch won't burn the tree. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1359,3 @@
>  
> +  BT_LOG("%s: %s, %s, %s", __FUNCTION__,
> +                       dbus_message_get_interface(aMsg),

nit: alignment

@@ +1366,5 @@
>    signalPath = NS_ConvertUTF8toUTF16(dbus_message_get_path(aMsg));
>    signalName = NS_ConvertUTF8toUTF16(dbus_message_get_member(aMsg));
>    nsString errorStr;
>    BluetoothValue v;
>  

I would like to suggest that we should leave comments here to explain why the signal path needs to be re-assigned.

@@ +1717,2 @@
>      BT_WARNING("%s: Can't unregister object path %s for agent!",
> +        __FUNCTION__, KEY_LOCAL_AGENT);

nit: alignment

@@ +1723,2 @@
>      BT_WARNING("%s: Can't unregister object path %s for agent!",
> +        __FUNCTION__, KEY_REMOTE_AGENT);

nit: alignment

Comment 10

[Mozilla RelEng Bot]

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

Comment 11

[Mozilla RelEng Bot]

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

Comment 12

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

Attachment: Final patch, Cache adapter path in BluetoothService, r=echou

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/df77c8581d8d

Comment 14

[Phil Ringnalda (:philor)]

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