Closed Bug 1186840 Opened 9 years ago Closed 8 years ago

[MAP] Implement MessageUpdate function

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.2r+, firefox45 fixed)

RESOLVED FIXED
2.6 S2 - 12/4
feature-b2g 2.2r+
Tracking Status
firefox45 --- fixed

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

Attachments

(1 file, 6 obsolete files)

Assignee: nobody → shuang
Depends on: 1195710
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+
Depends on: 1215924
No longer depends on: 1195710
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
Attachment #8686009 - Flags: review?(btian)
(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 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.
Attachment #8692845 - Flags: review?(btian)
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
Attachment #8692905 - Flags: review?(btian)
(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 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.
Attachment #8693442 - Flags: review?(btian) → review+
(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.
(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.
Blocks: 1196125
https://hg.mozilla.org/mozilla-central/rev/07f6fbbd3b2f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.6 S2 - 12/4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: