Closed
Bug 1061126
Opened 10 years ago
Closed 10 years ago
[Bluetooth] Remove remaining Bluedroid artifacts from Gecko
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S4 (12sep)
People
(Reporter: tzimmermann, Assigned: tzimmermann)
References
Details
Attachments
(4 files, 3 obsolete files)
(deleted),
patch
|
shawnjohnjr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
shawnjohnjr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
shawnjohnjr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
shawnjohnjr
:
review+
|
Details | Diff | Splinter Review |
After the removal of Bluedroid callbacks, there might be remaining references to Bluedroid in the source code of Gecko. These artifacts should be removed and the Gecko source code should be cleaned up.
Assignee | ||
Updated•10 years ago
|
Summary: [Bluetooth] Removed remaining Bluedroid artifacts from Gecko → [Bluetooth] Remove remaining Bluedroid artifacts from Gecko
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8482226 -
Flags: review?(shuang)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8482227 -
Flags: review?(shuang)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8482228 -
Flags: review?(shuang)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8482229 -
Flags: review?(shuang)
Assignee | ||
Comment 5•10 years ago
|
||
Hi Shawn,
After all these complicated internals, here are some simple patches for cleaning up Bluedroid from the Gecko source code. :)
Assignee | ||
Comment 6•10 years ago
|
||
Changes since v1:
- don't handle btrc_interface_t in constructor of |BluetoothAvrcpInterface| if ANDROID_VERSION < 18
This fixes the patch for Flatfish.
Attachment #8482228 -
Attachment is obsolete: true
Attachment #8482228 -
Flags: review?(shuang)
Attachment #8482274 -
Flags: review?(shuang)
Assignee | ||
Comment 7•10 years ago
|
||
Changes since v1:
- rebased onto bug 1061219
Attachment #8482229 -
Attachment is obsolete: true
Attachment #8482229 -
Flags: review?(shuang)
Attachment #8482296 -
Flags: review?(shuang)
Assignee | ||
Updated•10 years ago
|
Attachment #8482274 -
Attachment description: [03] [03] Bug 1061126: Make Bluetooth AVRCP interface generally available (v2) → [03] Bug 1061126: Make Bluetooth AVRCP interface generally available (v2)
Comment on attachment 8482226 [details] [diff] [review]
[01] Bug 1061126: Add AVRCP_UID_SIZE to Bluetooth
Review of attachment 8482226 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bluetooth/BluetoothCommon.h
@@ +364,5 @@
>
> +enum {
> + AVRCP_UID_SIZE = 8
> +};
> +
Is there any special reason to use enum hack instead of const?
Attachment #8482226 -
Flags: review?(shuang) → review+
Comment on attachment 8482227 [details] [diff] [review]
[02] Bug 1061126: Fix constants in Bluedroid HFP manager
Review of attachment 8482227 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1434,5 @@
> BluetoothHfpManager::CnumNotification()
> {
> + static const uint8_t sAddressType[] {
> + [HFP_CALL_ADDRESS_TYPE_UNKNOWN] = 0x81,
> + [HFP_CALL_ADDRESS_TYPE_INTERNATIONAL] = 0x91 // for completeness
This seems to break flatfish build again?
Attachment #8482227 -
Flags: review?(shuang)
Attachment #8482274 -
Flags: review?(shuang) → review+
Attachment #8482296 -
Flags: review?(shuang) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Hi Shawn
> > +enum {
> > + AVRCP_UID_SIZE = 8
> > +};
> > +
>
> Is there any special reason to use enum hack instead of const?
I wouldn't call it a 'hack', as it's usual an commonly used style. There's no actual reason to use this instead of a constant. A constant would have the disadvantage of requiring memory in the binaries data section, while this is built directly into the code.
I'm OK with either. Shall I change it?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> Hi Shawn
>
> > > +enum {
> > > + AVRCP_UID_SIZE = 8
> > > +};
> > > +
> >
> > Is there any special reason to use enum hack instead of const?
>
> I wouldn't call it a 'hack', as it's usual an commonly used style. There's
> no actual reason to use this instead of a constant. A constant would have
> the disadvantage of requiring memory in the binaries data section, while
> this is built directly into the code.
>
> I'm OK with either. Shall I change it?
No. It's ok for me just out of curiosity. Apparently it's not a problem.
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #9)
> Comment on attachment 8482227 [details] [diff] [review]
> [02] Bug 1061126: Fix constants in Bluedroid HFP manager
>
> Review of attachment 8482227 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> @@ +1434,5 @@
> > BluetoothHfpManager::CnumNotification()
> > {
> > + static const uint8_t sAddressType[] {
> > + [HFP_CALL_ADDRESS_TYPE_UNKNOWN] = 0x81,
> > + [HFP_CALL_ADDRESS_TYPE_INTERNATIONAL] = 0x91 // for completeness
>
> This seems to break flatfish build again?
I just tried again, but it worked. Looks like designated initializers are supported for basic primitive types, such as 'uint8_t'. However I found that [03] breaks Flatfish, despite my attempts to prevent this. I'll upload another patch in a moment.
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8482227 [details] [diff] [review]
[02] Bug 1061126: Fix constants in Bluedroid HFP manager
Tested and builds on Flatfish. Adding this patch for review again.
Attachment #8482227 -
Flags: review?(shuang)
Assignee | ||
Comment 14•10 years ago
|
||
Changes since v2:
- fix template specialization |CreateProfileInterface<BluetoothAvrcpInterface>|
- remove ANDROID_VERSION around |DispatchBluetoothAvrcpResult|
More Flatfish fixes. It built for me now.
Attachment #8482274 -
Attachment is obsolete: true
Attachment #8482593 -
Flags: review?(shuang)
Attachment #8482593 -
Flags: review?(shuang) → review+
Attachment #8482227 -
Flags: review?(shuang) → review+
Assignee | ||
Comment 15•10 years ago
|
||
Thanks a lot!
https://hg.mozilla.org/integration/b2g-inbound/rev/518ab7491783
https://hg.mozilla.org/integration/b2g-inbound/rev/ad902f1fd574
https://hg.mozilla.org/integration/b2g-inbound/rev/bae6b0b0a4b5
https://hg.mozilla.org/integration/b2g-inbound/rev/92f2ed449adb
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=92f2ed449adb
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/518ab7491783
https://hg.mozilla.org/mozilla-central/rev/ad902f1fd574
https://hg.mozilla.org/mozilla-central/rev/bae6b0b0a4b5
https://hg.mozilla.org/mozilla-central/rev/92f2ed449adb
Note that this was merged post-uplift, i.e. B2G v2.2.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
You need to log in
before you can comment on or make changes to this bug.
Description
•