Closed Bug 850157 Opened 7 years ago Closed 7 years ago

[b2g-bluetooth] Add GetConnectedDevices() in nsIDOMBluetoothAdapter.idl

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: gyeh, Assigned: gyeh)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(2 files, 2 obsolete files)

In order to verify the connection status of each device before restoring connection in Settings app, GetConnectedDevices() is created to meet this requirement.
Comment on attachment 723884 [details] [diff] [review]
Patch 1(v1): Add GetConnectedDevices() in nsIDOMBluetoothAdapter.idl

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

Looks good to me. You may need to rebase to the current codebase. Redirect to Blake for superreview because of DOM API change.

::: dom/bluetooth/ipc/BluetoothServiceChildProcess.h
@@ +50,5 @@
> +  GetConnectedDevicePropertiesInternal(uint16_t aProfileId,
> +                                       BluetoothReplyRunnable* aRunnable)
> +                                       MOZ_OVERRIDE;
> +
> +

nit: extra line

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1904,5 @@
>  private:
>    BluetoothSignal mSignal;
>  };
>  
> +bool

Please make it static

@@ +1911,5 @@
> +  // We don't have to filter device here
> +  return true;
> +}
> +
> +bool

Please make it static

@@ +1929,5 @@
> +    if (deviceProperties[p].name().EqualsLiteral("Paired")) {
> +      return deviceProperties[p].value().get_bool();
> +    }
> +  }
> + 

nit: extra blank

@@ +1938,4 @@
>  {
>  public:
> +  BluetoothArrayOfDevicePropertiesRunnable(
> +    const nsTArray<nsString>& aDeviceAddresses

Please re-organize the alignment. It doesn't look good.

@@ +1943,5 @@
> +    , FilterFunc aFilterFunc)
> +    : mDeviceAddresses(aDeviceAddresses)
> +    , mRunnable(dont_AddRef(aRunnable))
> +    , mFilterFunc(aFilterFunc)
> +  {  

nit: trailing blanks

@@ +1983,2 @@
>      return NS_OK;
> +  } 

nit: trailing blanks
Attachment #723884 - Flags: superreview?(mrbkap)
Attachment #723884 - Flags: review?(echou)
Attachment #723884 - Flags: review+
Comment on attachment 723884 [details] [diff] [review]
Patch 1(v1): Add GetConnectedDevices() in nsIDOMBluetoothAdapter.idl

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

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +494,5 @@
> +BluetoothAdapter::GetConnectedDevices(uint16_t aProfileId, nsIDOMDOMRequest** aRequest)
> +{
> +  nsCOMPtr<nsIDOMDOMRequest> req;
> +  nsresult rv;
> +  rv = PrepareDOMRequest(GetOwner(), getter_AddRefs(req));

Nit: nsresult rv = Prepare...
Attachment #723884 - Flags: superreview?(mrbkap) → superreview+
Blocks: 862402
Attached patch v2 patch: rebase patch (obsolete) — Splinter Review
When there's no connected device, we should reply success with a 0-length array of devices.
Attachment #723884 - Attachment is obsolete: true
Attachment #741201 - Flags: review?(echou)
Also, add method GetAddress() in both BluetoothHfpManager and BluetoothOppManager, so that GetConnectedDevices() can get socket address from profile managers.
Comment on attachment 741201 [details] [diff] [review]
v2 patch: rebase patch

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

r=me with nits addressed.

::: dom/bluetooth/BluetoothAdapter.cpp
@@ +460,5 @@
>  
>  NS_IMETHODIMP
> +BluetoothAdapter::GetConnectedDevices(uint16_t aProfileId,
> +                                      nsIDOMDOMRequest** aRequest)
> +{

need any assertion here? Like MOZ_ASSERT(NS_IsMainThread())

@@ +471,5 @@
> +
> +  BluetoothService* bs = BluetoothService::Get();
> +  NS_ENSURE_TRUE(bs, NS_ERROR_FAILURE);
> +  rv = bs->GetConnectedDevicePropertiesInternal(aProfileId, results);
> +  if (NS_FAILED(rv)) {

NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE); can be used here as well.
Attachment #741201 - Flags: review?(echou) → review+
Attachment #741201 - Attachment is obsolete: true
Whiteboard: [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/4db652bb932f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Nominate this as tef+ because we need this GetConnecteDevices() API for solving bug 870689 correctly instead of using a workaround. Gaia/settings needs to know which profile is having a connection by this function.
blocking-b2g: --- → tef?
But is this needed for certification? I've seen that bug 870689 is already fixed
Flags: needinfo?(echou)
I refined the patch so that bug 870689 no longer depends on this bug.
No longer blocks: 870689
(In reply to Daniel Coloma:dcoloma from comment #11)
> But is this needed for certification? I've seen that bug 870689 is already
> fixed

We don't need it for bug 870689 now since Arthur used a workaround. Remove tef? nomination.

Thank you, Daniel.
blocking-b2g: tef? → ---
Flags: needinfo?(echou)
Re-nominate as leo+ since we need this API to complete bug 862402.
blocking-b2g: --- → leo?
blocking-b2g: leo? → leo+
The patch cannot be merged into b2g18. We need a new patch based on current v2g18, Gina.
No problem. Will update b2g18 patch later.
Attached patch b2g18 patchSplinter Review
Here comes the b2g18 patch!
Whiteboard: [fixed-in-birch] → [fixed-in-birch] [checkin-needed]
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.