1065897 - [Bluetooth] Port bug Bug 1061489 to bluetooth2

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1065897: Move Bluedroid code out of BluetoothInterface.{cpp,h} (under bluetooth2/)

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1065897: Distribute Bluetooth HAL implementation among multiple files (unter bluetooth2/)

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1065897: Updating CLOBBER

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: diffs.patch

FYI, this is the diff between 'bluetooth/' and 'bluetooth2/'. I added the changes of 'bluetooth2/' to the ported patches; except for a few build and version fixes. 'bluetooth/' has way better testing for different compilers and ANDROID_VERSIONs, so it makes sense to keep this code.

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8488550 [details] [diff] [review]
[01] Bug 1065897: Move Bluedroid code out of BluetoothInterface.{cpp,h} (under bluetooth2/)

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

LGTM.

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8488552 [details] [diff] [review]
[03] Bug 1065897: Updating CLOBBER

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

LGTM

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8488551 [details] [diff] [review]
[02] Bug 1065897: Distribute Bluetooth HAL implementation among multiple files (unter bluetooth2/)

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

r=me with comments addressed.

::: dom/bluetooth2/bluedroid/BluetoothHALHelpers.h
@@ +18,5 @@
> +#include "mozilla/ArrayUtils.h"
> +#include "mozilla/dom/bluetooth/BluetoothTypes.h"
> +#include "nsThreadUtils.h"
> +
> +#if MOZ_IS_GCC && MOZ_GCC_VERSION_AT_LEAST(4, 7, 0)

Please rebase on bug 1061481 patch 3/3 changes:
http://hg.mozilla.org/mozilla-central/rev/abc3a737f800

1) moves macro |CONVERT| definition to BluetoothCommon.h
2) revise function |Convert(bt_device_type_t aIn, BluetoothDeviceType& aOut)|
3) replace |mDeviceType| with |mTypeOfDevice| in function |Convert(const bt_property_t& aIn, BluetoothProperty& aOut)|.

@@ +80,5 @@
> +  aOut = sScanMode[aIn];
> +  return NS_OK;
> +}
> +
> +

nit: remove extra newline.

@@ +769,5 @@
> + * instance of this structure as the first argument to |Convert|
> + * to convert an array. The output type has to support the array
> + * subscript operator.
> + */
> +template <typename T>

nit: Please unify the inconsistency between "template <" and "template<" in this file.

@@ +1301,5 @@
> +  {
> +    MOZ_ASSERT(mMethod);
> +  }
> +
> +  template<typename T1,typename T2, typename T3, typename T4>

nit: add space before |typename T2|.

::: dom/bluetooth2/bluedroid/BluetoothSocketHALInterface.cpp
@@ +7,5 @@
> +#include "BluetoothSocketHALInterface.h"
> +#include <errno.h>
> +#include <sys/socket.h>
> +#include <unistd.h>
> +#include "BluetoothHALHelpers.h"

nit: move this including to right after |#include "BluetoothSocketHALInterface.h"|.

@@ +14,5 @@
> +BEGIN_BLUETOOTH_NAMESPACE
> +
> +typedef
> +  BluetoothHALInterfaceRunnable1<BluetoothSocketResultHandler, void,
> +                                       int, int>

nit: fix the indention and all the following.

@@ +77,5 @@
> +}
> +
> +void
> +BluetoothSocketHALInterface::Listen(BluetoothSocketType aType,
> +                                          const nsAString& aServiceName,

nit: fix the indention.

@@ +393,5 @@
> +};
> +
> +void
> +BluetoothSocketHALInterface::Connect(const nsAString& aBdAddr,
> +                                           BluetoothSocketType aType,

Ditto.

@@ +459,5 @@
> +};
> +
> +void
> +BluetoothSocketHALInterface::Accept(int aFd,
> +                                          BluetoothSocketResultHandler* aRes)

Ditto.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Ben,

thanks for the review. No problem, except for

> ::: dom/bluetooth2/bluedroid/BluetoothSocketHALInterface.cpp
> @@ +7,5 @@
> > +#include "BluetoothSocketHALInterface.h"
> > +#include <errno.h>
> > +#include <sys/socket.h>
> > +#include <unistd.h>
> > +#include "BluetoothHALHelpers.h"
> 
> nit: move this including to right after |#include
> "BluetoothSocketHALInterface.h"|.

I think this violates the style guile, as listed in

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1065897: Distribute Bluetooth HAL implementation among multiple files (unter bluetooth2/) (v2)

Changes since v1:

  - integrated changes from bug 1061481
  - cleaned up template declarations
  - cleaned up indention
  - cleaned up new lines

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1065897: Move Bluedroid code out of BluetoothInterface.{cpp,h} (under bluetooth2/) (v2)

Changes since v1:

  - rebased only

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1065897: Updating CLOBBER (v2)

Changes since v1:

  - rebased only

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Ben,

I'm seeing massive problems with my Internet connection today. Could you land these patches for me? Thanks a lot.

Comment 13

[Ben Tian (inactive)]

Landed on b2g-inbound.

https://hg.mozilla.org/integration/b2g-inbound/rev/365cd34d303d
https://hg.mozilla.org/integration/b2g-inbound/rev/3c8dfe1b5616
https://hg.mozilla.org/integration/b2g-inbound/rev/7fd6e16da5c8

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/365cd34d303d
https://hg.mozilla.org/mozilla-central/rev/3c8dfe1b5616
https://hg.mozilla.org/mozilla-central/rev/7fd6e16da5c8