Closed Bug 930853 Opened 11 years ago Closed 11 years ago

[bluedroid]Support SetProperty function which enables SetName, SetDiscoverable, SetDiscoveryTimeout

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

Attachments

(1 file, 6 obsolete files)

Bug 930853 - [Bluetooth] Support SetProperty function which enables SetName, SetDiscoverable, SetDiscoveryTimeout, r=echou
5.32 KB, patch
Details | Diff | Splinter Review 
Support SetProperty function which enables SetName, SetDiscoverable, SetDiscoveryTimeout
Assignee: nobody → shuang
Attachment #823334 - Attachment is obsolete: true
Attachment #823334 - Flags: review?(echou)
Attachment #823419 - Attachment is obsolete: true
Attachment #823419 - Flags: review?(echou)
+    // bluedroid BTU task was stored in the task queue, see GKI_send_msg
+    if (!sSetPropertyRunnableArray.IsEmpty()) {
+      BluetoothValue values(true);
+      DispatchBluetoothReply(sSetPropertyRunnableArray[0], values, EmptyString());
+      sSetPropertyRunnableArray.RemoveElementAt(0);

SetProperty in Bluedroid, it actually sends set property task to a task queue.
In gki/common/gki_buffer.c, function GKI_send_msg add task to the task queue.
In gki/ulinux/gki_ulinux.c::GKI_send_event, pthread_cond_signal notify other thread.
We can expect SetProperty task will be treated as FCFS.
Another problem is that Gaia might call the same function call many times at once,
however, bluedroid seems return BT_STATUS_BUSY directly (at least non BT_STATUS_SUCCESS, for example: bond)
if there is another operation is ongoing. So we don't expect there will be multiple request at the same time.
If this is still not convinced, i can do more tests.
Comment on attachment 823435 [details] [diff] [review]
Bug 930853 - [Bluetooth] Support SetProperty function which enables SetName, SetDiscoverable, SetDiscoveryTimeout

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

Hi Shawn, please see my comments below. Thanks.

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +68,5 @@
>  static bool sAdapterDiscoverable = false;
>  static nsString sAdapterBdAddress;
>  static nsString sAdapterBdName;
>  static uint32_t sAdapterDiscoverableTimeout;
> +static nsTArray<BluetoothReplyRunnable*> sSetPropertyRunnableArray;

nsTArray<nsRefPtr<BluetoothReplyRunnable> > should be used.

@@ +162,5 @@
> +
> +    // bluedroid BTU task was stored in the task queue, see GKI_send_msg
> +    if (!sSetPropertyRunnableArray.IsEmpty()) {
> +      BluetoothValue values(true);
> +      DispatchBluetoothReply(sSetPropertyRunnableArray[0], values, EmptyString());

nit: you may want to replace 'values' with 'BluetoothValue(true)'.

@@ +349,5 @@
> +    prop.val = (void*)aValue.value().get_uint32_t();
> +  } else if (aValue.value().type() == BluetoothValue::TnsString) {
> +    // Set name
> +    nsString str = aValue.value().get_nsString();
> +    char* cstr = ToNewUTF8String(str);

This will cause memory leak. Please use NS_ConvertUTF16toUTF8(str).get() instead or find other ways to avoid leakage.

@@ +367,5 @@
> +    return NS_OK;
> +  }
> +
> +  sSetPropertyRunnableArray.AppendElement(aRunnable);
> +  sBtInterface->set_adapter_property(&prop);

This may fail. Value "BT_STATUS_NOT_READY" may return if protocol stack is not ready. We need to fire aRunnable right here for error handling.
Attachment #823435 - Flags: review?(echou) → review-
Attachment #824406 - Attachment is obsolete: true
Attachment #824406 - Flags: review?(echou)
Comment on attachment 824550 [details] [diff] [review]
Bug 930853 - [Bluetooth] Support SetProperty function which enables SetName, SetDiscoverable, SetDiscoveryTimeout

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

Almost there. :)

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +68,5 @@
>  static bool sAdapterDiscoverable = false;
>  static nsString sAdapterBdAddress;
>  static nsString sAdapterBdName;
>  static uint32_t sAdapterDiscoverableTimeout;
> +static nsTArray<BluetoothReplyRunnable*> sSetPropertyRunnableArray;

Please use nsRefPtr instead of raw ptr.

@@ +228,5 @@
>  }
>  
> +static void
> +ReplyStatusError(BluetoothReplyRunnable* aBluetoothReplyRunnable,
> +  int aStatusCode, const nsAString& aCustomMsg)

nit: please line up parameters.

@@ +249,5 @@
> +    replyError.AppendLiteral(":BT_STATUS_FAIL");
> +  }
> +
> +  DispatchBluetoothReply(aBluetoothReplyRunnable, BluetoothValue(true),
> +    replyError);

Ditto.

@@ +378,5 @@
> +  } else if (aValue.value().type() == BluetoothValue::TnsString) {
> +    // Set name
> +    nsString str = aValue.value().get_nsString();
> +    const char* name = NS_ConvertUTF16toUTF8(str).get();
> +    prop.val = (void*)name;

This would be a problem. The memory space of 'str' will become invalid outside this block and so will the value held by prop.val. The easiest way to solve this may be moving 'nsString str' outside this if-block.
Attachment #824550 - Flags: review?(echou) → review-
Comment on attachment 824571 [details] [diff] [review]
Bug 930853 - [Bluetooth] Support SetProperty function which enables SetName, SetDiscoverable, SetDiscoveryTimeout

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

r=me with nits addressed.

::: dom/bluetooth/BluetoothServiceBluedroid.cpp
@@ +68,5 @@
>  static bool sAdapterDiscoverable = false;
>  static nsString sAdapterBdAddress;
>  static nsString sAdapterBdName;
>  static uint32_t sAdapterDiscoverableTimeout;
> +static nsTArray<nsRefPtr<BluetoothReplyRunnable>> sSetPropertyRunnableArray;

nit: although the compilation may succeed, please insert a blank between two '>'s. It would be safer and more common.

@@ +164,5 @@
>      }
> +
> +    // bluedroid BTU task was stored in the task queue, see GKI_send_msg
> +    if (!sSetPropertyRunnableArray.IsEmpty()) {
> +      DispatchBluetoothReply(sSetPropertyRunnableArray[0], BluetoothValue(true), EmptyString());

nit: 80-characters per line, please.
Attachment #824571 - Flags: review?(echou) → review+
https://hg.mozilla.org/mozilla-central/rev/046777105576
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: