1186840 - [MAP] Implement MessageUpdate function

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug1186840-mc.patch

Comment 2

[Ben Tian (inactive)]

Revise flag to feature-b2g: 2.2r+ since MAP is required bluetooth feature for 2.2r.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1186840 - [MAP] Implement MessageUpdate function

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8686009 [details] [diff] [review]
Bug 1186840 - [MAP] Implement MessageUpdate function

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

Please revise code flow to ensure all PUT/GET paths reply to client.

::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
@@ +364,5 @@
>            HandleSetMessageStatus(pktHeaders);
>          } else if (type.EqualsLiteral("x-bt/message")) {
>            HandleSmsMmsPushMessage(pktHeaders);
> +        } else if (type.EqualsLiteral("x-bt/MAP-messageUpdate")) {
> +          HandleSmsMmsUpdateInbox(pktHeaders);

The reply should replace |ReplyToPut| in [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp#330

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Ben Tian [:btian] from comment #4)
> Comment on attachment 8686009 [details] [diff] [review]
> Bug 1186840 - [MAP] Implement MessageUpdate function
> 
> Review of attachment 8686009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please revise code flow to ensure all PUT/GET paths reply to client.
> 
> ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
> @@ +364,5 @@
> >            HandleSetMessageStatus(pktHeaders);
> >          } else if (type.EqualsLiteral("x-bt/message")) {
> >            HandleSmsMmsPushMessage(pktHeaders);
> > +        } else if (type.EqualsLiteral("x-bt/MAP-messageUpdate")) {
> > +          HandleSmsMmsUpdateInbox(pktHeaders);
> 
> The reply should replace |ReplyToPut| in [1].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/
> BluetoothMapSmsManager.cpp#330

Ben,
Thanks. But after revisit the code again, I found current implementation did not handle Put multiple packets. This is really bad!

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug1186840-mc.patch

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8692845 [details] [diff] [review]
bug1186840-mc.patch

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

Please see my comment below.

::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
@@ +351,5 @@
>          return;
>        }
>  
> +      // Multi-packet PUT request (0x02) may not contain Type header
> +      if (opCode == ObexRequestCode:: PutFinal &&

Remove this section first and consider only single PUT packet in this bug. We can revisit condition here in the follow-up bug(s) for multi-packet PUT request.

@@ +362,1 @@
>        if (pktHeaders.Has(ObexHeaderId::Type)) {

Revise with guardian clause as Get:

  // TODO: Handle multi-packet PUT request
  if (!pktHeaders.Has(ObexHeaderId::Type)) {
    SendReply(ObexResponseCode::BadRequest);
    return;
  }

  pktHeaders.GetContentType(type);
  ...

@@ +376,5 @@
> +           * with a 'Not implemented' error response.
> +           */
> +          SendReply(ObexResponseCode::NotImplemented);
> +        } else {
> +          BT_LOGR("Not supported type: %s", NS_ConvertUTF16toUTF8(type).get());

Revise log as Get case

  "Unknown MAP PUT request type %s", ...

@@ +850,5 @@
>    }
>  
>    // Section 3.3.3.2 "PutResponse", IrOBEX 1.2
>    // [opcode:1][length:2][Headers:var]
> +  uint8_t req[3];

Replace 3 with |kObexRespHeaderSize|.

https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/ObexBase.h

@@ +1532,5 @@
>  BluetoothMapSmsManager::HandleSetMessageStatus(const ObexHeaderSet& aHeader)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  ReplyToPut();

I think we should reply after gaia triggers |ReplyToSetMessageStatus|[1]. Please confirm the timing to reply to PUT for the 4 cases [2], and call |ReplyToPut| outside these handling methods.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp#333

::: dom/bluetooth/common/BluetoothCommon.h
@@ +262,5 @@
>   */
>  #define MAP_MESSAGES_LISTING_REQ_ID          "mapmessageslistingreq"
>  #define MAP_GET_MESSAGE_REQ_ID               "mapgetmessagereq"
>  #define MAP_SET_MESSAGE_STATUS_REQ_ID        "mapsetmessagestatusreq"
> +#define MAP_PUSH_MESSAGE_REQ_ID              "mapsendmessagereq"

Rename to MAP_SEND_MESSAGE_REQ_ID to conform with the string.

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1186840 - [MAP] Implement MessageUpdate function

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1186840 - [MAP] Implement MessageUpdate function

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8692905 [details] [diff] [review]
Bug 1186840 - [MAP] Implement MessageUpdate function

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

::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
@@ +339,5 @@
>          ReplyToPut();
> +      } else if (type.EqualsLiteral("x-bt/messageStatus")) {
> +        HandleSetMessageStatus(pktHeaders);
> +      } else if (type.EqualsLiteral("x-bt/message")) {
> +        HandleSmsMmsPushMessage(pktHeaders);

Where do you reply to PUT for message pushing request?

@@ +347,5 @@
> +         * with a 'Not implemented' error response.
> +         */
> +        SendReply(ObexResponseCode::NotImplemented);
> +      } else {
> +        BT_LOGR("Unknown MAP PUT type: %s", NS_ConvertUTF16toUTF8(type).get());

Unknown MAP PUT request type

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Ben Tian [:btian] from comment #10)
> Comment on attachment 8692905 [details] [diff] [review]
> Bug 1186840 - [MAP] Implement MessageUpdate function
> 
> Review of attachment 8692905 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
> @@ +339,5 @@
> >          ReplyToPut();
> > +      } else if (type.EqualsLiteral("x-bt/messageStatus")) {
> > +        HandleSetMessageStatus(pktHeaders);
> > +      } else if (type.EqualsLiteral("x-bt/message")) {
> > +        HandleSmsMmsPushMessage(pktHeaders);
> 
> Where do you reply to PUT for message pushing request?

In ReplyToSendMessage().

> 
> @@ +347,5 @@
> > +         * with a 'Not implemented' error response.
> > +         */
> > +        SendReply(ObexResponseCode::NotImplemented);
> > +      } else {
> > +        BT_LOGR("Unknown MAP PUT type: %s", NS_ConvertUTF16toUTF8(type).get());
> 
> Unknown MAP PUT request type

Ok.

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1186840 - [MAP] Implement MessageUpdate function

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8693442 [details] [diff] [review]
Bug 1186840 - [MAP] Implement MessageUpdate function

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
@@ +325,5 @@
>        }
>  
> +      // Multi-packet PUT request (0x02) may not contain Type header
> +      if (!pktHeaders.Has(ObexHeaderId::Type)) {
> +        BT_LOGR("Missing OBEX Type header");

Conform code in both PUT and GET requests: either print logs of no type header and type for both requests, or none.

Comment 14

[Ben Tian (inactive)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> > Where do you reply to PUT for message pushing request?
> 
> In ReplyToSendMessage().

I missed it. Thanks for informing.

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Ben Tian [:btian] from comment #13)
> Comment on attachment 8693442 [details] [diff] [review]
> Bug 1186840 - [MAP] Implement MessageUpdate function
> 
> Review of attachment 8693442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comment addressed.
> 
> ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp
> @@ +325,5 @@
> >        }
> >  
> > +      // Multi-packet PUT request (0x02) may not contain Type header
> > +      if (!pktHeaders.Has(ObexHeaderId::Type)) {
> > +        BT_LOGR("Missing OBEX Type header");
> 
> Conform code in both PUT and GET requests: either print logs of no type
> header and type for both requests, or none.

I will add log for both GET and PUT.

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1186840 - [MAP] Implement MessageUpdate function, r=btian

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/07f6fbbd3b2f

Comment 18

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=07f6fbbd3b2f

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/07f6fbbd3b2f