Closed
Bug 1184017
Opened 9 years ago
Closed 9 years ago
[MAP] Dispatch events to MAP event handlers
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(feature-b2g:2.2r+, firefox44 fixed, b2g-v2.2r fixed)
People
(Reporter: shawnjohnjr, Assigned: shawnjohnjr)
References
Details
(Whiteboard: [red-tai])
Attachments
(2 files, 15 obsolete files)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → shuang
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8635284 -
Attachment is obsolete: true
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8635981 -
Attachment is obsolete: true
Assignee | ||
Comment 3•9 years ago
|
||
Revised patch for ReplyTo* api in RequestHandle.
Assignee | ||
Updated•9 years ago
|
Attachment #8636450 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Add BluetoothMapFolderListingEvent.
Assignee | ||
Comment 5•9 years ago
|
||
Add GetMessageEvent, FolderListingEvent, SendMessageEvent, SetMessageStatusEvent
Assignee | ||
Updated•9 years ago
|
Attachment #8636542 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8637216 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8637767 [details] [diff] [review]
bug1184017-mc.patch (WIP)
Review of attachment 8637767 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/webidl/BluetoothMapRequestHandle.webidl
@@ +17,5 @@
> + * Reply the Messages-Listing object to the MAP request. The DOMRequest will
> + * be rejected if the MAP request operation fails.
> + */
> + [NewObject, Throws, AvailableIn=CertifiedApps]
> + DOMRequest replyToMapMessagesListing(long handleid, Blob messageslisting);
There are three parameters are missing to reply to Msg-listing.
- NewMessage
- MSETime
- MessagesListingSize
Assignee | ||
Updated•9 years ago
|
blocking-b2g: --- → 2.2r?
Comment 8•9 years ago
|
||
Revise flag to feature-b2g: 2.2r+ since MAP is required bluetooth feature for 2.2r.
blocking-b2g: 2.2r? → ---
feature-b2g: --- → 2.2r+
Assignee | ||
Updated•9 years ago
|
Attachment #8637767 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Rebase to the latest m-c.
Assignee | ||
Updated•9 years ago
|
Attachment #8661221 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8666010 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8667301 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8667693 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8669699 -
Attachment description: bug1184017-mc.patch (WIP) → Bug 1184017 - [MAP] Dispatch events to MAP event handlers
Assignee | ||
Updated•9 years ago
|
Attachment #8669699 -
Flags: review?(btian)
Comment 14•9 years ago
|
||
Comment on attachment 8669699 [details] [diff] [review]
Bug 1184017 - [MAP] Dispatch events to MAP event handlers
Review of attachment 8669699 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comment addressed.
::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
@@ +764,4 @@
> break;
> }
> case Map::AppParametersTagId::FilterReadStatus: {
> uint8_t filterReadStatus = *((uint8_t *)buf);
Revise as [1] to check range based on webidl, and revise BluetoothAdapter as [2].
[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothPbapManager.cpp#519
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothAdapter.cpp#1298
@@ +787,4 @@
> break;
> }
> case Map::AppParametersTagId::FilterPriority: {
> uint8_t filterPriority = *((uint8_t *)buf);
Ditto.
@@ +800,4 @@
> break;
> }
> case Map::AppParametersTagId::Charset: {
> uint8_t charset = *((uint8_t *)buf);
Ditto.
::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
@@ +1387,5 @@
> + const InfallibleTArray<BluetoothNamedValue>& arr =
> + aValue.get_ArrayOfBluetoothNamedValue();
> +
> + MOZ_ASSERT(arr.Length() >= 1 &&
> + arr[0].value().type() == BluetoothValue::Tuint32_t);
nit: alignment as following. Also revise all the follows.
MOZ_ASSERT(arr.Length() >= 1 &&
arr[0].value().type() == BluetoothValue::Tuint32_t);
@@ +1434,5 @@
> + init.mSubjectLength = value.get_uint32_t();
> + } else if (name.EqualsLiteral("parameterMask")) {
> + init.mParameterMask = GetParameterMask(value);
> + } else if (name.EqualsLiteral("filterMessageType")) {
> + if (value.get_uint32_t() == (FILTER_NO_EMAIL | FILTER_NO_MMS |
Move this logic into MapSmsManager.
@@ +1451,5 @@
> + init.mFilterPeriodBegin = value.get_nsString();
> + } else if (name.EqualsLiteral("filterPeriodEnd")) {
> + init.mFilterPeriodEnd = value.get_nsString();
> + } else if (name.EqualsLiteral("filterReadStatus")) {
> + if (value.get_uint32_t() == 0x01) {
Revise as [2] above.
::: dom/bluetooth/common/webapi/BluetoothAdapter.h
@@ +410,5 @@
> + */
> + void HandleMapSendMessage(const BluetoothValue& aValue);
> +
> + /**
> + * Handle "HandleMapUpdate" bluetooth signal.
Replace HandleMapUpdate with MapMessageUpdate.
@@ +413,5 @@
> + /**
> + * Handle "HandleMapUpdate" bluetooth signal.
> + *
> + * @param aValue [in] Properties array of the scanned device.
> + * - nsString 'MASInstanceID'
nit: align to 'Properties array'
::: dom/bluetooth/common/webapi/BluetoothMapRequestHandle.cpp
@@ +47,5 @@
> +
> +already_AddRefed<DOMRequest>
> +BluetoothMapRequestHandle::ReplyToMapFolderListing(long aMasId,
> + const nsAString&
> + aFolderlists,
nit: align as following
BluetoothMapRequestHandle::ReplyToMapFolderListing(
long aMasId, const nsAString& aFolderlists, ErrorResult& aRv)
@@ +101,5 @@
> +}
> +
> +JSObject*
> +BluetoothMapRequestHandle::WrapObject(JSContext* aCx,
> + JS::Handle<JSObject*> aGivenProto)
nit: alignment
::: dom/bluetooth/common/webapi/BluetoothMapRequestHandle.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_bluetooth_bluetoothmaprequesthandle_h
nit: mozilla_dom_bluetooth_BluetoothMapRequestHandle_h
@@ +22,5 @@
> +
> +BEGIN_BLUETOOTH_NAMESPACE
> +
> +class BluetoothMapRequestHandle final : public nsISupports
> + , public nsWrapperCache
nit: alignment
@@ +39,5 @@
> +
> + virtual JSObject* WrapObject(JSContext* aCx,
> + JS::Handle<JSObject*> aGivenProto) override;
> + /**
> + * Reply to folder listing request
nit: remove redundant 'request' to conform with other methods' comment.
@@ +42,5 @@
> + /**
> + * Reply to folder listing request
> + *
> + * @param aMasId [in] MAS ID.
> + * @param aFolderlists [in] MAP Folder Name.
nit: MAP folder name to conform with capitalization.
@@ +47,5 @@
> + * @param aRv [out] Error result to set in case of error.
> + */
> + already_AddRefed<DOMRequest> ReplyToMapFolderListing(long aMasId,
> + const nsAString&
> + aFolderlists,
nite: align as following.
already_AddRefed<DOMRequest> ReplyToMapFolderListing(
long aMasId, const nsAString& aFolderlists, ErrorResult& aRv);
@@ +54,5 @@
> + * Reply to messages listing
> + *
> + * @param aMasId [in] MAS ID.
> + * @param aBlob [in] MAP messages listing content.
> + * @param aNewMessage [in] Indicates that a new message has been received
Simplify to:
Whether MSE has received a new message
@@ +57,5 @@
> + * @param aBlob [in] MAP messages listing content.
> + * @param aNewMessage [in] Indicates that a new message has been received
> + * by the MSE.
> + * @param aTimestamp [in] Report the Local Time basis of the MSE and its
> + * UTC offset. the MCE is supported in
Simplify to:
The local time basis and UTC offset of MSE. MCE will interpret the timestamps ...
@@ +60,5 @@
> + * @param aTimestamp [in] Report the Local Time basis of the MSE and its
> + * UTC offset. the MCE is supported in
> + * interpreting the timestamps of the messages
> + * listing entries.
> + * @param aSize [in] Report the number of accessible messages in the
Simplify to:
Number of accessible messages ...
@@ +68,5 @@
> + already_AddRefed<DOMRequest> ReplyToMapMessagesListing(long aMasid,
> + Blob& aBlob,
> + bool aNewMessage,
> + const nsAString&
> + aTimestamp,
nite: align as following.
already_AddRefed<DOMRequest> ReplyToMapMessagesListing(
long aMasId, Blob& aBlob, bool aNewMessage,
const nsAString& aTimestamp, int aSize, ErrorResult& aRv);
@@ +103,5 @@
> + /**
> + * Reply to message update request
> + *
> + * @param aMasId [in] MAS ID.
> + * @param aStatus [in] Update inbox results.
nit: result
::: dom/webidl/BluetoothMapParameters.webidl
@@ +5,5 @@
> + */
> +
> +/**
> + * MAP Application Parameters
> + *
nit: remove this extra line
::: dom/webidl/BluetoothMapRequestHandle.webidl
@@ +10,5 @@
> + * Reply to Folder-Listing object for MAP request. The DOMRequest will be rejected if
> + * the MAP request operation fails.
> + */
> + [NewObject, Throws, AvailableIn=CertifiedApps]
> + DOMRequest replyToMapFolderListing(long masid, DOMString folders);
nit:
- remove Map from all method names since BluetoothMapRequestHandle already indicates these methods are for MAP.
- rename |masid| to |masId| to conform with |instanceId|
DOMRequest replyToFolerListing(long masId, DOMString folders);
Attachment #8669699 -
Flags: review?(btian) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8669699 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8670670 [details] [diff] [review]
Bug 1184017 - [MAP] Dispatch events to MAP event handlers
Review of attachment 8670670 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/tests/mochitest/general/test_interfaces.html
@@ +223,5 @@
> + {name: "BluetoothMapFolderListingEvent", b2g: true, permission: ["bluetooth"]},
> +// IMPORTANT: Do not change this list without review from a DOM peer!
> + {name: "BluetoothMapGetMessageEvent", b2g: true, permission: ["bluetooth"]},
> +// IMPORTANT: Do not change this list without review from a DOM peer!
> + {name: "BluetoothMapMessagesListingEvent:", b2g: true, permission: ["bluetooth"]},
typo :( Extra ":".
This causes test_interfaces.html fail.
180 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface BluetoothMapMessagesListingEvent to all webpages as a property on the window (XBL: false)?
Assignee | ||
Comment 18•9 years ago
|
||
Fix one problem in test_interface.html.
Push to try server again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcbd4cde495c
Assignee | ||
Updated•9 years ago
|
Attachment #8670670 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8670746 -
Attachment description: Bug 1184017 - [MAP] Dispatch events to MAP event handlers, r=btian → Bug 1184017 - [MAP] Dispatch events to MAP event handlers, r=btian (m-c)
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8670746 [details] [diff] [review]
Bug 1184017 - [MAP] Dispatch events to MAP event handlers, r=btian (m-c)
Review of attachment 8670746 [details] [diff] [review]:
-----------------------------------------------------------------
Hi Blake,
Would you mind helping to review this patch regarding to WebIDL part?
MAP (Message Access Profile) specification defines the procedures and protocols to exchange Messages objects (such as SMS/MMS/Email) between devices.
A typical scenario is to let a Car-Kit retrieve SMS/MMS from a mobile device. Please refer to [1].
Due to the request from our partner, Bluetooth team would like to support MAP on FxOS as a MSE (Messaging Server Equipment).
Since the SMS/MMS are controlled by Gaia apps, I think it's reasonable to expose few related APIs to certified app only.
Per discussion with Gaia developer Sean Lee, I implemented three MAP events and their event handlers in this Bug.
The events are used to notify Gaia the request from remote Bluetooth device.
There are 8 new webidl files in this patch.
- BluetoothMapFolderListingEvent.webidl
The event is used to notify Gaia about the 'FolderListing' request from MCE(Messaging Client Equipment).
The 'FolderListing function is a MAP function which is designed to retrieve an entire folder listing from the MSE (Messaging Server Equipment).
- BluetoothMapGetMessageEvent.webidl
The event is used to notify Gaia about the 'GetMessage' request from MCE.
The 'GetMessage' function retrieves the MSE’s bMessage object (to retrieve the specific message).
- BluetoothMapMessagesListingEvent.webidl
The event is used to notify Gaia about the 'MessagesListing' request from MCE.
The 'MessagesListing' function retrieves a list of messages.
- BluetoothMapMessageUpdateEvent.webidl
The event is used to notify Gaia about the 'MessagesUpdate' request from MCE.
The 'MessagesUpdate' function enfores MSE Gaia application to fetech the latest messages from the network.
- BluetoothMapSendMessageEvent.webidl
The event is used to notify Gaia about the 'SendMessage' request from MCE.
The 'SendMessage' function requests MSE Gaia application to send the message to the network.
- BluetoothMapSetMessageStatusEvent.webidl
The event is used to notify Gaia about the 'SetMessageStatus' request from MCE.
The 'SetMessageStatus' function requests MSE Gaia application to set message status.
- BluetoothMapRequestHandle.webidl
The webidl defines a handle which is used to reply to the MAP requests.
- BluetoothMapParameters.webidl
The webidl defines few enumerations that MAP needs.
I also added three event handlers to |BluetoothAdapter.webidl| for handling MAP requests.
If you you need more details about MAP, please feel free to let me know.
[1] Introduction of MAP
https://developer.bluetooth.org/TechnologyOverview/Pages/MAP.aspx
[2] MAP 1.2 spec.
https://www.bluetooth.org/DocMan/handlers/DownloadDoc.ashx?doc_id=274479
Attachment #8670746 -
Flags: review?(mrbkap)
Comment 21•9 years ago
|
||
Comment on attachment 8670746 [details] [diff] [review]
Bug 1184017 - [MAP] Dispatch events to MAP event handlers, r=btian (m-c)
Review of attachment 8670746 [details] [diff] [review]:
-----------------------------------------------------------------
I have a couple of comments that need addressing before I sr this.
::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
@@ +760,5 @@
> + uint32_t filterMessageType = *((uint8_t *)buf);
> +
> + if (filterMessageType == (FILTER_NO_EMAIL | FILTER_NO_MMS |
> + FILTER_NO_SMS_GSM) || (FILTER_NO_EMAIL |
> + FILTER_NO_MMS | FILTER_NO_SMS_CDMA)) {
This looks wrong and will always always test as true. It looks like you intended to write:
if (filterMessageType == (FILTER_NO_EMAIL | FILTER_NO_MMS | FILTER_NO_SMS_GSM) ||
filterMessageType == (FILTER_NO_EMAIL | FILTER_NO_MMS | FILTER_NO_SMS_CDMA)) {
::: dom/webidl/BluetoothMapRequestHandle.webidl
@@ +10,5 @@
> + * Reply to Folder-Listing object for MAP request. The DOMRequest will be rejected if
> + * the MAP request operation fails.
> + */
> + [NewObject, Throws, AvailableIn=CertifiedApps]
> + DOMRequest replyToFolderListing(long masId, DOMString folders);
I know that the Bluetooth APIs are a mix of DOMRequest and promises, but new interfaces should probably use promises since we're eventually hoping to phase out DOMRequest (promises also have the advantage that they declare right in the IDL what is returned through the promise).
Attachment #8670746 -
Flags: review?(mrbkap)
Assignee | ||
Updated•9 years ago
|
Whiteboard: [red-tai]
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #21)
> Comment on attachment 8670746 [details] [diff] [review]
> Bug 1184017 - [MAP] Dispatch events to MAP event handlers, r=btian (m-c)
>
> Review of attachment 8670746 [details] [diff] [review]:
> -----------------------------------------------------------------
> This looks wrong and will always always test as true. It looks like you
> intended to write:
>
> if (filterMessageType == (FILTER_NO_EMAIL | FILTER_NO_MMS |
> FILTER_NO_SMS_GSM) ||
> filterMessageType == (FILTER_NO_EMAIL | FILTER_NO_MMS |
> FILTER_NO_SMS_CDMA)) {
Thanks for catching this.
>
> ::: dom/webidl/BluetoothMapRequestHandle.webidl
> @@ +10,5 @@
> > + * Reply to Folder-Listing object for MAP request. The DOMRequest will be rejected if
> > + * the MAP request operation fails.
> > + */
> > + [NewObject, Throws, AvailableIn=CertifiedApps]
> > + DOMRequest replyToFolderListing(long masId, DOMString folders);
>
> I know that the Bluetooth APIs are a mix of DOMRequest and promises, but new
> interfaces should probably use promises since we're eventually hoping to
> phase out DOMRequest (promises also have the advantage that they declare
> right in the IDL what is returned through the promise).
I see. I originally thought that I shall follow the style of BluetoothPbapRequestHandle, so I choose DOMRequest. I will change the patch to return Promise.
Assignee | ||
Updated•9 years ago
|
Attachment #8670746 -
Attachment is obsolete: true
Assignee | ||
Comment 23•9 years ago
|
||
Fix Comment 21 addressed:
1. Fix filterMessageType case.
2. Change DOMRequest to Promise.
Attachment #8671160 -
Flags: review?(mrbkap)
Assignee | ||
Updated•9 years ago
|
Attachment #8671160 -
Attachment is obsolete: true
Attachment #8671160 -
Flags: review?(mrbkap)
Assignee | ||
Comment 24•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8671183 -
Attachment is obsolete: true
Assignee | ||
Comment 25•9 years ago
|
||
Fix Comment 21 addressed:
1. Fix filterMessageType case.
2. Change DOMRequest to Promise.
3. Remove DOMRequest/BlobSet file header
Attachment #8671187 -
Flags: review?(mrbkap)
Comment 26•9 years ago
|
||
Comment on attachment 8671187 [details] [diff] [review]
Bug 1184017 - [MAP] Dispatch events to MAP event handlers, r=btian
Review of attachment 8671187 [details] [diff] [review]:
-----------------------------------------------------------------
Yeah, I should have remembered for the pbap stuff as well. I'm sorry about that. This looks good. Thanks for addressing my comments so quickly.
Attachment #8671187 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 27•9 years ago
|
||
Thanks, Blake.
Push to try server again to check change in Comment 25.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb91e3caed94
Assignee | ||
Comment 29•9 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #27)
> Thanks, Blake.
> Push to try server again to check change in Comment 25.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb91e3caed94
I checked busted test cases and testfailed cases are not related to my patch.
Assignee | ||
Comment 30•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S9 (16Oct)
Assignee | ||
Comment 33•9 years ago
|
||
Flags: needinfo?(shuang)
Assignee | ||
Comment 34•9 years ago
|
||
It took me some time to re-base.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cbook)
Comment 35•9 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #34)
> It took me some time to re-base.
np :) landed on 2.2r
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/3fd115de2bc5
status-b2g-v2.2r:
--- → fixed
Flags: needinfo?(cbook)
You need to log in
before you can comment on or make changes to this bug.
Description
•