990392 - [Bluetooth] Cleanup shared variables in BluetoothDBusService.cpp

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

BluetoothDBusService.cpp lists several shared variables that should be reviewer for thread-safety and possibly cleaned up.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

s/reviewer/reviewed

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 990392: Make |sIsPairing| non-atomic

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 990392: Move sAuthorizedServiceClass to I/O thread

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 990392: Cleanup |GetServiceChannel| in BluetoothDBusService

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 990392: Move |sAdapterPath| to I/O thread

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 990392: Move |sControllerArray| to main thread

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 990392: Fix comments about shared variables in BluetoothDBusService

Comment 8

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

Comment on attachment 8401279 [details] [diff] [review]
[02] Bug 990392: Move sAuthorizedServiceClass to I/O thread

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

Hi Thomas,

This looks fine to me. However I was thinking that maybe we should make sAuthorizedServiceClass a constant which contains BluetoothServiceClass::A2DP and BluetoothServiceClass::HID since sAuthorizedServiceClass won't be changed from the beginning. What do you think?

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

(In reply to Eric Chou [:ericchou] [:echou] from comment #8)
> Comment on attachment 8401279 [details] [diff] [review]
> [02] Bug 990392: Move sAuthorizedServiceClass to I/O thread
> 
> Review of attachment 8401279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Thomas,
> 
> This looks fine to me. However I was thinking that maybe we should make
> sAuthorizedServiceClass a constant which contains
> BluetoothServiceClass::A2DP and BluetoothServiceClass::HID since
> sAuthorizedServiceClass won't be changed from the beginning. What do you
> think?

This definitely makes sense to me. I'll update the patch accordingly.

Comment 10

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

Comment on attachment 8401281 [details] [diff] [review]
[03] Bug 990392: Cleanup |GetServiceChannel| in BluetoothDBusService

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

I can't believe you found this!

Comment 11

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

Comment on attachment 8401282 [details] [diff] [review]
[04] Bug 990392: Move |sAdapterPath| to I/O thread

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

r=me with nits addressed.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +3565,5 @@
>  
>    void Run() MOZ_OVERRIDE
>    {
>      MOZ_ASSERT(!NS_IsMainThread()); // I/O thread
>      MOZ_ASSERT(sDBusConnection);

nit: MOZ_ASSERT(!sAdapterPath.IsEmpty()), please.

@@ +3736,5 @@
>  
>  class SendMetadataTask : public Task
>  {
>  public:
> +  SendMetadataTask(const nsString& aDeviceAddress,

nit: please use const nsAString& instead of const nsString& if there's no special reason.

@@ +3760,5 @@
>  
>    void Run() MOZ_OVERRIDE
>    {
>      MOZ_ASSERT(!NS_IsMainThread()); // I/O thread
>      MOZ_ASSERT(sDBusConnection);

nit: MOZ_ASSERT(!sAdapterPath.IsEmpty()), please.

@@ +3920,5 @@
>  
>    void Run() MOZ_OVERRIDE
>    {
>      MOZ_ASSERT(!NS_IsMainThread()); // I/O thread
>      MOZ_ASSERT(sDBusConnection);

nit: MOZ_ASSERT(!sAdapterPath.IsEmpty()), please.

@@ +4042,5 @@
>  
>    void Run() MOZ_OVERRIDE
>    {
>      MOZ_ASSERT(!NS_IsMainThread()); // I/O thread
>      MOZ_ASSERT(sDBusConnection);

nit: MOZ_ASSERT(!sAdapterPath.IsEmpty()), please.

@@ +4107,5 @@
>  
>    void Run() MOZ_OVERRIDE
>    {
>      MOZ_ASSERT(!NS_IsMainThread()); // I/O thread
>      MOZ_ASSERT(sDBusConnection);

nit: MOZ_ASSERT(!sAdapterPath.IsEmpty()), please.

Comment 12

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

Comment on attachment 8401284 [details] [diff] [review]
[06] Bug 990392: Fix comments about shared variables in BluetoothDBusService

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

Thanks!

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 990392: Move sAuthorizedServiceClass to I/O thread (v2)

Changes since v1:

  - converted sAuthorizedServiceClass to static const array.

That's so much better than before. :)

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 990392: Move |sAdapterPath| to I/O thread (v2)

Changes since v1:

  - fixed parameter of type nsString
  - added extra asserts for !sAdapterPath.IsEmpty()

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 990392: Move |sControllerArray| to main thread (v2)

Changes since v1:

  - only rebased

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 990392: Fix comments about shared variables in BluetoothDBusService (v2)

Changes since v1:

  - rebased
  - added missing period in comment

Comment 17

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

Comment on attachment 8403921 [details] [diff] [review]
[02] Bug 990392: Move sAuthorizedServiceClass to I/O thread (v2)

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

Looks decent!

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Great! Thanks for reviewing.

https://hg.mozilla.org/integration/b2g-inbound/rev/b98b91b3dfe1
https://hg.mozilla.org/integration/b2g-inbound/rev/d928cffbf731
https://hg.mozilla.org/integration/b2g-inbound/rev/d391c6c5228f
https://hg.mozilla.org/integration/b2g-inbound/rev/eee537db59b7
https://hg.mozilla.org/integration/b2g-inbound/rev/dd22e084b2af
https://hg.mozilla.org/integration/b2g-inbound/rev/59aa466f0a65

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=59aa466f0a65

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b98b91b3dfe1
https://hg.mozilla.org/mozilla-central/rev/d928cffbf731
https://hg.mozilla.org/mozilla-central/rev/d391c6c5228f
https://hg.mozilla.org/mozilla-central/rev/eee537db59b7
https://hg.mozilla.org/mozilla-central/rev/dd22e084b2af
https://hg.mozilla.org/mozilla-central/rev/59aa466f0a65