Closed Bug 1166647 Opened 5 years ago Closed 4 years ago

Implement MAP bMessage class (bMessage builder/parser)

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

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

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

(Whiteboard: needs-uplift)

Attachments

(7 files, 14 obsolete files)

381 bytes, text/plain
Details
246 bytes, application/x-shellscript
Details
381 bytes, text/plain
Details
7.02 KB, text/x-python
Details
246 bytes, application/x-shellscript
Details
26.33 KB, patch
Details | Diff | Splinter Review
26.49 KB, patch
Details | Diff | Splinter Review
Implement MAP bMessage class (bMessage builder/parser)
Assignee: nobody → shuang
I got some sample here for bMessage parsing clarification. Testing data dumped from MCE.
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"
Attached patch bug1166647-mc.patch (WIP) (obsolete) — Splinter Review
bMsg/Vcard parsing done.
Attached patch bug1166647-mc.patch (WIP) (obsolete) — Splinter Review
Add PushMessage code to use bMsg/vcard parser.
Attachment #8657714 - Attachment is obsolete: true
Attachment #8657714 - Flags: review?(btian)
Attachment #8657782 - Attachment is obsolete: true
Attachment #8657782 - Flags: review?(btian)
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)
(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.
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 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.
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 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 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).
Attached file map-client
Add list-messages with parameter test.
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)
Attachment #8665958 - Attachment is obsolete: true
Attachment #8665958 - Flags: review?(btian)
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+
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
Oops.It seems my local flame-kk environment doesn't catch this compiling error.
blocking-b2g: --- → 2.2r?
Flags: needinfo?(whuang)
blocking-b2g: 2.2r? → ---
feature-b2g: --- → 2.2r+
Flags: needinfo?(whuang)
Keywords: checkin-needed
Whiteboard: needs-uplift
https://hg.mozilla.org/mozilla-central/rev/8d1212cc5ce4
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S9 (16Oct)
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)
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)
(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.
(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
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.