Closed Bug 1091575 Opened 9 years ago Closed 9 years ago

[Bluetooth] Port Bugs 1073548 and 1091577 to bluetooth2

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
2.1 S9 (21Nov)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(6 files, 3 obsolete files)

[01] Bug 1091575: Cleanup Bluetooth's CONVERT macro (under bluetooth2/)
1.20 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
[02] Bug 1091575: Added general-purpose notification runnables for Bluetooth (under bluetooth2/)
19.74 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
[06] Bug 1091575: Add support for Bluetooth daemon backend (under bluetooth2/)
3.12 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
[03] Bug 1091575: Add core interfaces and Setup module for Bluetooth daemon (under bluetooth2/) (v2)
39.38 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
[04] Bug 1091575: Implement Bluetooth Core module for Bluetooth daemon (under bluetooth2/) (v2)
76.35 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
[05] Bug 1091575: Implement Socket module for Bluetooth daemon (under bluetooth2/) (v2)
30.22 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review 
      No description provided.
Ben,

I have the patches from bug 1073548 and their cleanups from bug 1091577 for porting to bluetooth2/. Would you rather review them separately, or combined in one package?
Flags: needinfo?(btian)
Thomas,

I prefer them in one package. Thanks!
Flags: needinfo?(btian)
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Summary: [Bluetooth] Port Bug 1073548 to bluetooth2 → [Bluetooth] Port Bugs 1073548 and 1091577 to bluetooth2
Comment on attachment 8519961 [details] [diff] [review]
[02] Bug 1091575: Added general-purpose notification runnables for Bluetooth (under bluetooth2/)

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

LGTM.
Attachment #8519961 - Flags: review?(btian) → review+
Comment on attachment 8519962 [details] [diff] [review]
[03] Bug 1091575: Add core interfaces and Setup module for Bluetooth daemon (under bluetooth2/)

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

Please see my comment below.

::: dom/bluetooth2/bluedroid/BluetoothDaemonHelpers.h
@@ +42,5 @@
> +
> +//
> +// Conversion
> +//
> +// PDUs can only store primitive data types, such as intergers or

typo: integers

@@ +44,5 @@
> +// Conversion
> +//
> +// PDUs can only store primitive data types, such as intergers or
> +// strings. Gecko often uses more complex data types, such as
> +// enumators or stuctures. Conversion functions convert between

typo: enum'er'ators or st'r'uctures

::: dom/bluetooth2/bluedroid/BluetoothDaemonInterface.cpp
@@ +445,5 @@
> +    NS_WARNING(__func__);
> +    if (!mRegisteredSocketModule) {
> +      mRegisteredSocketModule = true;
> +      // Init, step 4: Register Socket module
> +      mInterface->mProtocol->RegisterModuleCmd(0x02, 0x00, this);

To help understand why pass 0x02 here, please 1) state socket module id=0x02 in comment OR 2) make it an enumeration.

  // Init, step 4: Register Socket module (aId = 0x02)

@@ +470,5 @@
> +      // Init, step 2: Connect notification channel...
> +      if (mNtfChannel->GetConnectionStatus() != SOCKET_CONNECTED) {
> +        nsresult rv = mNtfChannel->ConnectSocket(this, mProtocol);
> +        if (NS_FAILED(rv)) {
> +          OnConnectError(CMD_CHANNEL);

Why treat |mNtfChannel|'s ConnectSocket failure as command channel's connection error?

@@ +482,5 @@
> +    case NTF_CHANNEL: {
> +        nsRefPtr<BluetoothResultHandler> res = mResultHandlerQ.ElementAt(0);
> +        mResultHandlerQ.RemoveElementAt(0);
> +
> +        // Init, step 3: Register Core module

Ditto.

@@ +502,5 @@
> +
> +  switch (aChannel) {
> +    case NTF_CHANNEL:
> +      // Close command channel
> +      mCmdChannel->CloseSocket();

|break| here.

@@ +590,5 @@
> +    MOZ_ASSERT(mInterface->mProtocol);
> +
> +    if (!mUnregisteredCoreModule) {
> +      mUnregisteredCoreModule = true;
> +      // Cleanup, step 2: Unregister Core module

Ditto.

@@ +609,5 @@
> +  sNotificationHandler = nullptr;
> +
> +  mResultHandlerQ.AppendElement(aRes);
> +
> +  // Cleanup, step 1: Unregister Socket module

Ditto.
Attachment #8519962 - Flags: review?(btian)
Comment on attachment 8519960 [details] [diff] [review]
[01] Bug 1091575: Cleanup Bluetooth's CONVERT macro (under bluetooth2/)

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

Why not replace the macro but only wrap another one?
Attachment #8519960 - Flags: review?(btian)
Hi Ben,

Thanks so far.

> To help understand why pass 0x02 here, please 1) state socket module id=0x02
> in comment OR 2) make it an enumeration.

I'll add some enums.

> 
>   // Init, step 4: Register Socket module (aId = 0x02)
> 
> @@ +470,5 @@
> > +      // Init, step 2: Connect notification channel...
> > +      if (mNtfChannel->GetConnectionStatus() != SOCKET_CONNECTED) {
> > +        nsresult rv = mNtfChannel->ConnectSocket(this, mProtocol);
> > +        if (NS_FAILED(rv)) {
> > +          OnConnectError(CMD_CHANNEL);
> 
> Why treat |mNtfChannel|'s ConnectSocket failure as command channel's
> connection error?

This is a bug. It should be 'NTF_CHANNEL' in order to close the command channel.

> @@ +482,5 @@
> > +    case NTF_CHANNEL: {
> > +        nsRefPtr<BluetoothResultHandler> res = mResultHandlerQ.ElementAt(0);
> > +        mResultHandlerQ.RemoveElementAt(0);
> > +
> > +        // Init, step 3: Register Core module
> 
> Ditto.

These 'ditto's all refer to the hexadecimal constants?

> @@ +502,5 @@
> > +
> > +  switch (aChannel) {
> > +    case NTF_CHANNEL:
> > +      // Close command channel
> > +      mCmdChannel->CloseSocket();
> 
> |break| here.

Having no break is correct. We (1) close mCmdChannel (2) fall through (3) cleanup and (4) dispatch the error. And for CMD_CHANNEL, we only clean up and dispatch the error. I'll leave a comment that no break is required.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> > @@ +482,5 @@
> > > +    case NTF_CHANNEL: {
> > > +        nsRefPtr<BluetoothResultHandler> res = mResultHandlerQ.ElementAt(0);
> > > +        mResultHandlerQ.RemoveElementAt(0);
> > > +
> > > +        // Init, step 3: Register Core module
> > 
> > Ditto.
> 
> These 'ditto's all refer to the hexadecimal constants?

Yes.

> > @@ +502,5 @@
> > > +
> > > +  switch (aChannel) {
> > > +    case NTF_CHANNEL:
> > > +      // Close command channel
> > > +      mCmdChannel->CloseSocket();
> > 
> > |break| here.
> 
> Having no break is correct. We (1) close mCmdChannel (2) fall through (3)
> cleanup and (4) dispatch the error. And for CMD_CHANNEL, we only clean up
> and dispatch the error. I'll leave a comment that no break is required.

Got it. Thanks.
(In reply to Ben Tian [:btian] from comment #11)
> Comment on attachment 8519960 [details] [diff] [review]
> [01] Bug 1091575: Cleanup Bluetooth's CONVERT macro (under bluetooth2/)
> 
> Review of attachment 8519960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why not replace the macro but only wrap another one?

For 'indirect documentation'. :)

INIT_ARRAY_AT is used for initializing several arrays with callbacks methods. Using CONVERT there wouldn't make much sense. However in the methods that actually convert values, using CONVERT adds more context to better understand the code. So both macros tell their purpose in the places they're used.
Comment on attachment 8519960 [details] [diff] [review]
[01] Bug 1091575: Cleanup Bluetooth's CONVERT macro (under bluetooth2/)

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

LGTM. Thanks for explanation in comment 14.
Attachment #8519960 - Flags: review+
Comment on attachment 8519963 [details] [diff] [review]
[04] Bug 1091575: Implement Bluetooth Core module for Bluetooth daemon (under bluetooth2/)

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

r=me with comment addressed.

::: dom/bluetooth2/bluedroid/BluetoothDaemonInterface.cpp
@@ +579,5 @@
> +    };
> +
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +    (this->*(HandleOp[!!(aHeader.mOpcode & 0x80)]))(aHeader, aPDU, aUserData);

Leave comment to explain !! here as in bug 1073548 comment 53.

"That's a quick way to map 0x00 to 0 and 0x80 to 1.

'value & 0x80' will return either 0x00 or 0x80; depending on the MSB of |value|. But as an index into the array, we want either 0 or 1. Negating once will turn 0x00 into 1 and 0x80 into 0. Negating a second time will now turn 1 to 0, and 0 to 1."
Attachment #8519963 - Flags: review?(btian) → review+
Comment on attachment 8519965 [details] [diff] [review]
[06] Bug 1091575: Add support for Bluetooth daemon backend (under bluetooth2/)

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

LGTM.
Attachment #8519965 - Flags: review?(btian) → review+
Comment on attachment 8519964 [details] [diff] [review]
[05] Bug 1091575: Implement Socket module for Bluetooth daemon (under bluetooth2/)

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

LGTM.
Attachment #8519964 - Flags: review?(btian) → review+
Changes since v1:

  - fixed typos
  - pass NTF_CHANNEL to |OnConnectError| if opening notification channel fails
  - introduces enum constants for Setup module service and ops
  - clarify cleanup sequence in comment

Some of the hexadecimal values are cleaned up in v2 of patches [04] and [05]. I'll put them up for review again, because they contain significant changes since v1.
Attachment #8519962 - Attachment is obsolete: true
Attachment #8521399 - Flags: review?(btian)
Changes since v1:

  - comment on mapping from notification bit into array
  - added enum constants for Core module service and ops

Maybe you want to take another look.
Attachment #8519963 - Attachment is obsolete: true
Attachment #8521401 - Flags: review?(btian)
Changes since v1:

  - added enum constants for Socket module service and ops
Attachment #8519964 - Attachment is obsolete: true
Attachment #8521402 - Flags: review?(btian)
Comment on attachment 8521399 [details] [diff] [review]
[03] Bug 1091575: Add core interfaces and Setup module for Bluetooth daemon (under bluetooth2/) (v2)

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

LGTM.
Attachment #8521399 - Flags: review?(btian) → review+
Comment on attachment 8521402 [details] [diff] [review]
[05] Bug 1091575: Implement Socket module for Bluetooth daemon (under bluetooth2/) (v2)

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

LGTM.
Attachment #8521402 - Flags: review?(btian) → review+
Comment on attachment 8521401 [details] [diff] [review]
[04] Bug 1091575: Implement Bluetooth Core module for Bluetooth daemon (under bluetooth2/) (v2)

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

LGTM.
Attachment #8521401 - Flags: review?(btian) → review+
You need to log in before you can comment on or make changes to this bug.