Closed Bug 1252841 Opened 8 years ago Closed 8 years ago

Convert Bluetooth module to |UniquePtr<>|

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
2.6 S10 - 3/25
Tracking Status
firefox48 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Details

Attachments

(3 files, 3 obsolete files)

|nsAutoPtr<>| is going away and being replaced by |UniquePtr<>|. We should implement this change for the Bluetooth module.
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Attachment #8725651 - Flags: review?(brsun)
Comment on attachment 8725654 [details] [diff] [review]
[03] Bug 1252841: Convert Bluetooth module to |UniquePtr<>|

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

::: dom/bluetooth/common/BluetoothService.cpp
@@ +24,4 @@
>  #include "mozilla/dom/bluetooth/BluetoothTypes.h"
>  #include "mozilla/dom/ipc/BlobChild.h"
>  #include "mozilla/dom/ipc/BlobParent.h"
> +#include "nsAutoPtr.h"

Will you remove nsAutoPtr from BluetoothService in another patch/bug? BluetoothService [1] still uses nsAutoPtr.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothService.cpp#306
ni? Thomas for comment 4.
Flags: needinfo?(tzimmermann)
Hi

> Will you remove nsAutoPtr from BluetoothService in another patch/bug?
> BluetoothService [1] still uses nsAutoPtr.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/
> BluetoothService.cpp#306

Sorry for not mentioning this. This is the one remaining use |nsAutoPtr|. I could not find the place where these signal-observer lists are defined, so I expected them to be Gecko-wide code and left this instance of |nsAutoPtr|. I'm happy to remove it as well, if that's possible.

There are also a few instances of |StaticAutoPtr| left, but that's a different type. :)
Flags: needinfo?(tzimmermann)
Comment on attachment 8725651 [details] [diff] [review]
[01] Bug 1252841: Convert HAL daemon socket to |UniquePtr<>|

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

LGTM.
Attachment #8725651 - Flags: review?(brsun) → review+
Comment on attachment 8725653 [details] [diff] [review]
[02] Bug 1252841: Convert Bluetooth daemon interfaces to |UniquePtr<>|

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

LGTM.

Some thoughts which might not directly relate to this bug because this bug is referring to replacing |nsAutoPtr<>| with |UniquePtr<>| only. If |Send()| can take the ownership of |UniquePtr<>| properly, probably we can save many duplicated codes of checking the return value of |Send()| and then explicitly discarding the ownership of |UniquePtr<>|. Letting |UniquePtr<>| handle ownership in its own way would be good for maintenance. Not sure whether it makes sense or not. Just my two cents.
Attachment #8725653 - Flags: review?(brsun) → review+
Not sure what definition you need. Do [1] and [2] help?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothService.h#34
[2] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/Observer.h#39

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #6)
> Sorry for not mentioning this. This is the one remaining use |nsAutoPtr|. I
> could not find the place where these signal-observer lists are defined, so I
> expected them to be Gecko-wide code and left this instance of |nsAutoPtr|.
Flags: needinfo?(tzimmermann)
(In reply to Bruce Sun [:brsun] from comment #8)
> Comment on attachment 8725653 [details] [diff] [review]
> [02] Bug 1252841: Convert Bluetooth daemon interfaces to |UniquePtr<>|
> 
> Review of attachment 8725653 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> Some thoughts which might not directly relate to this bug because this bug
> is referring to replacing |nsAutoPtr<>| with |UniquePtr<>| only. If |Send()|
> can take the ownership of |UniquePtr<>| properly, probably we can save many
> duplicated codes of checking the return value of |Send()| and then
> explicitly discarding the ownership of |UniquePtr<>|. Letting |UniquePtr<>|
> handle ownership in its own way would be good for maintenance. Not sure
> whether it makes sense or not. Just my two cents.

Nathan had such a patch set in bug 1223771. It never landed because of disagreement about the details. Feel free to pick up the work.
Flags: needinfo?(tzimmermann)
Hi

(In reply to Ben Tian [:btian] from comment #9)
> Not sure what definition you need. Do [1] and [2] help?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/
> BluetoothService.h#34
> [2] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/Observer.h#39

Suddenly, things make sense to me. [2] is what I was looking for. And there's also the hash-table typedef at [3]. This hash table internally protects the data with |nsAutoPtr|. [4] So that's where it comes from.

[3] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothService.h#784
[4] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsClassHashtable.h#25
Comment on attachment 8725654 [details] [diff] [review]
[03] Bug 1252841: Convert Bluetooth module to |UniquePtr<>|

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

r=me with comment addressed.

::: dom/bluetooth/common/BluetoothService.cpp
@@ +24,4 @@
>  #include "mozilla/dom/bluetooth/BluetoothTypes.h"
>  #include "mozilla/dom/ipc/BlobChild.h"
>  #include "mozilla/dom/ipc/BlobParent.h"
> +#include "nsAutoPtr.h"

Per comment 11, please leave comment in BluetoothService [1] to remind us to remove nsAutoPtr when hash-table [4] does.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothService.cpp#306
[4] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsClassHashtable.h#25
Attachment #8725654 - Flags: review?(btian) → review+
Changes since v1:

  - added comment about removing include statement
Attachment #8725654 - Attachment is obsolete: true
Attachment #8726669 - Flags: review+
Changes since v1:

  - rebased onto latest m-c
  - convert HID module
Attachment #8725653 - Attachment is obsolete: true
Attachment #8733303 - Flags: review+
Changes since v2:

  - rebased onto latest m-c
  - BlueZ fixes
Attachment #8726669 - Attachment is obsolete: true
Attachment #8733304 - Flags: review+
You need to log in before you can comment on or make changes to this bug.