Closed
Bug 1100818
Opened 10 years ago
Closed 10 years ago
[Bluetooth 2] Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent.
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jaliu, Assigned: jaliu)
References
Details
(Whiteboard: [webbt-api])
Attachments
(1 file, 9 obsolete files)
Since we decided to move pairingReq listener from system app to bluetooth app, it's necessary to provide a mechanism to launch bluetooth app when receiving incoming pairing request.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8525866 -
Flags: review?(btian)
Comment 2•10 years ago
|
||
Comment on attachment 8525866 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v0)
Ian,
Per your offline discussion with Jamin, please verify this patch with gaia API v2 change and let us know for any problem. Thanks.
Attachment #8525866 -
Flags: feedback?(iliu)
Comment 3•10 years ago
|
||
Comment on attachment 8525866 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v0)
Per offline discussion with Jamin, the patch is not working for receiving 'bluetooth-request-pairing' system message @Bluetooth app.
Attachment #8525866 -
Flags: feedback?(iliu)
Comment 4•10 years ago
|
||
Comment on attachment 8525866 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v0)
Clear r? since the patch requires revision for comment 3.
Attachment #8525866 -
Flags: review?(btian)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Ian Liu [:ianliu] from comment #3)
Thank you for the feedback.
I'll investigate it.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8525866 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8528337 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8528724 -
Flags: review?(btian)
Assignee | ||
Comment 8•10 years ago
|
||
Hi Ben,
I'd like to ask you to review it. :)
Ian and I have verified #attachment 8528724 [details] [diff] [review] by sending and receiving pairing request with the WIP patch of Bug 1102796 Bug 1070823.
Comment 9•10 years ago
|
||
Comment on attachment 8528724 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v1)
Review of attachment 8528724 [details] [diff] [review]:
-----------------------------------------------------------------
Please see my comment below.
::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +199,5 @@
> , mDiscoveryHandleInUse(nullptr)
> {
> MOZ_ASSERT(aWindow);
>
> + // Retrieval the app status and origin for permission checking
'Retrieve'
@@ +208,5 @@
> + doc->NodePrincipal()->GetAppStatus(&appStatus);
> + doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));
> + }
> +
> + // Only allow certificated bluetooth application to receive pairing request
'certified'
@@ +209,5 @@
> + doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));
> + }
> +
> + // Only allow certificated bluetooth application to receive pairing request
> + if (appStatus == nsIPrincipal::APP_STATUS_CERTIFIED
Wrap checking of bluetooth as a function.
if (IsBluetoothApp()) {
// create pairing listener
}
@@ +210,5 @@
> + }
> +
> + // Only allow certificated bluetooth application to receive pairing request
> + if (appStatus == nsIPrincipal::APP_STATUS_CERTIFIED
> + && appOrigin.EqualsASCII(BLUETOOTH_APP_ORIGIN)) {
nit: put '&&' at the end of previous line.
::: dom/bluetooth2/BluetoothCommon.h
@@ +139,5 @@
> #define KEY_LOCAL_AGENT "/B2G/bluetooth/agent"
> #define KEY_REMOTE_AGENT "/B2G/bluetooth/remote_device_agent"
> #define KEY_MANAGER "/B2G/bluetooth/manager"
> #define KEY_ADAPTER "/B2G/bluetooth/adapter"
> +#define KEY_PAIRINGREQS "/B2G/bluetooth/adapter/pairingreqs"
Should be #define KEY_PAIRING_LISTENER "B2G/bluetooth/pairing_listener"
@@ +168,5 @@
> #define PAIRING_REQ_TYPE_CONFIRMATION "pairingconfirmationreq"
> #define PAIRING_REQ_TYPE_CONSENT "pairingconsentreq"
>
> /**
> + * A system message used for launching bluetooth certified app if there is no
System message to launch bluetooth app if no pairing listener is ready to receive pairing requests.
@@ +174,5 @@
> + */
> +#define SYS_MSG_BT_PAIRING_REQ "bluetooth-pairing-request"
> +
> +/**
> + * The app origin of the bluetooth app which is responsible for listening
... of bluetooth app, which ...
::: dom/bluetooth2/BluetoothPairingListener.cpp
@@ +25,5 @@
> MOZ_ASSERT(aWindow);
> +
> + BluetoothService* bs = BluetoothService::Get();
> + NS_ENSURE_TRUE_VOID(bs);
> + bs->RegisterBluetoothSignalHandler( NS_LITERAL_STRING(KEY_PAIRINGREQS), this);
nit: remove extra space
@@ +43,5 @@
>
> BluetoothPairingListener::~BluetoothPairingListener()
> {
> + BluetoothService* bs = BluetoothService::Get();
> + // It can be null on shutdown.
nullptr
@@ +107,5 @@
> +
> + // Notify pairing listener of pairing requests
> + DispatchPairingEvent(device, passkey, type);
> + } else {
> + BT_WARNING("Not handling adapter signal: %s",
'pairing listener' signal
::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +881,5 @@
> return NS_OK;
> }
>
> +void
> +BluetoothServiceBluedroid::RegisterBluetoothSignalHandler(
Move out the queuing mechanism to BluetoothService.cpp.
::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp
@@ +93,4 @@
> if (sBluetoothChild && !IsSignalRegistered(aNodeName)) {
> sBluetoothChild->SendUnregisterSignalHandler(nsString(aNodeName));
> }
> + BluetoothService::UnregisterBluetoothSignalHandler(aNodeName, aHandler);
Why move this line?
::: dom/webidl/BluetoothAdapter2.webidl
@@ +41,5 @@
> readonly attribute boolean discoverable;
> readonly attribute boolean discovering;
>
> + /**
> + * Only allow one certified app to listen pairing requests.
Suggest to revise as folllowing.
Only bluetooth app is allowed to listen to pairing requests.
Bluetooth app's origin is "app://bluetooth.gaiamobile.org".
Attachment #8528724 -
Flags: review?(btian)
Assignee | ||
Comment 10•10 years ago
|
||
- Revised patch v1 based on #comment 9.
Attachment #8528724 -
Attachment is obsolete: true
Attachment #8535400 -
Flags: review?(btian)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8535400 -
Attachment is obsolete: true
Attachment #8535400 -
Flags: review?(btian)
Attachment #8535408 -
Flags: review?(btian)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Ben Tian [:btian] from comment #9)
> ::: dom/bluetooth2/ipc/BluetoothServiceChildProcess.cpp
> @@ +93,4 @@
> > if (sBluetoothChild && !IsSignalRegistered(aNodeName)) {
> > sBluetoothChild->SendUnregisterSignalHandler(nsString(aNodeName));
> > }
> > + BluetoothService::UnregisterBluetoothSignalHandler(aNodeName, aHandler);
>
> Why move this line?
After I read comments on Bug 860075, I reverted this change at Attachment #8535408 [details] [diff].
Thank you for asking.
Comment 13•10 years ago
|
||
Comment on attachment 8535408 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v3)
Review of attachment 8535408 [details] [diff] [review]:
-----------------------------------------------------------------
Please see my comment below. I prefer to move all queuing mechanism from BluetoothServiceBluedroid into BluetoothService, since the mechanism is backend-independent.
::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +198,5 @@
> , mDiscoveryHandleInUse(nullptr)
> {
> MOZ_ASSERT(aWindow);
>
> + // Only allow certified bluetooth application to receive pairing request
nit: request's'
@@ +230,5 @@
>
> BluetoothService* bs = BluetoothService::Get();
> NS_ENSURE_TRUE_VOID(bs);
> bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_ADAPTER), this);
> + bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_PAIRING_LISTENER),
Move this unregistration into |BluetoothPairingListener| and add function |DisconnectFromOwner| as [1], since |BluetoothPairingListener| also inherits |DOMEventTargetHelper| [2].
[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth2/BluetoothAdapter.cpp#222
[2] http://dxr.mozilla.org/mozilla-central/source/dom/events/DOMEventTargetHelper.h#128
@@ +770,5 @@
> +bool
> +BluetoothAdapter::IsBluetoothCertifiedApp()
> +{
> + // Retrieve the app status and origin for permission checking
> + nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
nit: reorder to declare |doc| close to where it's used.
{
uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
nsAutoCString appOrigin;
// Retrieve the app status and origin for permission checking
nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
NS_ENSURE_TRUE(doc, false);
doc->NodePrincipal()->GetAppStatus(&appStatus);
doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));
...
}
::: dom/bluetooth2/BluetoothPairingListener.cpp
@@ +109,5 @@
> +
> + // Notify pairing listener of pairing requests
> + DispatchPairingEvent(device, passkey, type);
> + } else {
> + BT_WARNING("Not handling 'pairing listener' signal: %s",
In prior comment I leave single quote mark of 'pairing listener' only to emphasize. Please remove it:)
"Not handling pairing listener signal: %s",
::: dom/bluetooth2/BluetoothService.cpp
@@ +291,5 @@
> }
>
> ol->AddObserver(aHandler);
> +
> + // Distribute pending paring requests when pairing listener has been added
typo: pa'i'ring requests
::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1528,5 @@
> +
> + // If there is no BluetoohPairingListener in observer table, put the signal
> + // into a pending queue of paring requests and send a system message to launch
> + // the app.
> + if (!mBluetoothSignalObserverTable.Get(
Move this queuing mechanism into |BluetoothService::DistributeSignal|.
@@ +1586,5 @@
> + NS_LITERAL_STRING(KEY_PAIRING_LISTENER),
> + BluetoothValue(propertiesArray));
> +
> +
> + // If there is no BluetoohPairingListener in observer table, put the signal
Ditto.
Attachment #8535408 -
Flags: review?(btian)
Assignee | ||
Comment 14•10 years ago
|
||
- Revise patch v3 based on comment 13
Attachment #8535408 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8535527 -
Flags: review?(btian)
Comment 15•10 years ago
|
||
Comment on attachment 8535527 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v4)
Review of attachment 8535527 [details] [diff] [review]:
-----------------------------------------------------------------
Almost there. Please see my comment below.
::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +771,5 @@
> + // Retrieve the app status and origin for permission checking
> + uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
> + nsAutoCString appOrigin;
> + nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> + if (doc) {
I prefer to check |!doc| here and return false directly here, to clarify that false is returned when |!doc|. But I'm fine with either way.
@@ +777,5 @@
> + doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));
> + }
> +
> + return appStatus == nsIPrincipal::APP_STATUS_CERTIFIED &&
> + appOrigin.EqualsASCII(BLUETOOTH_APP_ORIGIN);
Use |EqualsLiteral|.
::: dom/bluetooth2/BluetoothPairingListener.cpp
@@ +128,5 @@
> +
> + BluetoothService* bs = BluetoothService::Get();
> + NS_ENSURE_TRUE_VOID(bs);
> + bs->UnregisterBluetoothSignalHandler(NS_LITERAL_STRING(KEY_PAIRING_LISTENER),
> + this);
nit: align the 2 lines.
::: dom/bluetooth2/BluetoothService.cpp
@@ +294,5 @@
> +
> + // Distribute pending pairing requests when pairing listener has been added
> + // to signal observer table.
> + if (IsMainProcess() &&
> + aNodeName.Equals(NS_LITERAL_STRING(KEY_PAIRING_LISTENER))) {
Use |EqualsLiteral|. Also ensure |!mPendingPairReqSignals.IsEmpty()|.
@@ +359,5 @@
>
> BluetoothSignalObserverList* ol;
> if (!mBluetoothSignalObserverTable.Get(aSignal.path(), &ol)) {
> + // If there is no BluetoohPairingListener in observer table, put the signal
> + // into a pending queue of paring requests and send a system message to launch
nit: queue of pa'i'ring requests
@@ +360,5 @@
> BluetoothSignalObserverList* ol;
> if (!mBluetoothSignalObserverTable.Get(aSignal.path(), &ol)) {
> + // If there is no BluetoohPairingListener in observer table, put the signal
> + // into a pending queue of paring requests and send a system message to launch
> + // the app.
nit: 'launch bluetooth certified app' is clearer.
@@ +361,5 @@
> if (!mBluetoothSignalObserverTable.Get(aSignal.path(), &ol)) {
> + // If there is no BluetoohPairingListener in observer table, put the signal
> + // into a pending queue of paring requests and send a system message to launch
> + // the app.
> + if (aSignal.path().EqualsASCII(KEY_PAIRING_LISTENER) ) {
Use |EqualsLiteral|. Also remove space between two ')'
@@ +364,5 @@
> + // the app.
> + if (aSignal.path().EqualsASCII(KEY_PAIRING_LISTENER) ) {
> + mPendingPairReqSignals.AppendElement(aSignal);
> +
> + nsString type = NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ);
Simplify to NS_NAMED_LITERAL_STRING(type, SYS_MSG_BT_PAIRING_REQ)
http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsLiteralString.h#31
@@ +365,5 @@
> + if (aSignal.path().EqualsASCII(KEY_PAIRING_LISTENER) ) {
> + mPendingPairReqSignals.AppendElement(aSignal);
> +
> + nsString type = NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ);
> + InfallibleTArray<BluetoothNamedValue> parameters;
nit: rename to |emptyArr| to clarify it's an empty array.
@@ +366,5 @@
> + mPendingPairReqSignals.AppendElement(aSignal);
> +
> + nsString type = NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ);
> + InfallibleTArray<BluetoothNamedValue> parameters;
> + BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(type, parameters);
Can we simplify all 3 lines to the following?
BT_ENSURE_TRUE_VOID_BROADCAST_SYSMSG(NS_LITERAL_STRING(SYS_MSG_BT_PAIRING_REQ),
BluetoothValue(EmptyString()));
::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1521,5 @@
> BT_APPEND_NAMED_VALUE(propertiesArray, "passkey", EmptyString());
> BT_APPEND_NAMED_VALUE(propertiesArray, "type",
> NS_LITERAL_STRING(PAIRING_REQ_TYPE_ENTERPINCODE));
>
> +
nit: remove this extra newline.
Updated•10 years ago
|
Attachment #8535527 -
Flags: review?(btian)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8535527 -
Attachment is obsolete: true
Attachment #8537055 -
Flags: review?(btian)
Comment 17•10 years ago
|
||
Comment on attachment 8537055 [details] [diff] [review]
Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v5)
Review of attachment 8537055 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comment addressed. Thanks for the effort! Please request DOM peer review to land the webidl change.
::: dom/bluetooth2/BluetoothAdapter.cpp
@@ +767,5 @@
>
> +bool
> +BluetoothAdapter::IsBluetoothCertifiedApp()
> +{
> + // Retrieve the app status and origin for permission checking
nit: reorder as following to make declaration close to where it's used.
{
// Retrieve the app status and origin for permission checking
nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
NS_ENSURE_TRUE(doc, false);
uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
nsAutoCString appOrigin;
doc->NodePrincipal()->GetAppStatus(&appStatus);
doc->NodePrincipal()->GetOrigin(getter_Copies(appOrigin));
return appStatus == nsIPrincipal::APP_STATUS_CERTIFIED &&
appOrigin.EqualsLiteral(BLUETOOTH_APP_ORIGIN);
}
@@ +771,5 @@
> + // Retrieve the app status and origin for permission checking
> + uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
> + nsAutoCString appOrigin;
> + nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> + if (!doc) {
Simplify to NS_ENSURE_TRUE(doc, false)
Attachment #8537055 -
Flags: review?(btian) → review+
Assignee | ||
Comment 18•10 years ago
|
||
Thank Ben for reviewing the patch. :)
Attachment #8537055 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
It looks fine on treeherder.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c358dc3d61f
Assignee | ||
Comment 20•10 years ago
|
||
- convert to hg format
Attachment #8538386 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 21•10 years ago
|
||
- Revert the change on BluetoothAdatper2.webidl.
I put an comment on BluetoothAdatper2.webidl to explain the usage and restriction of adapter.pairingReqs on patch v6.
However, since every webidl change requires for a DOM peer review, it seems annoying to ask DOM peer to review a patch which only added one simple comment.
Per offline discussion with Ben, I decide to move this comment to Bug 1063444 which is going to change BluetoothAdatper2.webidl and ask for DOM peer review.
Attachment #8538918 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8538939 -
Attachment description: (final) Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v7) → (final) Launch bluetooth certified app by sending system message if it's not ready for receiving BluetoothPairingEvent. (v7), r=btian
Comment 22•10 years ago
|
||
Keywords: checkin-needed
Comment 23•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•