Closed
Bug 1186840
Opened 9 years ago
Closed 8 years ago
[MAP] Implement MessageUpdate function
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
Tracking
(feature-b2g:2.2r+, firefox45 fixed)
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: shawnjohnjr, Assigned: shawnjohnjr)
References
Details
Attachments
(1 file, 6 obsolete files)
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
blocking-b2g: --- → 2.2r?
Comment 2•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
|
Assignee | ||
Updated•9 years ago
|
Attachment #8649283 -
Attachment is obsolete: true
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8686009 -
Flags: review?(btian)
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
(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!
Assignee | ||
Updated•9 years ago
|
Attachment #8686009 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8692845 -
Flags: review?(btian)
Comment 7•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8692845 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8692903 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8692905 -
Flags: review?(btian)
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Attachment #8692905 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8693442 -
Flags: review?(btian)
Comment 13•8 years ago
|
||
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+
Comment 14•8 years ago
|
||
(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.
Assignee | ||
Comment 15•8 years ago
|
||
(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.
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8693442 -
Attachment is obsolete: true
Assignee | ||
Comment 18•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=07f6fbbd3b2f
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/07f6fbbd3b2f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox45:
--- → fixed
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.
Description
•