[Bluetooth] Port bug Bug 1061489 to bluetooth2

RESOLVED FIXED

Status

Firefox OS
Bluetooth
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Comment hidden (empty)
Created attachment 8488550 [details] [diff] [review]
[01] Bug 1065897: Move Bluedroid code out of BluetoothInterface.{cpp,h} (under bluetooth2/)
Attachment #8488550 - Flags: review?(btian)
Created attachment 8488551 [details] [diff] [review]
[02] Bug 1065897: Distribute Bluetooth HAL implementation among multiple files (unter bluetooth2/)
Attachment #8488551 - Flags: review?(btian)
Created attachment 8488552 [details] [diff] [review]
[03] Bug 1065897: Updating CLOBBER
Attachment #8488552 - Flags: review?(btian)
Created attachment 8488554 [details] [diff] [review]
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

4 years ago
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.
Attachment #8488550 - Flags: review?(btian) → review+

Comment 6

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

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

LGTM
Attachment #8488552 - Flags: review?(btian) → review+

Comment 7

4 years ago
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.
Attachment #8488551 - Flags: review?(btian) → review+
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
Created attachment 8493083 [details] [diff] [review]
[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
Attachment #8488551 - Attachment is obsolete: true
Attachment #8493083 - Flags: review+
Created attachment 8493100 [details] [diff] [review]
[01] Bug 1065897: Move Bluedroid code out of BluetoothInterface.{cpp,h} (under bluetooth2/) (v2)

Changes since v1:

  - rebased only
Attachment #8488550 - Attachment is obsolete: true
Attachment #8493100 - Flags: review+
Created attachment 8493101 [details] [diff] [review]
[03] Bug 1065897: Updating CLOBBER (v2)

Changes since v1:

  - rebased only
Attachment #8488552 - Attachment is obsolete: true
Attachment #8493101 - Flags: review+
Ben,

I'm seeing massive problems with my Internet connection today. Could you land these patches for me? Thanks a lot.
Flags: needinfo?(btian)
You need to log in before you can comment on or make changes to this bug.