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?

Thomas, I prefer them in one package. Thanks!

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.

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.

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?

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.

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."

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.

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.

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.

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.

Changes since v1: - added enum constants for Socket module service and ops

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.

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.

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.

Thank you https://hg.mozilla.org/integration/b2g-inbound/rev/884b3d5eee63 https://hg.mozilla.org/integration/b2g-inbound/rev/b7d22b4ba3cf https://hg.mozilla.org/integration/b2g-inbound/rev/c91863c880df https://hg.mozilla.org/integration/b2g-inbound/rev/e4cfdcae2f80 https://hg.mozilla.org/integration/b2g-inbound/rev/9b8fec58f964 https://hg.mozilla.org/integration/b2g-inbound/rev/b133c8460ea0 https://treeherder.mozilla.org/ui/#/jobs?repo=b2g-inbound&revision=b133c8460ea0

https://hg.mozilla.org/mozilla-central/rev/884b3d5eee63 https://hg.mozilla.org/mozilla-central/rev/b7d22b4ba3cf https://hg.mozilla.org/mozilla-central/rev/c91863c880df https://hg.mozilla.org/mozilla-central/rev/e4cfdcae2f80 https://hg.mozilla.org/mozilla-central/rev/9b8fec58f964 https://hg.mozilla.org/mozilla-central/rev/b133c8460ea0