Closed Bug 1054242 Opened 5 years ago Closed 5 years ago

[Bluetooth] Port bug Bug 1048915 to bluetooth2

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S4 (12sep)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(6 files, 8 obsolete files)

12.52 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
24.63 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
4.48 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
18.99 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
23.13 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
11.96 KB, patch
ben.tian
: review+
Details | Diff | Splinter Review
No description provided.
Depends on: 1054218, 1048915
Hi Ben,

There have been several large changes un der bluetooth2/ that affect these patches, so I almost had to rewrite patches [03] and [05], compared to bluetooth/. I have been very careful to not break anything, but those patches might require a closer inspection.
Comment on attachment 8476647 [details] [diff] [review]
[01] Bug 1054242: Add infrastructure for Bluetooth notifications (under bluetooth2/)

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

LGTM.
Attachment #8476647 - Flags: review?(btian) → review+
Comment on attachment 8476648 [details] [diff] [review]
[02] Bug 1054242: Add Bluetooth Core notifications (under bluetooth2/)

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

r=me with comment addressed.

::: dom/bluetooth2/bluedroid/BluetoothInterface.cpp
@@ +416,5 @@
> +    CONVERT(BT_DEVICE_DEVTYPE_BREDR, DEVICE_TYPE_BREDR),
> +    CONVERT(BT_DEVICE_DEVTYPE_BLE, DEVICE_TYPE_BLE),
> +    CONVERT(BT_DEVICE_DEVTYPE_DUAL, DEVICE_TYPE_DUAL)
> +  };
> +  if (aIn >= MOZ_ARRAY_LENGTH(sDeviceType)) {

if (!aIn || aIn >= MOZ_ARRAY_LENGTH(sDeviceType))
Attachment #8476648 - Flags: review?(btian) → review+
Attachment #8476649 - Flags: feedback?(joliu)
Attachment #8476651 - Flags: feedback?(joliu)
Comment on attachment 8476650 [details] [diff] [review]
[04] Bug 1054242: Use Bluetooth Core notifications (under bluetooth2/)

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

r=me with nit addressed.

::: dom/bluetooth2/bluedroid/BluetoothInterface.cpp
@@ +3039,5 @@
> +    .adapter_state_changed_cb = BluetoothCallback::AdapterStateChanged,
> +    .adapter_properties_cb = BluetoothCallback::AdapterProperties,
> +    .remote_device_properties_cb = BluetoothCallback::RemoteDeviceProperties,
> +    .device_found_cb = BluetoothCallback::DeviceFound,
> +    .discovery_state_changed_cb= BluetoothCallback::DiscoveryStateChanged,

nit: space before '='
Attachment #8476650 - Flags: review?(btian) → review+
Reminding myself to remove the designated initializers from the callback structure because it breaks Flatfish.
Comment on attachment 8476649 [details] [diff] [review]
[03] Bug 1054242: Implement Bluetooth Core notifications (under bluetooth2/)

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

Please see my feedbacks below.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1808,5 @@
> +  if (isBtEnabled &&
> +      NS_FAILED(NS_DispatchToMainThread(new SetupAfterEnabledTask()))) {
> +    BT_WARNING("Failed to dispatch to main thread!");
> +    return;
> +  }

There was a AdapterStateChangedCallbackTask dispatched to main thread.
Why is it removed?
http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp#322

@@ +1834,5 @@
> +
> +    if (p.mType == PROPERTY_BDADDR) {
> +      sAdapterBdAddress = p.mString;
> +      propertyValue = sAdapterBdAddress;
> +      BT_APPEND_NAMED_VALUE(props, "Address", propertyValue);

Is propertyValue assignment necessary?
How about we use
BT_APPEND_NAMED_VALUE(props, "Address", sAdapterBdAddress);
directly?
Same thing applies to other properties, then we can remove propertyValue variable.

@@ +1839,5 @@
> +
> +    } else if (p.mType == PROPERTY_BDNAME) {
> +      sAdapterBdName = p.mString;
> +      propertyValue = sAdapterBdName;
> +      BT_APPEND_NAMED_VALUE(props, "Name", propertyValue);

DITTO.

@@ +1850,5 @@
> +      } else {
> +        propertyValue = sAdapterDiscoverable = false;
> +      }
> +
> +      BT_APPEND_NAMED_VALUE(props, "Discoverable", propertyValue);

DITTO.

@@ +1903,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  InfallibleTArray<BluetoothNamedValue> props;
> +
> +  BT_APPEND_NAMED_VALUE(props, "Address", BluetoothValue(nsString(aBdAddr)));

This macro is different from the one under dom/bluetooth.
Use BT_APPEND_NAMED_VALUE(props, "Address", nsString(aBdAddr)) instead;
http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothCommon.h#70

@@ +1947,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  BluetoothValue propertyValue;
> +  InfallibleTArray<BluetoothNamedValue> propertiesArray;

In this file, we use both props and propertiesArray, can we choose just one?

@@ +1962,5 @@
> +
> +    } else if (p.mType == PROPERTY_CLASS_OF_DEVICE) {
> +      uint32_t cod = p.mUint32;
> +      BT_APPEND_NAMED_VALUE(propertiesArray, "Cod", cod);
> +

Why is BT_PROPERTY_UUIDS case removed?

@@ +1996,5 @@
> +                                                  uint32_t aCod)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  InfallibleTArray<BluetoothNamedValue> propertiesArray;

DITTO.

@@ +2005,5 @@
> +                        NS_LITERAL_STRING(PAIRING_REQ_TYPE_ENTERPINCODE));
> +
> +  BluetoothSignal signal(NS_LITERAL_STRING("PairingRequest"),
> +                         NS_LITERAL_STRING(KEY_ADAPTER),
> +                         BluetoothValue(propertiesArray));

insert a newline.

@@ +2021,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  InfallibleTArray<BluetoothNamedValue> props;
> +

remove this newline.
Comment on attachment 8476651 [details] [diff] [review]
[05] Bug 1054242: Integrate helper runnables into notification methods (under bluetooth2/)

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

I think AdapterStateChangedCallbackTask should also be integrated into AdapterStateChangedNotification, no?

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1880,5 @@
> +                                   NS_LITERAL_STRING(KEY_ADAPTER),
> +                                   BluetoothValue(props)));
> +
> +  // Send reply for SetProperty
> +

remove this newline.

@@ +1938,5 @@
>    }
>  
> +  // Update to registered BluetoothDevice objects
> +  BluetoothSignal signal(NS_LITERAL_STRING("PropertyChanged"),
> +                         nsString(aBdAddr), props);

Any reason to delay the signal distribution after DispatchBluetoothReply?
How about we distribute the signal right at this line?
It's more consistent with other cases, we always distribute some signals first to notify changes to adapter/device objects, then resolve the promise at last.
And we don't need to write multiple |DistributeSignal(signal)|.

@@ +1961,5 @@
> +  }
> +
> +  // Use address as the index
> +  sRemoteDevicesPack.AppendElement(BluetoothNamedValue(nsString(aBdAddr),
> +                                   props));

Personally I would prefer,
sRemoteDevicesPack.AppendElement(
  BluetoothNamedValue(nsString(aBdAddr), props));
Just a suggestion though.

@@ +2007,5 @@
>    }
>  
> +  DistributeSignal(BluetoothSignal(NS_LITERAL_STRING("DeviceFound"),
> +                                   NS_LITERAL_STRING(KEY_ADAPTER),
> +                                   BluetoothValue(propertiesArray)));

As I mentioned, please use either props or propertiesArray.

@@ +2119,5 @@
>    BT_APPEND_NAMED_VALUE(props, "Paired", bonded);
>  
> +  DistributeSignal(BluetoothSignal(NS_LITERAL_STRING("PropertyChanged"),
> +                                   NS_LITERAL_STRING(KEY_ADAPTER),
> +                                   BluetoothValue(props)));

This signal is aimed to be distributed to the corresponding device objects.
The target should be nsString(aRemoteBdAddr) instead of KEY_ADAPTER.
Hi Jocelyn,

Thanks for pointing out all these bugs.

> ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +1808,5 @@
> > +  if (isBtEnabled &&
> > +      NS_FAILED(NS_DispatchToMainThread(new SetupAfterEnabledTask()))) {
> > +    BT_WARNING("Failed to dispatch to main thread!");
> > +    return;
> > +  }
> 
> There was a AdapterStateChangedCallbackTask dispatched to main thread.
> Why is it removed?

I think this got lost. Fixed now.
 
> @@ +1834,5 @@
> > +
> > +    if (p.mType == PROPERTY_BDADDR) {
> > +      sAdapterBdAddress = p.mString;
> > +      propertyValue = sAdapterBdAddress;
> > +      BT_APPEND_NAMED_VALUE(props, "Address", propertyValue);
> 
> Is propertyValue assignment necessary?
> How about we use
> BT_APPEND_NAMED_VALUE(props, "Address", sAdapterBdAddress);
> directly?
> Same thing applies to other properties, then we can remove propertyValue
> variable.

Fixed, but I usually try to minimize the changes per patch. This cleanup is something that can be done later.
 
> @@ +1903,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  InfallibleTArray<BluetoothNamedValue> props;
> > +
> > +  BT_APPEND_NAMED_VALUE(props, "Address", BluetoothValue(nsString(aBdAddr)));
> 
> This macro is different from the one under dom/bluetooth.
> Use BT_APPEND_NAMED_VALUE(props, "Address", nsString(aBdAddr)) instead;
> http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothCommon.
> h#70

Fixed.

> @@ +1947,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  BluetoothValue propertyValue;
> > +  InfallibleTArray<BluetoothNamedValue> propertiesArray;
> 
> In this file, we use both props and propertiesArray, can we choose just one?

I renamed all instances to 'propertiesArray'. 'Props' is also American slang IIRC.

> @@ +1962,5 @@
> > +
> > +    } else if (p.mType == PROPERTY_CLASS_OF_DEVICE) {
> > +      uint32_t cod = p.mUint32;
> > +      BT_APPEND_NAMED_VALUE(propertiesArray, "Cod", cod);
> > +
> 
> Why is BT_PROPERTY_UUIDS case removed?

Also got lost. Fixed now.
> @@ +1938,5 @@
> >    }
> >  
> > +  // Update to registered BluetoothDevice objects
> > +  BluetoothSignal signal(NS_LITERAL_STRING("PropertyChanged"),
> > +                         nsString(aBdAddr), props);
> 
> Any reason to delay the signal distribution after DispatchBluetoothReply?
> How about we distribute the signal right at this line?

In the original code, we first complete |RemoteDevicePropertiesCallbackTask::Run| before distributing the signal in the dispatched runnable. If we distribute the signal here immediately, we'd revert the order of operations. The code would be much nicer, but I don't know if this has bad side effects, so I kept the original order.

> @@ +2119,5 @@
> >    BT_APPEND_NAMED_VALUE(props, "Paired", bonded);
> >  
> > +  DistributeSignal(BluetoothSignal(NS_LITERAL_STRING("PropertyChanged"),
> > +                                   NS_LITERAL_STRING(KEY_ADAPTER),
> > +                                   BluetoothValue(props)));
> 
> This signal is aimed to be distributed to the corresponding device objects.
> The target should be nsString(aRemoteBdAddr) instead of KEY_ADAPTER.

Fixed.
Changes since v1:

  - check for !mIn in conversion function
Attachment #8476648 - Attachment is obsolete: true
Attachment #8479931 - Flags: review+
Changes since v2:

  - add lost dispatch of |AdapterStateChangedCallbackTask|
  - remove some local variables from notification handlers
  - remove copy constructor from call to |BT_APPEND_NAMED_VALUE|
  - rename all property arrays to |propertiesArray|
  - add lost PROPERTIES_UUID branches
  - style fixes
Attachment #8476649 - Attachment is obsolete: true
Attachment #8476649 - Flags: review?(btian)
Attachment #8476649 - Flags: feedback?(joliu)
Attachment #8479934 - Flags: review?(btian)
Attachment #8479934 - Flags: feedback?(joliu)
Changes since v1:

  - removed designated initializers from callback structure
  - protect |LeTestMode| callback by ANDROID_VERSION
Attachment #8476650 - Attachment is obsolete: true
Attachment #8479937 - Flags: review?(btian)
Changes since v1:

  - renamed all property arrays to 'propertiesArray'
  - distribute 'Paired' signal to device objects
  - style fixes
Attachment #8476651 - Attachment is obsolete: true
Attachment #8476651 - Flags: review?(btian)
Attachment #8476651 - Flags: feedback?(joliu)
Attachment #8479939 - Flags: review?(btian)
Attachment #8479939 - Flags: feedback?(joliu)
Comment on attachment 8479937 [details] [diff] [review]
[04] Bug 1054242: Use Bluetooth Core notifications (under bluetooth2/) (v2)

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

LGTM
Attachment #8479937 - Flags: review?(btian) → review+
Comment on attachment 8479934 [details] [diff] [review]
[03] Bug 1054242: Implement Bluetooth Core notifications (under bluetooth2/) (v2)

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

LGTM.
Thanks for your effort, Thomas!
Attachment #8479934 - Flags: feedback?(joliu) → feedback+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #15)
> > @@ +1938,5 @@
> > >    }
> > >  
> > > +  // Update to registered BluetoothDevice objects
> > > +  BluetoothSignal signal(NS_LITERAL_STRING("PropertyChanged"),
> > > +                         nsString(aBdAddr), props);
> > 
> > Any reason to delay the signal distribution after DispatchBluetoothReply?
> > How about we distribute the signal right at this line?
> 
> In the original code, we first complete
> |RemoteDevicePropertiesCallbackTask::Run| before distributing the signal in
> the dispatched runnable. If we distribute the signal here immediately, we'd
> revert the order of operations. The code would be much nicer, but I don't
> know if this has bad side effects, so I kept the original order.
> 
Yes, it's the current order though distributing signal first makes more sense here.
I think it's a minor bug in our implementation and not related to your patch.
Hence I will open a bug to change the order in both bluetooth and bluetooth2 later.

> > @@ +2119,5 @@
> > >    BT_APPEND_NAMED_VALUE(props, "Paired", bonded);
> > >  
> > > +  DistributeSignal(BluetoothSignal(NS_LITERAL_STRING("PropertyChanged"),
> > > +                                   NS_LITERAL_STRING(KEY_ADAPTER),
> > > +                                   BluetoothValue(props)));
> > 
> > This signal is aimed to be distributed to the corresponding device objects.
> > The target should be nsString(aRemoteBdAddr) instead of KEY_ADAPTER.
> 
> Fixed.
Comment on attachment 8479939 [details] [diff] [review]
[05] Bug 1054242: Integrate helper runnables into notification methods (under bluetooth2/) (v2)

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

LGTM, thanks!
Attachment #8479939 - Flags: feedback?(joliu) → feedback+
Comment on attachment 8479934 [details] [diff] [review]
[03] Bug 1054242: Implement Bluetooth Core notifications (under bluetooth2/) (v2)

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

r=me with comment addressed.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1789,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  BT_LOGR("BT_STATE: %d", aState);
> +
> +  bool isBtEnabled = (aState == true);

Update |sAdapterEnabled| as [1] since [2] requires the latest adapter state.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp#300
[2] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp#1102

@@ +1816,5 @@
> +}
> +
> +/**
> + * AdapterPropertiesNotification will be called after enable() but
> + * before AdapterStateChangeCallback is called. At that moment, both

Replace |AdapterStateChangeCallback| with |AdapterStateChangedNotification|.

@@ +1817,5 @@
> +
> +/**
> + * AdapterPropertiesNotification will be called after enable() but
> + * before AdapterStateChangeCallback is called. At that moment, both
> + * BluetoothManager and BluetoothAdapter, do not register observer

nit: ... BluetoothAdapter have not register ...

@@ +1859,5 @@
> +      for (size_t index = 0; index < p.mStringArray.Length(); index++) {
> +        pairedDeviceAddresses.AppendElement(p.mStringArray[index]);
> +      }
> +
> +      BT_APPEND_NAMED_VALUE(propertiesArray, "Devices", pairedDeviceAddresses);

Should be "PairedDevices" as [3].

[3] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp#387
Attachment #8479934 - Flags: review?(btian) → review+
Comment on attachment 8479939 [details] [diff] [review]
[05] Bug 1054242: Integrate helper runnables into notification methods (under bluetooth2/) (v2)

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

Please see my comment below. I think in comment 15 the order of original code is distributing signal -> dispatching reply. Let me know if I missed anything.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1941,5 @@
> +                         nsString(aBdAddr), propertiesArray);
> +
> +  // FetchUuids task
> +  if (!sFetchUuidsRunnableArray.IsEmpty()) {
> +    // mProps contains Address and Uuids only

Replace |mProps| with |propertiesArray|.

@@ +1946,5 @@
> +    DispatchBluetoothReply(sFetchUuidsRunnableArray[0],
> +                           propertiesArray[1].value() /* Uuids */,
> +                           EmptyString());
> +    sFetchUuidsRunnableArray.RemoveElementAt(0);
> +    DistributeSignal(signal);

I think original code distributes signal first before dispatching reply since |DispatchBluetoothReply| inside dispatches a runnable to main thread as well. So the order should be |RemoteDevicePropertiesCallbackTask::Run| -> runnable that distributes signal -> runnable that dispatches reply.

Note the order we want is distributing signal -> dispatching reply, resulting in an onattributechanged event handler fired before promise is resolved.
ni? Thomas for comment 25.
Flags: needinfo?(tzimmermann)
Comment on attachment 8476652 [details] [diff] [review]
[06] Bug 1054242: Cleanup |BluetoothServiceBluedroid| and related functions (under bluetooth2/)

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

r=me with nit addressed.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +73,5 @@
>  static nsTArray<nsRefPtr<BluetoothReplyRunnable> > sUnbondingRunnableArray;
>  
> +static bool sAdapterDiscoverable(false);
> +static bool sAdapterDiscovering(false);
> +static bool sAdapterEnabled(false);

nit: Group these static variables with |sAdapterBdAddress| and |sAdapterBdName|.
Attachment #8476652 - Flags: review?(btian) → review+
See Also: → 1050068
> 
> I think original code distributes signal first before dispatching reply
> since |DispatchBluetoothReply| inside dispatches a runnable to main thread
> as well. So the order should be |RemoteDevicePropertiesCallbackTask::Run| ->
> runnable that distributes signal -> runnable that dispatches reply.

You're right. Thanks for checking once more. I'll update the code to first send the signal, and the reply afterwards.
Flags: needinfo?(tzimmermann)
Changes since v2:

  - update |sAdapterEnabled|
  - cleaned up comment
  - use value name "PairedDevices" for pairing updates
Attachment #8479934 - Attachment is obsolete: true
Attachment #8481205 - Flags: review+
Changes since v2:

  - use 'propertiesArray' in comment
  - minor fix to order of signal and reply in |RemoteDevicePropertiesNotification|

I made a minor adjustment to the order of operations, but the signal will always be delivered first, because it's done immediately. The reply is sent from another runnable. I think that's the order we want, right?
Attachment #8479939 - Attachment is obsolete: true
Attachment #8479939 - Flags: review?(btian)
Attachment #8481208 - Flags: review?(btian)
Changes since v1:

  - moved global boolean variables
Attachment #8476652 - Attachment is obsolete: true
Attachment #8481210 - Flags: review+
Comment on attachment 8481208 [details] [diff] [review]
[05] Bug 1054242: Integrate helper runnables into notification methods (under bluetooth2/) (v3)

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

r=me with the execution order fixed. Thanks for the additional effort to port on bluetooth2.

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1937,5 @@
>    }
>  
> +  // Update to registered BluetoothDevice objects
> +  BluetoothSignal signal(NS_LITERAL_STRING("PropertyChanged"),
> +                         nsString(aBdAddr), propertiesArray);

Distribute signal right here and remove other |DistributeSignal| calls in this function, as original code [1]. So that we distribute signal before dispatching any reply.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp#430
Attachment #8481208 - Flags: review?(btian) → review+
Blocks: 1019376
Hi Ben

(In reply to Ben Tian [:btian] from comment #32)
> ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +1937,5 @@
> >    }
> >  
> > +  // Update to registered BluetoothDevice objects
> > +  BluetoothSignal signal(NS_LITERAL_STRING("PropertyChanged"),
> > +                         nsString(aBdAddr), propertiesArray);
> 
> Distribute signal right here and remove other |DistributeSignal| calls in
> this function, as original code [1]. So that we distribute signal before
> dispatching any reply.
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/bluedroid/
> BluetoothServiceBluedroid.cpp#430

I think the current patch already does the correct thing.

There's the code below "// Use address as the index". In the original code, it's executed _before_ the signal gets distributed; and the current patch does the same. But with the intended change it would be executed afterwards.

Can you confirm?
Flags: needinfo?(btian)
Changes since v3:

  - go back to v2 ordering
  - explain the order of operations in |RemoteDevicePropertiesNotification| in a comment
Attachment #8481208 - Attachment is obsolete: true
Attachment #8482208 - Flags: review?(btian)
Comment on attachment 8482208 [details] [diff] [review]
[05] Bug 1054242: Integrate helper runnables into notification methods (under bluetooth2/) (v4)

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

r=me. Thanks for the effort!
Attachment #8482208 - Flags: review?(btian) → review+
Flags: needinfo?(btian)
Thanks Ben and Jocelyn for your help and patience with this bug. Usually porting these patches is straight-forward, but this time there where several subtle changes between the trees, so porting was a more complicated.

I'll land the patches as soon as the tree's open again.
You need to log in before you can comment on or make changes to this bug.