Closed
Bug 1252841
Opened 8 years ago
Closed 8 years ago
Convert Bluetooth module to |UniquePtr<>|
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
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)
1.05 KB,
patch
|
brsun
:
review+
|
Details | Diff | Splinter Review |
82.87 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
21.87 KB,
patch
|
tzimmermann
:
review+
|
Details | Diff | Splinter Review |
|nsAutoPtr<>| is going away and being replaced by |UniquePtr<>|. We should implement this change for the Bluetooth module.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8725653 -
Flags: review?(brsun)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8725654 -
Flags: review?(btian)
Comment 4•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
(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)
Assignee | ||
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Assignee | ||
Comment 13•8 years ago
|
||
Changes since v1: - added comment about removing include statement
Attachment #8725654 -
Attachment is obsolete: true
Attachment #8726669 -
Flags: review+
Assignee | ||
Comment 14•8 years ago
|
||
Changes since v1: - rebased onto latest m-c - convert HID module
Attachment #8725653 -
Attachment is obsolete: true
Attachment #8733303 -
Flags: review+
Assignee | ||
Comment 15•8 years ago
|
||
Changes since v2: - rebased onto latest m-c - BlueZ fixes
Attachment #8726669 -
Attachment is obsolete: true
Attachment #8733304 -
Flags: review+
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/efed456bdb89 https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5719984451 https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c047192403
Assignee | ||
Comment 17•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c7c047192403
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/efed456bdb89 https://hg.mozilla.org/mozilla-central/rev/1f5719984451 https://hg.mozilla.org/mozilla-central/rev/c7c047192403
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.6 S10 - 3/25
You need to log in
before you can comment on or make changes to this bug.
Description
•