Closed Bug 1184017 Opened 9 years ago Closed 9 years ago

[MAP] Dispatch events to MAP event handlers

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(feature-b2g:2.2r+, firefox44 fixed, b2g-v2.2r fixed)

Status:
RESOLVED FIXED
Milestone:
FxOS-S9 (16Oct)
Project Flags:
feature-b2g 2.2r+
Tracking Flags:
Tracking Status
firefox44 --- fixed
b2g-v2.2r --- fixed

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

(Whiteboard: [red-tai])

Attachments

(2 files, 15 obsolete files)

Bug 1184017 - [MAP] Dispatch events to MAP event handlers, r=btian
57.84 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
Bug 1184017 - [MAP] Dispatch events to MAP event handlers, r=btian, sr=mrbkap (v2.2r)
55.47 KB, patch
Details | Diff | Splinter Review
Assignee: nobody → shuang
Attached patch bug1184017-mc.patch (WIP) (obsolete) — — Splinter Review 
Revised patch for ReplyTo* api in RequestHandle.
Attached patch bug1184017-mc.patch (WIP) (obsolete) — — Splinter Review 
Add BluetoothMapFolderListingEvent.
Attached patch bug1184017-mc.patch (WIP) (obsolete) — — Splinter Review 
Add GetMessageEvent, FolderListingEvent, SendMessageEvent, SetMessageStatusEvent
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
blocking-b2g: --- → 2.2r?
Revise flag to feature-b2g: 2.2r+ since MAP is required bluetooth feature for 2.2r.
blocking-b2g: 2.2r? → ---
feature-b2g: --- → 2.2r+
Attached patch bug1184017-mc.patch (WIP) (obsolete) — — Splinter Review 
Rebase to the latest m-c.
Attachment #8669699 - Attachment description: bug1184017-mc.patch (WIP) → Bug 1184017 - [MAP] Dispatch events to MAP event handlers
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+
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)?
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)
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 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)
(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.
Fix Comment 21 addressed:
1. Fix filterMessageType case.
2. Change DOMRequest to Promise.
Attachment #8671160 - Flags: review?(mrbkap)
Attachment #8671160 - Attachment is obsolete: true
Attachment #8671160 - Flags: review?(mrbkap)
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 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+
(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.
https://hg.mozilla.org/mozilla-central/rev/16d10dba95ee
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S9 (16Oct)
shawn, we need a 2.2r PR request here too or ?
Flags: needinfo?(shuang)
It took me some time to re-base.
Flags: needinfo?(cbook)
(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.

Attachment

General

Creator:
Created:
Updated:
Size: