Closed
Bug 1166647
Opened 9 years ago
Closed 9 years ago
Implement MAP bMessage class (bMessage builder/parser)
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: needs-uplift)
Attachments
(7 files, 14 obsolete files)
Implement MAP bMessage class (bMessage builder/parser)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → shuang
Assignee | ||
Comment 1•9 years ago
|
||
I got some sample here for bMessage parsing clarification. Testing data dumped from MCE.
Assignee | ||
Comment 2•9 years ago
|
||
Ok, now i have some further progress to work on bMessage parser. I tried to BlueZ 5.33 and run test/map-client to test PushMessage. "./map-client -d <<bd_addr>> -c /telecom/msg/outbox -p /home/shawnjohnjr/receivedBMessage.txt"
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8656002 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
bMsg/Vcard parsing done.
Assignee | ||
Updated•9 years ago
|
Attachment #8657121 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Add PushMessage code to use bMsg/vcard parser.
Assignee | ||
Updated•9 years ago
|
Attachment #8657568 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8657714 -
Flags: review?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8657714 -
Attachment is obsolete: true
Attachment #8657714 -
Flags: review?(btian)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8657782 -
Flags: review?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8657782 -
Attachment is obsolete: true
Attachment #8657782 -
Flags: review?(btian)
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8658036 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Fix nits and style issue.
Attachment #8658037 -
Flags: review?(btian)
Comment 10•9 years ago
|
||
Comment on attachment 8658037 [details] [diff] [review] Bug 1166647 - Implement MAP bMessage class (bMessage builder/parser) Review of attachment 8658037 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.cpp @@ +15,5 @@ > +BluetoothMapBMessage::~BluetoothMapBMessage() > +{} > + > +//static > +//BluetoothMapBMessage* Remove the unused code. @@ +28,5 @@ > + > +// On input, nextLineStart is the start of the current line. On output, > +// nextLineStart is the start of the next line. > +int > +ReadLine(/*in/out*/ const char* & nextLineStart, /*out*/ nsCString & line) Is it static function? Mark it if so. Also rename input argument as |aNexLineStart| and |aLine|. @@ +57,5 @@ > + } > +} > + > +bool > +ExtractParameter(const nsAutoCString& aCurrLine, Mark static function if it is. @@ +61,5 @@ > +ExtractParameter(const nsAutoCString& aCurrLine, > + const char* aPattern, nsCString& aParam) > +{ > + aParam.Truncate(); > + if(aCurrLine.Find(aPattern, false, 0, PL_strlen(aPattern)) != kNotFound) { nit: space after if @@ +65,5 @@ > + if(aCurrLine.Find(aPattern, false, 0, PL_strlen(aPattern)) != kNotFound) { > + int32_t colonPos = aCurrLine.FindChar(':'); > + aParam = nsDependentCSubstring(aCurrLine, colonPos + 1, aCurrLine.Length()); > + return true; > + } else { Remove else and return false directly. @@ +71,5 @@ > + } > +} > + > +/* > + * ProcessDecode parse bMessage following MAP 3.1.3 Message Format. nit: |ProcessDecode| parses bMessage based on following MAP ... @@ +134,5 @@ > +BluetoothMapBMessage::ParseBeginBmsg(const nsAutoCString& aCurrLine) > +{ > + if (aCurrLine.EqualsLiteral("BEGIN:BMSG")) { > + mState = BMSG_PARSING_STATE_VERSION; > + return; Remove the redundant return. @@ +141,5 @@ > + > +void > +BluetoothMapBMessage::ParseVersion(const nsAutoCString& aCurrLine) > +{ > + if(aCurrLine.Find("VERSION:") != kNotFound) { nit: space after if @@ +147,5 @@ > + nsCString version; > + version += nsDependentCSubstring(aCurrLine, colonPos + 1, > + aCurrLine.Length()); > + if (version.EqualsLiteral("1.0")) { > + mState = BMSG_PARSING_STATE_STATUS; Why is it STATE_STATUS instead of STATE_VERSION? @@ +148,5 @@ > + version += nsDependentCSubstring(aCurrLine, colonPos + 1, > + aCurrLine.Length()); > + if (version.EqualsLiteral("1.0")) { > + mState = BMSG_PARSING_STATE_STATUS; > + return; Remove the redundant return. @@ +158,5 @@ > +BluetoothMapBMessage::ParseStatus(const nsAutoCString& aCurrLine) > +{ > + nsCString status; > + ExtractParameter(aCurrLine, "STATUS:", status); > + if (status.EqualsLiteral("READ")) { Is there other possibility beside READ and UNREAD? If no, simplify as following: mReadStatus = status.EqualsLiteral("READ"); @@ +190,5 @@ > + > +void > +BluetoothMapBMessage::ParseOriginator(const nsAutoCString& aCurrLine) > +{ > + if(aCurrLine.Find("BEGIN:VCARD") != kNotFound) { nit: space after if. @@ +197,5 @@ > + mOriginators.AppendElement(vcard); > + > + mUnwindState = BMSG_PARSING_STATE_ORIGINATOR; > + } else { > + if (aCurrLine.EqualsLiteral("BEGIN:BENV")) { nit: combine else and if. } else if (aCurrLine.EqualsLiteral("BEGIN:BENV")) { mState = BMSG_PARSING_STATE_BEGIN_ENVELOPE; } @@ +206,5 @@ > + > +void > +BluetoothMapBMessage::ParseVcard(const nsAutoCString& aCurrLine) > +{ > + if(aCurrLine.EqualsLiteral("END:VCARD")) { nit: space after if @@ +210,5 @@ > + if(aCurrLine.EqualsLiteral("END:VCARD")) { > + mState = mUnwindState; > + return; > + } > + if (mUnwindState == BMSG_PARSING_STATE_ORIGINATOR && nit: newline before this line. @@ +211,5 @@ > + mState = mUnwindState; > + return; > + } > + if (mUnwindState == BMSG_PARSING_STATE_ORIGINATOR && > + mOriginators.Length() >= 1) { Replace with |!mOriginators.IsEmpty()|. @@ +214,5 @@ > + if (mUnwindState == BMSG_PARSING_STATE_ORIGINATOR && > + mOriginators.Length() >= 1) { > + mOriginators.LastElement()->Parse(aCurrLine); > + } else if (mUnwindState == BMSG_PARSING_STATE_RECIPIENT && > + mRecipients.Length() >= 1) { Replace with |!mRecipients.IsEmpty()|. @@ +222,5 @@ > + > +void > +BluetoothMapBMessage::ParseEnvelope(const nsAutoCString& aCurrLine) > +{ > + if(!aCurrLine.EqualsLiteral("BEGIN:VCARD")) { nit: space after if @@ +231,5 @@ > + if (aCurrLine.EqualsLiteral("BEGIN:VCARD")) { > + nsRefPtr<VCard> vcard = new VCard(); > + mRecipients.AppendElement(vcard); > + mState = BMSG_PARSING_STATE_VCARD; > + // In case there are many recipents nit: remove extra space after many @@ +246,5 @@ > + > +void > +BluetoothMapBMessage::ParseRecipient(const nsAutoCString& aCurrLine) > +{ > + // After parsing VCard, check it's bMessage BODY or VCARD. nit: check whether it's bMessage BODY or VCARD. @@ +268,5 @@ > + nsCString encoding; > + if (ExtractParameter(aCurrLine, "ENCODING:", encoding)) { > + mEncoding = encoding; > + return; > + } nit: newline after } @@ +273,5 @@ > + nsCString charset; > + if (ExtractParameter(aCurrLine, "CHARSET:", charset)) { > + mCharset = charset; > + return; > + } nit: newline after } @@ +278,5 @@ > + nsCString language; > + if (ExtractParameter(aCurrLine, "LANGUAGE:", language)) { > + mLanguage = language; > + return; > + } nit: newline after } ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.h @@ +83,5 @@ > + void ParseBMsg(const nsAutoCString& aCurrLine); > + > + // Read/UnRead > + bool mReadStatus; > + // Indicate bMessage is located in Indicate bMessage location @@ +85,5 @@ > + // Read/UnRead > + bool mReadStatus; > + // Indicate bMessage is located in > + nsCString mFolderName; > + // If the content canot be delivered in one bmessage-content object What does the comment mean? Why is it a string rather than a boolean? @@ +89,5 @@ > + // If the content canot be delivered in one bmessage-content object > + nsCString mPartId; > + // The encoding used by bmessage-body-content if the content is binary > + nsCString mEncoding; > + // The character-set used by contained by bmessage-body-content if the content "used by" or "contained by"? @@ +109,5 @@ > + enum BMsgParserState mUnwindState; > + // Current level of bmessage-envelope. > + int mEnvelopeLevel; > + // Based on the formal BNF definition of the bMessage format, the originator > + // and recipients can be presented 0 or many times nit: and recipients may present 0 or many times. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +990,5 @@ > > AppendBtNamedValueByTagId(aHeader, data, > Map::AppParametersTagId::Transparent); > AppendBtNamedValueByTagId(aHeader, data, Map::AppParametersTagId::Retry); > + /* TODO: Support native format charset (mandatory format). nit: revise comment to be clearer /* TODO: Support native format charset (mandatory format). * * Charset indicates Gaia application how to deal with encoding. * - Native: If the message object shall be delivered without trans-coding. * - UTF-8: If the message text shall be trans-coded to UTF-8. * * We only support UTF-8 charset due to current SMS API limitation. */ @@ +1001,5 @@ > > + uint8_t* bodyPtr = nullptr; > + // Get Body > + if (aHeader.Has(ObexHeaderId::Body) || > + aHeader.Has(ObexHeaderId::EndOfBody)) { nit: alignment if (aHeader.Has(ObexHeaderId::Body) || aHeader.Has(ObexHeaderId::EndOfBody)) { @@ +1006,5 @@ > + aHeader.GetBody(&bodyPtr, &mBodySegmentLength); > + mBodySegment = bodyPtr; > + } > + > + /* FolderName is outbox: Revise comment to be clearer: /* If FolerName is outbox: * 1. Parse body to get SMS * 2. Get receipent subject * 3. Send it to Gaia * Otherwise reply HTTP_NOT_ACCEPTABLE. */ @@ +1022,5 @@ > + > + nsCString subject; > + bmsg->GetBody(subject); > + BT_APPEND_NAMED_VALUE(data, "subject", subject); > + nsCString recipient; nit: new line after BT_APPEND_NAMED_VALUE. @@ +1024,5 @@ > + bmsg->GetBody(subject); > + BT_APPEND_NAMED_VALUE(data, "subject", subject); > + nsCString recipient; > + // Get the topest level > + if (recipients.Length() > 0) { Replace with |!recipient.IsEmpty()| @@ +1027,5 @@ > + // Get the topest level > + if (recipients.Length() > 0) { > + recipients[0]->GetTelephone(recipient); > + } > + BT_APPEND_NAMED_VALUE(data, "recipient", recipient); We still append |recipient| even though it's empty string, right? nit: newline after BT_NAMED_VALUE. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.h @@ +8,5 @@ > #define mozilla_dom_bluetooth_bluedroid_BluetoothMapSmsManager_h > > #include "BluetoothCommon.h" > #include "BluetoothMapFolder.h" > +#include "BluetoothMapBMessage.h" nit: alphabetical order.
Attachment #8658037 -
Flags: review?(btian)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Ben Tian [:btian] from comment #10) > Comment on attachment 8658037 [details] [diff] [review] > Bug 1166647 - Implement MAP bMessage class (bMessage builder/parser) > > Review of attachment 8658037 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please see my comment below. > > @@ +147,5 @@ > > + nsCString version; > > + version += nsDependentCSubstring(aCurrLine, colonPos + 1, > > + aCurrLine.Length()); > > + if (version.EqualsLiteral("1.0")) { > > + mState = BMSG_PARSING_STATE_STATUS; > > Why is it STATE_STATUS instead of STATE_VERSION? MAP Specification v1.2, 3.1.3: bmessage-version-property: The value for this property shall be "VERSION:1.0 <CRLF>", which is the present bMessage version 1.0. If the validation pass, set parser state to next state which is status. So when |ReadLine| reads next line, execute |ParseStatus|. The parser is based on the following BNF. <bmessage-property>::=<bmessage-version-property> <bmessage-readstatus-property> <bmessage-type-property> <bmessage-folder-property> > @@ +85,5 @@ > > + // Read/UnRead > > + bool mReadStatus; > > + // Indicate bMessage is located in > > + nsCString mFolderName; > > + // If the content canot be delivered in one bmessage-content object > > What does the comment mean? Why is it a string rather than a boolean? It should be integer. I will change the comment and cast it to int.
Assignee | ||
Updated•9 years ago
|
Attachment #8658037 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8660433 -
Flags: review?(btian)
Comment 13•9 years ago
|
||
Comment on attachment 8660433 [details] [diff] [review] Bug 1166647 - Implement MAP bMessage class (bMessage builder/parser) (V2) Review of attachment 8660433 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. I'll take more time to look into bMessage parsing flow. Also please confirm comment 10 has been fully addressed. ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.cpp @@ +63,5 @@ > + if (aCurrLine.Find(aPattern, false, 0, PL_strlen(aPattern)) != kNotFound) { > + int32_t colonPos = aCurrLine.FindChar(':'); > + aParam = nsDependentCSubstring(aCurrLine, colonPos + 1, aCurrLine.Length()); > + return true; > + } else { Again, remove else and return false directly. @@ +69,5 @@ > + } > +} > + > +/* > + * ProcessDecode parses bMessage following MAP 3.1.3 Message Format. Again, nit: |ProcessDecode| parses bMessage based on following MAP ... @@ +115,5 @@ > + > + for (;;) { > + nsAutoCString curLine; > + int result = ReadLine(nextLineStart, curLine); > + if (result == -1) { Combine into if (ReadLine(nextLineStart, curLine) == -1) @@ +119,5 @@ > + if (result == -1) { > + // Cannot find eol symbol > + return; > + } > + if (curLine.Length() == 0) { curLine.IsEmpty() @@ +139,5 @@ > + > +void > +BluetoothMapBMessage::ParseVersion(const nsAutoCString& aCurrLine) > +{ > + if(aCurrLine.Find("VERSION:") != kNotFound) { nit: space after if Can we leverage |ExtractParameter| function? @@ +191,5 @@ > + mOriginators.AppendElement(vcard); > + > + mUnwindState = BMSG_PARSING_STATE_ORIGINATOR; > + } else if (aCurrLine.EqualsLiteral("BEGIN:BENV")) { > + mState = BMSG_PARSING_STATE_BEGIN_ENVELOPE; nit: 2-space indention ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.h @@ +13,5 @@ > +#include "nsTArray.h" > + > +BEGIN_BLUETOOTH_NAMESPACE > + > +class VCard; Remove this line since |VCard| is declared below. @@ +49,5 @@ > + ~VCard(); > +}; > + > +/* > + * This class represents as MAP bMessage. The bMessage object encapsulates nit: remove redundant 'as' @@ +50,5 @@ > +}; > + > +/* > + * This class represents as MAP bMessage. The bMessage object encapsulates > + * delivered message objects and provides additionally a suitable set offset Do you mean 'a suitable set of properties'? ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +1019,5 @@ > + * Otherwise reply HTTP_NOT_ACCEPTABLE > + */ > + > + nsRefPtr<BluetoothMapBMessage> bmsg = > + BluetoothMapBMessage::Parse(bodyPtr, mBodySegmentLength); Revise as following: nsRefPtr<BluetoothMapBMessage> bmsg = BluetoothMapBMessage::Parse(bodyPtr, mBodySegmentLength); nsCString subject; bmsg->GetBody(subject); AppendNamedValue(data, "subject", subject); nsTArray<nsRefPtr<VCard>> recipients; bmsg->GetRecipients(recipients); if (!recipients.IsEmpty()) { // Get the topmost level nsCString recipient; recipients[0]->GetTelephone(recipient); if (!recipient.IsEmpty()) { AppendNamedValue(data, "recipient", recipient); } } @@ +1027,5 @@ > + nsCString subject; > + bmsg->GetBody(subject); > + AppendNamedValue(data, "subject", subject); > + nsCString recipient; > + // Get the topest level nit: topmost.
Attachment #8660433 -
Flags: review?(btian)
Comment 14•9 years ago
|
||
Comment on attachment 8660433 [details] [diff] [review] Bug 1166647 - Implement MAP bMessage class (bMessage builder/parser) (V2) Review of attachment 8660433 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.h @@ +81,5 @@ > + void ParseRecipient(const nsAutoCString& aCurrLine); > + void ParseBody(const nsAutoCString& aCurrLine); > + void ParseBMsg(const nsAutoCString& aCurrLine); > + > + // Read/UnRead Use multi-line style comment and detail the usage of these member variables.
Assignee | ||
Updated•9 years ago
|
Attachment #8660433 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
Hi Ben, Sorry I missed a few comments to fix. I've updated patch again. Thanks for your patience.
Attachment #8661773 -
Flags: review?(btian)
Comment 16•9 years ago
|
||
Comment on attachment 8661773 [details] [diff] [review] Bug 1166647 - Implement MAP bMessage class (bMessage builder/parser) (V3) Review of attachment 8661773 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment below. ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.cpp @@ +8,5 @@ > +#include "plstr.h" > + > +BEGIN_BLUETOOTH_NAMESPACE > + > +BluetoothMapBMessage::BluetoothMapBMessage() Assign default values for member variables, except arrays and strings. @@ +26,5 @@ > + > +// On input, aNextLineStart is the start of the current line. On output, > +// aNextLineStart is the start of the next line. > +static int > +ReadLine(/*in/out*/ const char* & aNextLineStart, /*out*/ nsCString& aLine) Remove the comment here and state the input/output above if you want. /** * (Describe the function purpose, arguments, and return value ...) */ static int ReadLine(const char* & aNextLineStart, nsCString& aLine) == Also why not pass |const nsACString& aLine| but |nsCString&|? @@ +30,5 @@ > +ReadLine(/*in/out*/ const char* & aNextLineStart, /*out*/ nsCString& aLine) > +{ > + aLine.Truncate(); > + for (;;) { > + const char* eol = PL_strpbrk(aNextLineStart, "\r\n"); Define kMapCrlf = "\r\n" as [1] and reuse it below. [1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluez/BluetoothHfpManager.cpp#82 @@ +56,5 @@ > +} > + > +static bool > +ExtractParameter(const nsAutoCString& aCurrLine, > + const char* aPattern, nsCString& aParam) Ditto. Shall we replace |const nsACString&| with |nsCString&|? @@ +59,5 @@ > +ExtractParameter(const nsAutoCString& aCurrLine, > + const char* aPattern, nsCString& aParam) > +{ > + aParam.Truncate(); > + if (aCurrLine.Find(aPattern, false, 0, PL_strlen(aPattern)) != kNotFound) { Revise as following: if (aCurrLine.Find(aPattern, false, 0, PL_strlen(aPattern)) == kNotFound) { return false; } int32_t colonPos = aCurrLine.FindChar(':'); aParam = nsDependentCSubstring(aCurrLine, colonPos + 1, aCurrLine.Length()); return true; @@ +114,5 @@ > + }; > + > + for (;;) { > + nsAutoCString curLine; > + if (ReadLine(nextLineStart, curLine) == -1) { Pass |aBuf| directly and remove |nextLineStart|. @@ +118,5 @@ > + if (ReadLine(nextLineStart, curLine) == -1) { > + // Cannot find eol symbol > + return; > + } > + if (curLine.IsEmpty() == 0) { if (curLine.IsEmpty()) @@ +176,5 @@ > +{ > + nsCString folder; > + if (ExtractParameter(aCurrLine, "FOLDER:", folder)) { > + mFolderName = folder; > + } nit: newline after } @@ +186,5 @@ > +{ > + if (aCurrLine.EqualsLiteral("BEGIN:VCARD")) { > + mState = BMSG_PARSING_STATE_VCARD; > + nsRefPtr<VCard> vcard = new VCard(); > + mOriginators.AppendElement(vcard); Simplify to |mRecipients.AppendElement(new VCard())|. @@ +187,5 @@ > + if (aCurrLine.EqualsLiteral("BEGIN:VCARD")) { > + mState = BMSG_PARSING_STATE_VCARD; > + nsRefPtr<VCard> vcard = new VCard(); > + mOriginators.AppendElement(vcard); > + // It's possible to parse Vcard multiple times nit: use |vCard| in comment @@ +188,5 @@ > + mState = BMSG_PARSING_STATE_VCARD; > + nsRefPtr<VCard> vcard = new VCard(); > + mOriginators.AppendElement(vcard); > + // It's possible to parse Vcard multiple times > + mUnwindState = BMSG_PARSING_STATE_ORIGINATOR; Reorder this section as following: mRecipients.AppendElement(new VCard()); // We may parse vCard multiple times mUnwindState = mState; mState = BMSG_PARSING_STATE_VCARD; @@ +206,5 @@ > + if (mUnwindState == BMSG_PARSING_STATE_ORIGINATOR && > + !mOriginators.IsEmpty()) { > + mOriginators.LastElement()->Parse(aCurrLine); > + } else if (mUnwindState == BMSG_PARSING_STATE_RECIPIENT && > + mRecipients.Length() >= 1) { Replace with |!mRecipients.IsEmpty()|. @@ +214,5 @@ > + > +void > +BluetoothMapBMessage::ParseEnvelope(const nsAutoCString& aCurrLine) > +{ > + if(!aCurrLine.EqualsLiteral("BEGIN:VCARD")) { nit: space after if @@ +219,5 @@ > + mState = BMSG_PARSING_STATE_BEGIN_BODY; > + return; > + } > + > + if (aCurrLine.EqualsLiteral("BEGIN:VCARD")) { Remove the if since the condition is checked above. @@ +221,5 @@ > + } > + > + if (aCurrLine.EqualsLiteral("BEGIN:VCARD")) { > + nsRefPtr<VCard> vcard = new VCard(); > + mRecipients.AppendElement(vcard); Simplify to mRecipients.AppendElement(new VCard()); @@ +223,5 @@ > + if (aCurrLine.EqualsLiteral("BEGIN:VCARD")) { > + nsRefPtr<VCard> vcard = new VCard(); > + mRecipients.AppendElement(vcard); > + mState = BMSG_PARSING_STATE_VCARD; > + // In case there are many recipents nit: recipients @@ +228,5 @@ > + mUnwindState = BMSG_PARSING_STATE_RECIPIENT; > + return; > + } > + > + if (aCurrLine.EqualsLiteral("BEGIN:BENV")) { This section won't run since the case is early returned above. Please review and correct logic of this function. @@ +238,5 @@ > + > +void > +BluetoothMapBMessage::ParseRecipient(const nsAutoCString& aCurrLine) > +{ > + /* After parsing VCard, check whether it's bMessage BODY or VCARD. nit: vCard @@ +239,5 @@ > +void > +BluetoothMapBMessage::ParseRecipient(const nsAutoCString& aCurrLine) > +{ > + /* After parsing VCard, check whether it's bMessage BODY or VCARD. > + * It's possible that bmessage-recipient appears more than one time. Simplify to bmessage-recipient may appear more than once. @@ +265,5 @@ > + */ > +void > +BluetoothMapBMessage::ParseBody(const nsAutoCString& aCurrLine) > +{ > + nsCString partId; Simplify with else-if: nsCString param; if (ExtractParameter(aCurrLine, "PARTID:", param)) { nsresult rv; mPartId = param.ToInteger(&rv); } else if (ExtractParameter(aCurrLine, "ENCODING:", param)) { mEncoding = param; } ... } else if (aCurrLine.EqualsLiteral("BEGIN:MSG")) { // Parse <bmessage-body-content> mState = BMSG_PARSING_STATE_BEGIN_MSG; } @@ +318,5 @@ > + * For SMS: currently only UTF-8 is supported for textual content. > + */ > + if (aCurrLine.EqualsLiteral("END:MSG") || > + aCurrLine.EqualsLiteral("BEGIN:MSG")) { > + /* Set state to STATE_BEGIN_MSG due to <bmessage-body-content> may appears nit: may appear @@ +327,5 @@ > + } > + > + mMessageBody += aCurrLine.get(); > + // restore <CRLF> > + mMessageBody.AppendLiteral("\r\n"); Reuse |kMapCrlf| here. @@ +352,5 @@ > +void > +BluetoothMapBMessage::Dump() > +{ > + BT_LOGR("Dump: message body %s", mMessageBody.get()); > + BT_LOGR("Dump: read status %s", mReadStatus? "true":"false"); nit: space before and after : "true" : "false" @@ +370,5 @@ > + > +void > +VCard::Parse(const nsAutoCString& aCurrLine) > +{ > + nsCString name; Simplify with else-if: nsCString param; if (ExtractParameter(aCurrLine, "N:", param)) { mName = param; } else if (ExtractParameter(aCurrLine, "FN:", param)) { mFormattedName = param; } else if (ExtractParameter(aCurrLine, "TEL:", param)) { mTelephone = telephone; } else if (ExtractParameter(aCurrLine, "Email:", param)) { mEmail = email; } @@ +424,5 @@ > +VCard::Dump() > +{ > + BT_LOGR("Dump: Name %s", mName.get()); > + BT_LOGR("Dump: FormattedName %s", mFormattedName.get()); > + BT_LOGR("Dump: Tele %s", mTelephone.get()); Replace 'Tele' with 'Telephone'. ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.h @@ +42,5 @@ > + > +private: > + nsCString mName; > + nsCString mFormattedName; > + nsCString mTelephone; nit: order member variables as |Get*| functions. @@ +74,5 @@ > + void ParseStatus(const nsAutoCString& aCurrLine); > + void ParseType(const nsAutoCString& aCurrLine); > + void ParseFolder(const nsAutoCString& aCurrLine); > + void ParseOriginator(const nsAutoCString& aCurrLine); > + void ParseVcard(const nsAutoCString& aCurrLine); Rename to |ParseVCard| to conform with class |VCard|. @@ +80,5 @@ > + void ParseRecipient(const nsAutoCString& aCurrLine); > + void ParseBody(const nsAutoCString& aCurrLine); > + void ParseBMsg(const nsAutoCString& aCurrLine); > + > + /* ReadStatus property could be either "READ" or "UNREAD". Indicating whether Simplify as following: /* Indicate whether a message has been read by MCE or not. * MCE shall be responsible to set this status. */ @@ +88,5 @@ > + bool mReadStatus; > + // Indicate bMessage location > + nsCString mFolderName; > + > + /* If the content canot be delivered in one bmessage-content object, Part-ID Revise as following: /* Part-ID is used when the content cannot be delivered in one bmessage-content object. * The first bmessage-content object's part-ID shall be 0, and the following ones have * part-ID incremented by 1 each. */ @@ +97,5 @@ > + int mPartId; > + // The encoding used by bmessage-body-content if the content is binary > + nsCString mEncoding; > + > + /* The character-set would be used in bmessage-body-content if the content nit: remove redundant 'would be' @@ +104,5 @@ > + nsCString mCharset; > + // mLanguage may be used if the message includes textual content > + nsCString mLanguage; > + > + /* The length of the bmessage-body-content, starts with the "B" of the first /* Length of the bmessage-body-content, starting ... and ending ... @@ +105,5 @@ > + // mLanguage may be used if the message includes textual content > + nsCString mLanguage; > + > + /* The length of the bmessage-body-content, starts with the "B" of the first > + * occurrence of "BEGIN_BMSG" and ends with the <CRLF> of the last occurrence nit: one 'of the last occurrence' is redundant, right? @@ +110,5 @@ > + * of the last occurrence of "END:MSG"<CRLF>. > + */ > + nsCString mBMsgLength; > + // mMessageBody represents bmessage-body-content > + nsCString mMessageBody; Rename to |mBMsgBody| for consistency. @@ +123,5 @@ > + * encapsulation shall be 3. > + */ > + int mEnvelopeLevel; > + > + /* Based on the formal BNF definition of the bMessage format, the originator nit: originators Revise as following: /* Based on the formal BNF definition of the bMessage format, originators * and recipients may appear 0 or more times. * * |mOriginators| represents the original sender of the messages, and * |mRecipients| represents the recipient of the message. */ @@ +128,5 @@ > + * and recipients may present 0 or many times. mOriginators represents the > + * original sender of the message. > + */ > + nsTArray<nsRefPtr<VCard>> mOriginators; > + // mRecipients represents the recipient of the message Remove this line based on comment above. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +728,1 @@ > subLength = (subLength >> 8) | (subLength << 8); Shall we remove this conversion as well? Note bug 1206116 comment 0 points out this conversion is incorrect.
Attachment #8661773 -
Flags: review?(btian)
Comment 17•9 years ago
|
||
Comment on attachment 8661773 [details] [diff] [review] Bug 1166647 - Implement MAP bMessage class (bMessage builder/parser) (V3) Review of attachment 8661773 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for removing the bogus endian conversions. One small issue still below. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ -733,5 @@ > case Map::AppParametersTagId::ParameterMask: { > // 4 bytes > uint32_t parameterMask = *((uint32_t *)buf); > - // convert big endian to little endian > - parameterMask = (parameterMask >> 8) | (parameterMask << 8); Removing this doesn't seem completely right, unless parameterMask is stored in native-endian in buf (which, judging from other values, seems unlikely).
Assignee | ||
Updated•9 years ago
|
Attachment #8661773 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
Changes since v3 * Fix issues addressed in Comment 16 and Comment 17.
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Comment 20•9 years ago
|
||
Assignee | ||
Comment 21•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8664814 -
Flags: review?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8664817 -
Attachment is obsolete: true
Assignee | ||
Comment 22•9 years ago
|
||
Add list-messages with parameter test.
Assignee | ||
Comment 23•9 years ago
|
||
Comment 24•9 years ago
|
||
Comment on attachment 8664814 [details] [diff] [review] Bug 1166647 - Implement MAP bMessage class (bMessage builder/parser) (V4) Review of attachment 8664814 [details] [diff] [review]: ----------------------------------------------------------------- Almost there. Please see my few questions below. ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.cpp @@ +9,5 @@ > + > +BEGIN_BLUETOOTH_NAMESPACE > + > +namespace { > + static const char kHfpCrlf[] = "\r\n"; Rename to |kMapCrlf|. @@ +38,5 @@ > + * > + * @param aNextLineStart [in][out] On input, aNextLineStart is the start of > + * the current line. On output, aNextLineStart is the start of the next > + * line. > + * @param aLine [out] New line. Replace 'New Line' with 'The next line read' since new line may be misregarded as \n. @@ +71,5 @@ > +} > + > +static bool > +ExtractParameter(const nsAutoCString& aCurrLine, > + const char* aPattern, nsCString& aParam) Why not pass |const nsACString& aParam| but |nsCString&|? @@ +138,5 @@ > + // Blank line or eof, exit > + return; > + } > + // Parse bMessage > + (this->*(ParserActions[mState]))(curLine); Shall we add assertion or protection for ParseActions[BMSG_PARSING_STATE_INVALID] case? @@ +201,5 @@ > +BluetoothMapBMessage::ParseOriginator(const nsAutoCString& aCurrLine) > +{ > + if (aCurrLine.EqualsLiteral("BEGIN:VCARD")) { > + mOriginators.AppendElement(new VCard()); > + // We may parse vCard multiple times nit: newline before this line. @@ +233,5 @@ > + if(!aCurrLine.EqualsLiteral("BEGIN:VCARD") && > + !aCurrLine.EqualsLiteral("BEGIN:BENV")) { > + mState = BMSG_PARSING_STATE_BEGIN_BODY; > + return; > + } else if (aCurrLine.EqualsLiteral("BEGIN:BENV")) { Remove the else here since the if returns inside. if (...) { mState = BMSG_PARSING_STATE_BEGIN_BODY; return; } if (aCurrLine.EqualsLiteral("BEGIN:BENV")) { // nested BENV // TODO: Check nested BENV envelope level for Email use case ++mEnvelopeLevel; } @@ +239,5 @@ > + // TODO: Check nested BENV envelope level for Email use case > + ++mEnvelopeLevel; > + } > + > + mRecipients.AppendElement(new VCard()); nit: newline after this line. @@ +278,5 @@ > +{ > + nsCString param; > + if (ExtractParameter(aCurrLine, "PARTID::", param)) { > + nsresult rv; > + mPartId = param.ToInteger(&rv); Handle NS_FAILED(rv) case. ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.h @@ +45,5 @@ > + nsCString mName; > + nsCString mFormattedName; > + nsCString mTelephone; > + nsCString mEmail; > + ~VCard(); nit: move d'tor declaration before member variable ones as following. private: ~VCard(); nsCString mName; nsCString mFormattedName; nsCString mTelephone; nsCString mEmail; @@ +59,5 @@ > +public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(BluetoothMapBMessage) > + BluetoothMapBMessage(); > + // Parse OBEX body and return bMessage object. > + static already_AddRefed<BluetoothMapBMessage> Parse(uint8_t* aObexBody, Can we do parsing in constructor instead of extra static parsing function? constructor: BluetoothMapBMessage(uint8_t* aObexBody, int aLength) { const nsCString body = nsCString(reinterpret_cast<const char*>(aObexBody), aLength); ProcessDecode(body.get()); } constructor usage: nsRefPtr<BluetoothMapBMessage> bmsg = new BluetoothMapBMessage(bodyPtr, mBodySegmentLength) ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +1008,1 @@ > AppendBtNamedValueByTagId(aHeader, data, Map::AppParametersTagId::Charset); nit: newline after this line @@ +1008,2 @@ > AppendBtNamedValueByTagId(aHeader, data, Map::AppParametersTagId::Charset); > + uint8_t* bodyPtr = nullptr; Move the declaration into if since it's only used inside. // Get Body if (...) { uint8_t* bodyPtr = nullptr; ... }
Attachment #8664814 -
Flags: review?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8664814 -
Attachment is obsolete: true
Assignee | ||
Comment 25•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8665958 -
Flags: review?(btian)
Assignee | ||
Updated•9 years ago
|
Attachment #8665958 -
Attachment is obsolete: true
Attachment #8665958 -
Flags: review?(btian)
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8665961 -
Flags: review?(btian)
Comment 27•9 years ago
|
||
Comment on attachment 8665961 [details] [diff] [review] Bug 1166647 - Implement MAP bMessage class (bMessage builder/parser) (V5) Review of attachment 8665961 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. ::: dom/bluetooth/bluedroid/BluetoothMapBMessage.cpp @@ +283,5 @@ > + nsresult rv; > + mPartId = param.ToInteger(&rv); > + > + if (NS_FAILED(rv)) { > + BT_LOGR("Failed to convert PARTID"); Print |rv| for failed reason. ::: dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp @@ +995,5 @@ > AppendNamedValue(data, "folderName", name); > > AppendBtNamedValueByTagId(aHeader, data, > Map::AppParametersTagId::Transparent); > AppendBtNamedValueByTagId(aHeader, data, Map::AppParametersTagId::Retry); nit: newline after this line. @@ +1008,2 @@ > AppendBtNamedValueByTagId(aHeader, data, Map::AppParametersTagId::Charset); > + nsRefPtr<BluetoothMapBMessage> bmsg; Handle no Body/EndOfBody condition at the beginning of function: BluetoothMapSmsManager::HandleSmsMmsPushMessage(const ObexHeaderSet& aHeader) { MOZ_ASSERT(NS_IsMainThread()); BluetoothService* bs = BluetoothService::Get(); NS_ENSURE_TRUE_VOID(bs); if (!aHeader.Has(ObexHeaderId::Body) && !aHeader.Has(ObexHeaderId::EndOfBody)) { // print error log return; } InfallibleTArray<BluetoothNamedValue> data; nsString name; ... AppendBtNamedValueByTagId(aHeader, data, Map::AppParametersTagId::Charset); // Get Body uint8_t* bodyPtr = nullptr; aHeader.GetBody(&bodyPtr, &mBodySegmentLength); mBodySegment = bodyPtr; nsRefPtr<BluetoothMapBMessage> bmsg = new BluetoothMapBMessage(bodyPtr, mBodySegmentLength); ...
Attachment #8665961 -
Flags: review?(btian) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8665961 -
Attachment is obsolete: true
Assignee | ||
Comment 28•9 years ago
|
||
Assignee | ||
Comment 30•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=2fd54f322e75
Comment 31•9 years ago
|
||
This has caused bustage like this: ../../../../workspace/gecko/dom/bluetooth/bluedroid/BluetoothMapBMessage.h:34:41: error: 'BluetoothMapBMessage' was not declared in this scope ../../dist/include/nsISupportsImpl.h:77:68: error: template argument 1 is invalid
Comment 32•9 years ago
|
||
Backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/9c9a0a50f9cd
Assignee | ||
Updated•9 years ago
|
Attachment #8667760 -
Attachment is obsolete: true
Assignee | ||
Comment 33•9 years ago
|
||
Oops.It seems my local flame-kk environment doesn't catch this compiling error.
Assignee | ||
Comment 34•9 years ago
|
||
https://hg.mozilla.org/try/pushloghtml?changeset=da1712f9f98a
Assignee | ||
Comment 35•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da1712f9f98a
Assignee | ||
Comment 36•9 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #35) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=da1712f9f98a Looks good now.
Assignee | ||
Comment 38•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=8d1212cc5ce4
Assignee | ||
Comment 39•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
blocking-b2g: --- → 2.2r?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(whuang)
Updated•9 years ago
|
blocking-b2g: 2.2r? → ---
feature-b2g: --- → 2.2r+
Flags: needinfo?(whuang)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Whiteboard: needs-uplift
Comment 40•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8d1212cc5ce4
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S9 (16Oct)
Comment 41•9 years ago
|
||
Hi Shuan, this failed to apply to 2.2 A file named 'bug1166647-v22.patch' already exists in your patch directory. Rename patch 'Bug 1166647 - Implement MAP bMessage class, r=btian (v2.2)' (8668436) (r)/overwrite (o)? o renamed 1166647 -> bug1166647-v22.patch applying bug1166647-v22.patch unable to find 'dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp' for patching 4 out of 4 hunks FAILED -- saving rejects to file dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp.rej unable to find 'dom/bluetooth/bluedroid/BluetoothMapSmsManager.h' for patching 2 out of 2 hunks FAILED -- saving rejects to file dom/bluetooth/bluedroid/BluetoothMapSmsManager.h.rej patching file dom/bluetooth/moz.build Hunk #1 FAILED at 54 1 out of 1 hunks FAILED -- saving rejects to file dom/bluetooth/moz.build.rej could you take a look, thanks!
Flags: needinfo?(shuang)
Assignee | ||
Updated•9 years ago
|
Attachment #8668436 -
Attachment description: Bug 1166647 - Implement MAP bMessage class, r=btian (v2.2) → Bug 1166647 - Implement MAP bMessage class, r=btian (v2.2r)
Flags: needinfo?(shuang)
Assignee | ||
Comment 42•9 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #41) > Hi Shuan, this failed to apply to 2.2 > > A file named 'bug1166647-v22.patch' already exists in your patch directory. > Rename patch 'Bug 1166647 - Implement MAP bMessage class, r=btian (v2.2)' > (8668436) (r)/overwrite (o)? o > renamed 1166647 -> bug1166647-v22.patch > applying bug1166647-v22.patch > unable to find 'dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp' for > patching > 4 out of 4 hunks FAILED -- saving rejects to file > dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp.rej > unable to find 'dom/bluetooth/bluedroid/BluetoothMapSmsManager.h' for > patching > 2 out of 2 hunks FAILED -- saving rejects to file > dom/bluetooth/bluedroid/BluetoothMapSmsManager.h.rej > patching file dom/bluetooth/moz.build > Hunk #1 FAILED at 54 > 1 out of 1 hunks FAILED -- saving rejects to file dom/bluetooth/moz.build.rej > > could you take a look, thanks! Sorry, that patch should go to v2.2r not v2.2.
Assignee | ||
Comment 43•9 years ago
|
||
(In reply to Shawn Huang [:shawnjohnjr] from comment #42) > (In reply to Carsten Book [:Tomcat] from comment #41) > > Hi Shuan, this failed to apply to 2.2 > > > > A file named 'bug1166647-v22.patch' already exists in your patch directory. > > Rename patch 'Bug 1166647 - Implement MAP bMessage class, r=btian (v2.2)' > > (8668436) (r)/overwrite (o)? o > > renamed 1166647 -> bug1166647-v22.patch > > applying bug1166647-v22.patch > > unable to find 'dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp' for > > patching > > 4 out of 4 hunks FAILED -- saving rejects to file > > dom/bluetooth/bluedroid/BluetoothMapSmsManager.cpp.rej > > unable to find 'dom/bluetooth/bluedroid/BluetoothMapSmsManager.h' for > > patching > > 2 out of 2 hunks FAILED -- saving rejects to file > > dom/bluetooth/bluedroid/BluetoothMapSmsManager.h.rej > > patching file dom/bluetooth/moz.build > > Hunk #1 FAILED at 54 > > 1 out of 1 hunks FAILED -- saving rejects to file dom/bluetooth/moz.build.rej > > > > could you take a look, thanks! > > Sorry, that patch should go to v2.2r not v2.2. s/go/apply
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cbook)
Comment 44•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/263cc49a2260
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
•