1180554 - [Bluetooth] Notify Gaia the PBAP request by sending events to PBAP event handlers.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Jamin Liu [:jaliu][:Jamin]]

Notify Gaia when the PBAP requests comes by sending events to PBAP event handlers, including the events of PullvCardListing, PullPhonebook and PullvCardListing.

This bug should also support vCardSelecting.

Comment 1

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v0)

Comment 2

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v1)

Comment 3

[Ben Tian (inactive)]

Comment on attachment 8630414 [details] [diff] [review]
Dispatch events to PBAP event handlers when the PBAP requests comes. (v1)

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

Please see my comment below.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +252,5 @@
>          ReplyToSetPath();
>        }
>        break;
>      case ObexRequestCode::Put:
>      case ObexRequestCode::PutFinal:

OBEX requests 'Put' and 'PutFinal' should remain unsupported, right?

@@ +266,5 @@
> +      uint8_t response;
> +      nsString type;
> +      pktHeaders.GetContentType(type);
> +
> +      if (type.EqualsASCII("x-bt/vcard-listing")) {

Why not use |EqualsLiteral|?

Also move |response| declaration right before |if| to be close to where it's used.

@@ +269,5 @@
> +
> +      if (type.EqualsASCII("x-bt/vcard-listing")) {
> +        response = PullvCardListing(pktHeaders);
> +
> +        if (response != ObexResponseCode::Success) {

Remove this redundant check since identical check runs immediately after this |if|, here and following.

@@ +292,5 @@
> +        response = ObexResponseCode::NotImplemented;
> +        BT_LOGR("Unsupported ObexRequestCode %x", opCode);
> +      }
> +
> +      if (response != ObexResponseCode::Success) {

What to reply if |response| equals to success?

@@ +295,5 @@
> +
> +      if (response != ObexResponseCode::Success) {
> +        ReplyError(response);
> +        return;
> +      }

nit: indent the whole section above as |SetPath| case.

@@ +397,5 @@
> +    return ObexResponseCode::PreconditionFailed;
> +  }
> +
> +  nsString name;
> +  aHeader.GetName(name);

Move name getting after array declaration to make whole flow more clear -- declare array and append values one by one, here and following.

@@ +400,5 @@
> +  nsString name;
> +  aHeader.GetName(name);
> +
> +  InfallibleTArray<BluetoothNamedValue> data;
> +  BT_APPEND_NAMED_VALUE(data, "name", nsString(name));

Use |nsString(name)| directly since |name| is already of type nsString rather than nsAString.

@@ +403,5 @@
> +  InfallibleTArray<BluetoothNamedValue> data;
> +  BT_APPEND_NAMED_VALUE(data, "name", nsString(name));
> +
> +  uint8_t buf[64];
> +  bool hasFormat = aHeader.GetAppParameter(AppParameterTag::Format, buf, 64);

Can we have an array to get all these |has*| flags at once, and index by enumeration?

@@ +407,5 @@
> +  bool hasFormat = aHeader.GetAppParameter(AppParameterTag::Format, buf, 64);
> +  bool usevCard3 = false;
> +  if (hasFormat) {
> +    usevCard3 = buf[0];
> +    BT_APPEND_NAMED_VALUE(data, "format", usevCard3);

Can we have an array to store all these name literal strings?

@@ +410,5 @@
> +    usevCard3 = buf[0];
> +    BT_APPEND_NAMED_VALUE(data, "format", usevCard3);
> +  }
> +
> +  bool hasPropSelector = aHeader.GetAppParameter(AppParameterTag::PropertySelector, buf, 64);

nit: this line exceeds 80 characters. Please check all patch again for too long lines.

@@ +449,5 @@
> +      BT_APPEND_NAMED_VALUE(data, "vCardSelector_OR", BluetoothValue(props));
> +    }
> +  }
> +
> +  #ifdef MOZ_B2G_BT_API_V2

Use v2 by default as the latest m-c, here and following.

@@ +484,5 @@
> +  bool hasOrder = aHeader.GetAppParameter(AppParameterTag::Order, buf, 64);
> +  if (hasOrder) {
> +    uint8_t order = buf[0];
> +
> +    switch (order) {

Can we simplify with a string array indexed by order?

  if (hasOrder) {
    static const nsString sOrder[] = {
      CONVERT(0x00, NS_LITERAL_STRING("indexed")),
      CONVERT(0x00, NS_LITERAL_STRING("alphanumeric")),
      CONVERT(0x00, NS_LITERAL_STRING("phonetical"))
    };
    
    uint8_t order = buf[0];
    if (order < MOZ_ARRAY_LENGTH(sOrder)) {
      BT_APPEND_NAMED_VALUE(data, "order", sOrder[order]);
    } else {
      // print error message
    }

@@ +499,5 @@
> +  }
> +
> +  bool hasSearchText = aHeader.GetAppParameter(AppParameterTag::SearchValue, buf, 64);
> +  if (hasSearchText) {
> +    nsCString text((char *) buf);

Why use nsCString for search text only?

@@ +505,5 @@
> +    BT_APPEND_NAMED_VALUE(data, "searchText", text);
> +  }
> +
> +  bool hasSearchKey = aHeader.GetAppParameter(AppParameterTag::SearchProperty, buf, 64);
> +  if (hasSearchKey) {

Ditto.

@@ +569,5 @@
> +}
> +
> +  // required DOMString                 name;
> +  //          vCardVersion              format = "vCard21";
> +  // required sequence<vCardProperties> propSelector;

Replace with descriptive comment.

@@ +738,5 @@
> +  // convert big endian to little endian
> +  uint32_t x = (aData[7] << 0) | (aData[6] << 8) | (aData[5] << 16) | (aData[4] << 24);
> +
> +  uint32_t count = 0;
> +  while (x != 0) {

Replace with |!x|.

@@ +740,5 @@
> +
> +  uint32_t count = 0;
> +  while (x != 0) {
> +    uint32_t bit = x & 1;
> +    if(bit == 1) {

Simplify as following:

  if (x & 1) {
    propSelector.AppendElement(count);
  }

::: dom/bluetooth/bluedroid/BluetoothPbapManager.h
@@ +33,5 @@
>    PropertySelector        = 0x06,
>    Format                  = 0x07,
>    PhonebookSize           = 0x08,
>    NewMissedCalls          = 0x09,
> +  // ----- enumerators below are supported since PBAP 1.2 ----- //

I think it's 'enumeration' instead of 'enumerators'.

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +1225,5 @@
> +        value.get_ArrayOfuint32_t();
> +      for (uint32_t j = 0; j < propSelectorArr.Length(); ++j) {
> +        vCardSelector.AppendElement(
> +          static_cast<vCardProperties>(propSelectorArr[j]));
> +      }

Wrap this packing as a function to reuse.

::: dom/webidl/BluetoothAdapter.webidl
@@ +68,5 @@
>  
>    // Fired when remote devices query current media play status
>             attribute EventHandler   onrequestmediaplaystatus;
>  
> +  // Fired when PBAP manager request for 'pullphonebook'

nit: request's', here and following.

::: dom/webidl/BluetoothPbapParameters.webidl
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +/**
> + * This enum holds the parameters which is used to indicate the properties

nit: I prefer to remove 'which is used' to be concise, here and following.

  ... holds the parameters to indicate the properties...

@@ +44,5 @@
> +  "x-bt-uid"
> +};
> +
> +/**
> + * This enum holds the parameters which is used to indicate to the sorting order

nit: Remove redundant 'to' after indicate, here and following.

::: dom/webidl/BluetoothPbapRequestHandle.webidl
@@ +5,5 @@
> +
> +[CheckPermissions="bluetooth"]
> +interface BluetoothPbapRequestHandle
> +{
> +

nit: remove extra newline.

@@ +7,5 @@
> +interface BluetoothPbapRequestHandle
> +{
> +
> +  /**
> +   * Reply vCard object to the PBAP request. The promise will be rejected if the

nit: Replace promise with DOMRequest.

  The DOMRequest will return error if ...

@@ +8,5 @@
> +{
> +
> +  /**
> +   * Reply vCard object to the PBAP request. The promise will be rejected if the
> +   * pairing request type is not 'onpullvcardentryreq' or operation fails.

Replace 'pairing request' with 'PBAP request', here and the following. Also should request type be 'pullvcardentryreq' instead of 'onpullvcardentryreq'?

::: dom/webidl/BluetoothPhonebookPullingEvent.webidl
@@ +31,5 @@
> +           unsigned long             listStartOffset = 0;
> +  required sequence<vCardProperties> vcardSelector;
> +  required vCardSelectorOp vcardSelectorOperator;
> +
> +  required BluetoothPbapRequestHandle handle;

Confirm whether problem as bug 1174071 happens here and following events. If so please follow bug 1167064 comment 26 as well.

Comment 4

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v2)

- revise previous patch based on #comment 3

Comment 5

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v2.1)

- revise previous patch based on #comment 3

Comment 6

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #3)
> Comment on attachment 8630414 [details] [diff] [review]
> Dispatch events to PBAP event handlers when the PBAP requests comes. (v1)
> -----------------------------------------------------------------

Thank you for reviewing the patch.

> ::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
> @@ +252,5 @@
> >          ReplyToSetPath();
> >        }
> >        break;
> >      case ObexRequestCode::Put:
> >      case ObexRequestCode::PutFinal:
> 
> OBEX requests 'Put' and 'PutFinal' should remain unsupported, right?
Yes. Thank you for pointing it out.

> @@ +292,5 @@
> > +        response = ObexResponseCode::NotImplemented;
> > +        BT_LOGR("Unsupported ObexRequestCode %x", opCode);
> > +      }
> > +
> > +      if (response != ObexResponseCode::Success) {
> 
> What to reply if |response| equals to success?
The OBEX response will be sent after Gaia replies the PBAP request by BluetoothPbapRequestHandle.

I've leaved a comment here in attachment #8635170 [details] [diff] [review]. 

> @@ +403,5 @@
> > +  InfallibleTArray<BluetoothNamedValue> data;
> > +  BT_APPEND_NAMED_VALUE(data, "name", nsString(name));
> > +
> > +  uint8_t buf[64];
> > +  bool hasFormat = aHeader.GetAppParameter(AppParameterTag::Format, buf, 64);
> 
> Can we have an array to get all these |has*| flags at once, and index by
> enumeration?

I've revised this issue in Attachment #8635170 [details] [diff].
Please take a look and give me some feedback.

The length of appParameters array would be 17. 
Since we only need 2 ~ 7 appParameters to distribute BT signals, I prefer to handle them separately.

I think the main concern here is the readability, therefore, I added a function |AppendBtNamedValueByTagId| to simplified these statements.

> @@ +407,5 @@
> > +  bool hasFormat = aHeader.GetAppParameter(AppParameterTag::Format, buf, 64);
> > +  bool usevCard3 = false;
> > +  if (hasFormat) {
> > +    usevCard3 = buf[0];
> > +    BT_APPEND_NAMED_VALUE(data, "format", usevCard3);
> 
> Can we have an array to store all these name literal strings?

I had tried to revise the codes by using a literal strings array, but I can't find a proper way to do this.

One of the reasons is that the array will be used as the second parameter of 'BT_APPEND_NAMED_VALUE'. It means the array will be manipulated by the macro 'NS_LITERAL_STRING' first before the compilation process.

Another reason is AppParameterTags and BtSignal name are not 1 - 1 mapping. (For instance, 'vCardSelector' and 'vCardSelectorOperator' are two AppParameterTags, but them are be packed into one BT signal.)

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8635170 [details] [diff] [review]
Dispatch events to PBAP event handlers when the PBAP requests comes. (v2.1)

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

Please see my comment below.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ -252,5 @@
>          ReplyToSetPath();
>        }
>        break;
> -    case ObexRequestCode::Put:
> -    case ObexRequestCode::PutFinal:

Keep the two cases before default case since they are unsupported instead of unrecognized.

  case ObexRequestCode::Put:
  case ObexRequestCode::PutFinal:
    ReplyError(ObexResponseCode::BadRequest);
    BT_LOGR("Unsupported ObexRequestCode %x", opCode);
    break;

@@ +255,4 @@
>      case ObexRequestCode::Get:
> +    case ObexRequestCode::GetFinal: {
> +        // [opcode:1][length:2][Headers:var]
> +        if (!ParseHeaders(&data[3], receivedLength - 3, &pktHeaders)) {

Ensure receivedLength >= 3 as [1].

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

@@ +265,5 @@
> +
> +        uint8_t response;
> +        if (type.EqualsLiteral("x-bt/vcard-listing")) {
> +          response = PullvCardListing(pktHeaders);
> +        } else if(type.EqualsLiteral("x-bt/vcard")) {

nit: space after |if|.

@@ +267,5 @@
> +        if (type.EqualsLiteral("x-bt/vcard-listing")) {
> +          response = PullvCardListing(pktHeaders);
> +        } else if(type.EqualsLiteral("x-bt/vcard")) {
> +          response = PullvCardEntry(pktHeaders);
> +        } else if(type.EqualsLiteral("x-bt/phonebook")) {

Ditto.

@@ +270,5 @@
> +          response = PullvCardEntry(pktHeaders);
> +        } else if(type.EqualsLiteral("x-bt/phonebook")) {
> +          response = PullPhonebook(pktHeaders);
> +        } else {
> +          response = ObexResponseCode::NotImplemented;

Why is |response| NotImplemented instead of others?

@@ +271,5 @@
> +        } else if(type.EqualsLiteral("x-bt/phonebook")) {
> +          response = PullPhonebook(pktHeaders);
> +        } else {
> +          response = ObexResponseCode::NotImplemented;
> +          BT_LOGR("Unsupported ObexRequestCode %x", opCode);

Print unknown type instead of unsupported opcode.

@@ +274,5 @@
> +          response = ObexResponseCode::NotImplemented;
> +          BT_LOGR("Unsupported ObexRequestCode %x", opCode);
> +        }
> +
> +        // The OBEX response will be sent after Gaia replies the PBAP request.

nit: OBEX 'success' response

@@ +487,5 @@
> +
> +  switch (aTagId) {
> +    case AppParameterTag::Order:
> +      tagExists = aHeader.GetAppParameter(AppParameterTag::Order, buf, 64);
> +      if (tagExists) {

Use guard clause instead, here and the following.

 if (!tagExists) {
   break;
 }

 ...

@@ +495,5 @@
> +        uint8_t order = buf[0];
> +        if (order < MOZ_ARRAY_LENGTH(sOrderStr)) {
> +          BT_APPEND_NAMED_VALUE(aValues, "order", sOrderStr[order]);
> +        } else {
> +          BT_WARNING("%s: Unexpected value of 'Order'", __FUNCTION__);

Print the unexpected value for debugging.

@@ +524,5 @@
> +        uint8_t searchKey = buf[0];
> +        if (searchKey < MOZ_ARRAY_LENGTH(sSearchKeyStr)) {
> +          BT_APPEND_NAMED_VALUE(aValues, "searchKey", sSearchKeyStr[searchKey]);
> +        } else {
> +          BT_WARNING("%s: Unexpected value of 'SearchProperty'", __FUNCTION__);

Ditto.

@@ +572,5 @@
> +        BT_APPEND_NAMED_VALUE(aValues, "format", usevCard3);
> +      }
> +      break;
> +    case AppParameterTag::vCardSelector: {
> +        tagExists =

nit: remove extra indention for this section.

@@ +579,5 @@
> +          InfallibleTArray<uint32_t> props = PackPropertiesMask(buf, 64);
> +
> +          bool hasVCardSelectorOperator = aHeader.GetAppParameter(
> +            AppParameterTag::vCardSelectorOperator, buf, 64);
> +          if (hasVCardSelectorOperator && buf[0]) {

Suggest to revise as following to emphasize literal string difference.

  char operatorName[18] =
    (hasVCardSelectorOperator && buf[0]) ? "vCardSelector_AND"
                                         : "vCardSelector_OR";
  BT_APPEND_NAMED_VALUE(aValues, operatorName, BluetoothValue(props));

@@ +590,5 @@
> +        }
> +      }
> +      break;
> +    default:
> +      BT_LOGR("Unsupported AppParameterTag: %x", aTagId);

Add break for default case.

@@ +713,5 @@
> +{
> +  InfallibleTArray<uint32_t> propSelector;
> +
> +  // Table 5.1 "Property Mask", PBAP 1.2
> +  // PropertyMask is a 64 bits mask that indicates the properties contained in

nit: a 64-bit mask

@@ +714,5 @@
> +  InfallibleTArray<uint32_t> propSelector;
> +
> +  // Table 5.1 "Property Mask", PBAP 1.2
> +  // PropertyMask is a 64 bits mask that indicates the properties contained in
> +  // the requested vCard objects. We only supports bit 0 ~31 since the rest are

nit: We only support' '.

Also is the space after 0 redundant?

@@ +719,5 @@
> +  // reserved for future use or vendor specific properties.
> +
> +  // convert big endian to little endian
> +  uint32_t x = (aData[7] << 0)  | (aData[6] << 8)
> +             | (aData[5] << 16) | (aData[4] << 24);

nit: put the | to prior line.

  uint32_t x = (aData[7] << 0)  | (aData[6] << 8) |
               (aData[5] << 16) | (aData[4] << 24);

::: dom/bluetooth/bluetooth1/BluetoothAdapter.cpp
@@ +604,5 @@
> +  for (uint32_t i = 0, propCount = arr.Length(); i < propCount; ++i) {
> +    const nsString& name = arr[i].name();
> +    const BluetoothValue& value = arr[i].value();
> +    if (name.EqualsLiteral("name")) {
> +       init.mName = value.get_nsString();

nit: remove extra indention.

@@ +608,5 @@
> +       init.mName = value.get_nsString();
> +    } else if (name.EqualsLiteral("format")) {
> +      bool isVCard3 = false;
> +      isVCard3 = value.get_bool();
> +      init.mFormat = isVCard3 ? vCardVersion::VCard30 : vCardVersion::VCard21;

Simplify as following.

  init.mFormat = value.get_bool() ? vCardVersion::VCard30 : vCardVersion::VCard21;

@@ +647,5 @@
> +  for (uint32_t i = 0, propCount = arr.Length(); i < propCount; ++i) {
> +    const nsString& name = arr[i].name();
> +    const BluetoothValue& value = arr[i].value();
> +    if (name.EqualsLiteral("name")) {
> +       init.mName  = value.get_nsString();

nit: remove extra space and indention.

@@ +651,5 @@
> +       init.mName  = value.get_nsString();
> +    } else if (name.EqualsLiteral("format")) {
> +      bool isVCard3 = false;
> +      isVCard3 = value.get_bool();
> +      init.mFormat = isVCard3 ? vCardVersion::VCard30 : vCardVersion::VCard21;

Simplify as following.

  init.mFormat = value.get_bool() ? vCardVersion::VCard30 : vCardVersion::VCard21;

@@ +671,5 @@
> +  const InfallibleTArray<BluetoothNamedValue>& arr =
> +    aValue.get_ArrayOfBluetoothNamedValue();
> +
> +  MOZ_ASSERT(arr.Length() >= 1 &&
> +       arr[0].value().type() == BluetoothValue::TnsString);

nit: align as following

  MOZ_ASSERT(arr.Length() >= 1 &&
             arr[0].value().type() == BluetoothValue::TnsString);

@@ +680,5 @@
> +  for (uint32_t i = 0, propCount = arr.Length(); i < propCount; ++i) {
> +    const nsString& name = arr[i].name();
> +    const BluetoothValue& value = arr[i].value();
> +    if (name.EqualsLiteral("name")) {
> +       init.mName = value.get_nsString();

nit: remove extra indention.

@@ +683,5 @@
> +    if (name.EqualsLiteral("name")) {
> +       init.mName = value.get_nsString();
> +    } else if (name.EqualsLiteral("order")) {
> +      nsString order = value.get_nsString();
> +      if (order.EqualsLiteral("alphabetical")) {

Write a function as [1] for the conversion.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/BluetoothAdapter.cpp#923

@@ +694,5 @@
> +    } else if (name.EqualsLiteral("searchText")) {
> +      init.mSearchValue = value.get_nsString();
> +    } else if (name.EqualsLiteral("searchKey")) {
> +      nsString searchKey = value.get_nsString();
> +      if (searchKey.EqualsLiteral("name")) {

Ditto.

@@ +709,5 @@
> +    } else if (name.EqualsLiteral("vCardSelector_AND")) {
> +      const InfallibleTArray<uint32_t>& propSelectorArr =
> +        value.get_ArrayOfuint32_t();
> +      for (uint32_t j = 0; j < propSelectorArr.Length(); ++j) {
> +        vCardSelector.AppendElement(

Can we use |AppendElements| directly?

::: dom/webidl/BluetoothPbapParameters.webidl
@@ +64,5 @@
> +  "sound"
> +};
> +
> +/**
> + * This enum holds the parameters to indicate the vCard Version

nit: period at the end. Have all comment either with period or not to be consistent.

@@ +73,5 @@
> +};
> +
> +/**
> + * This enum holds the parameters to indicate the type of vCard
> + * selector.

nit: make these two lines in one.

::: dom/webidl/BluetoothPbapRequestHandle.webidl
@@ +6,5 @@
> +[CheckPermissions="bluetooth"]
> +interface BluetoothPbapRequestHandle
> +{
> +  /**
> +   * Reply vCard object to the PBAP request. The DOMRequest will be rejected if

nit: only promises use 'reject', I think 'get onerror callbacks' is more suitable for DOMRequests.

  The DOMRequest will get onerror callback if ...

Comment 8

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v3)

- Revise previous patch based on #comment 7.

Comment 9

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Ben Tian [:btian] from comment #7)
> Comment on attachment 8635170 [details] [diff] [review]
> Dispatch events to PBAP event handlers when the PBAP requests comes. (v2.1)
Thank you for reviewing the patch.

> @@ +270,5 @@
> > +          response = PullvCardEntry(pktHeaders);
> > +        } else if(type.EqualsLiteral("x-bt/phonebook")) {
> > +          response = PullPhonebook(pktHeaders);
> > +        } else {
> > +          response = ObexResponseCode::NotImplemented;
> 
> Why is |response| NotImplemented instead of others?

I've changed the response code to 'BadRequest' since it is more suitable for this case.

> @@ +579,5 @@
> > +          InfallibleTArray<uint32_t> props = PackPropertiesMask(buf, 64);
> > +
> > +          bool hasVCardSelectorOperator = aHeader.GetAppParameter(
> > +            AppParameterTag::vCardSelectorOperator, buf, 64);
> > +          if (hasVCardSelectorOperator && buf[0]) {
> 
> Suggest to revise as following to emphasize literal string difference.

I had tried to revise the codes, however, I can't find a proper way to do it right.
This statement would cause a compile error since the char array must be initialized with a brace enclosed initializer.
Besides, BT_APPEND_NAMED_VALUE can't take 'operatorName' as the second parameter since it will be manipulated by the macro 'NS_LITERAL_STRING' first before the compilation process.

> @@ +709,5 @@
> > +    } else if (name.EqualsLiteral("vCardSelector_AND")) {
> > +      const InfallibleTArray<uint32_t>& propSelectorArr =
> > +        value.get_ArrayOfuint32_t();
> > +      for (uint32_t j = 0; j < propSelectorArr.Length(); ++j) {
> > +        vCardSelector.AppendElement(
> 
> Can we use |AppendElements| directly?
I believe there is no easy way to do this.

vCardSelector.AppendElements() can only take InfallibleTArray<vCardSelector> as paramter.
However, value.get_ArrayOfuint32_t() can't be assigned to InfallibleTArray<vCardSelector>.
If we want to get InfallibleTArray<vCardSelector> from BtValue directly, we have to define a new type in BluetoothTypes.ipdlh. 
The other approach is to override the type converter of the array template.
I prefer to use AppendElement() compare to these two approaches.

Comment 10

[Ben Tian (inactive)]

Comment on attachment 8637031 [details] [diff] [review]
Dispatch events to PBAP event handlers when the PBAP requests comes. (v3)

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +255,2 @@
>      case ObexRequestCode::Put:
>      case ObexRequestCode::PutFinal:

nit: move these two unused cases 'right before' default case.

@@ +494,5 @@
> +
> +  switch (aTagId) {
> +    case AppParameterTag::Order: {
> +      tagExists = aHeader.GetAppParameter(AppParameterTag::Order, buf, 64);
> +      if (!tagExists) {

Simplify as following, here and afterward.

  if (!aHeader.GetAppParameter(AppParameterTag::Order, buf, 64)) {
    break;
  }

::: dom/bluetooth/bluetooth1/BluetoothAdapter.cpp
@@ +646,5 @@
> +  for (uint32_t i = 0, propCount = arr.Length(); i < propCount; ++i) {
> +    const nsString& name = arr[i].name();
> +    const BluetoothValue& value = arr[i].value();
> +    if (name.EqualsLiteral("name")) {
> +      init.mName  = value.get_nsString();

nit: remove extra space before =

@@ +696,5 @@
> +      for (uint32_t j = 0; j < propSelectorArr.Length(); ++j) {
> +        vCardSelector.AppendElement(
> +          static_cast<vCardProperties>(propSelectorArr[j]));
> +      }
> +      init.mVcardSelector = vCardSelector;

Use |getVCardProperties|.

 init.mVcardSelector = getVCardProperties(value);

@@ +705,5 @@
> +      for (uint32_t j = 0; j < propSelectorArr.Length(); ++j) {
> +        vCardSelector.AppendElement(
> +          static_cast<vCardProperties>(propSelectorArr[j]));
> +      }
> +      init.mVcardSelector = vCardSelector;

Ditto.

@@ +717,5 @@
> +  DispatchTrustedEvent(event);
> +}
> +
> +Sequence<vCardProperties>
> +BluetoothAdapter::getVCardProperties(const BluetoothValue &aValue) {

nit: move { to the next line.

@@ +733,5 @@
> +  return propSelector;
> +}
> +
> +vCardOrderType
> +BluetoothAdapter::ConvertStringToVCardOrderType(const nsAString& aString)

I meant to query from mozilla::dom::BluetoothVCardOrderTypeValue as [1] instead of hardcoded if conditions.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/BluetoothAdapter.cpp#923

@@ +749,5 @@
> +  return vCardOrderType::Indexed; // The default value is 'Indexed'.
> +}
> +
> +vCardSearchKeyType
> +BluetoothAdapter::ConvertStringToVCardSearchKeyType(const nsAString& aString)

Ditto.

::: dom/webidl/BluetoothPbapRequestHandle.webidl
@@ +6,5 @@
> +[CheckPermissions="bluetooth"]
> +interface BluetoothPbapRequestHandle
> +{
> +  /**
> +   * Reply vCard object to the PBAP request. The DOMRequest will get onerror

Should be onerror callback' ' since only one is fired to the DOMRequest. Sorry for the prior incorrect comment.

@@ +14,5 @@
> +  [NewObject, Throws, AvailableIn=CertifiedApps]
> +  DOMRequest replyTovCardPulling(Blob vcardObject);
> +
> +  /**
> +   * Reply vCard object to the PBAP request. The DOMRequest will get onerror

Ditto.

@@ +22,5 @@
> +  [NewObject, Throws, AvailableIn=CertifiedApps]
> +  DOMRequest replyToPhonebookPulling(Blob vcardObject,
> +                                     unsigned long long phonebookSize);
> +  /**
> +   * Reply vCard object to the PBAP request. The DOMRequest will get onerror

Ditto.

Comment 11

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v4), r=btian

- revise previous patch based on #comment 10.

Thank Ben for reviewing the patch.

Comment 12

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

Comment on attachment 8637109 [details] [diff] [review]
Dispatch events to PBAP event handlers when the PBAP requests comes. (v4), r=btian

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

::: dom/bluetooth/BluetoothPbapRequestHandle.h
@@ +18,5 @@
> +    class DOMRequest;
> +  }
> +}
> +
> +BEGIN_BLUETOOTH_NAMESPACE

Not sure why when I wrote similar patch for MAP, I hit build break and error is BEGIN_BLUETOOTH_NAMESPACE is not a type. I finally found that 'BluetoothCommon.h' is not included.

Comment 13

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.1), r=btian

rebase

Comment 14

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8643593 [details] [diff] [review]
Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.1), r=btian

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

Hi Blake,
Would you mind helping to review this patch regarding to WebIDL part?

The PBAP (Phone Book Access Profile) specification defines the procedures and protocols to exchange Phone Book objects between devices. 
A typical scenario is to let a Car-Kit retrieve Phone Book objects from a mobile device. Please refer to [1].

Due to the request from our partner, Bluetooth team would like to support PBAP on FxOS as a PSE (Phonebook Server Equipment).
Since the contacts and call logs are controlled by Gaia apps, I think it's reasonable to expose few related APIs to certified app.

Per discussion with Gaia developer Sean Lee, I implemented three PBAP events and their event handlers in this Bug.
The events are used to notify Gaia the request from remote Bluetooth device.


There are 5 new webidl files in this patch.

- BluetoothPhonebookPullingEvent.webidl
  The event is used to notify Gaia about the 'PullPhoneBook' request from PCE(Phonebook Client Equipment).
  The 'PullPhoneBook' function is a PBAP function which is designed to retrieve an entire phone book object from the object exchange server.

- BluetoothVCardListingEvent.webidl
  The event is used to notify Gaia about the 'PullvCardListing' request from PCE.
  The 'PullPhoneBook' function retrieves the PSE’s Phonebook-listing object.

- BluetoothVCardPullingEvent.webidl
  The event is used to notify Gaia about the 'PullvCardEntry' request from PCE.
  The 'PullvCardEntry' function retrieves a specific vCard from the object exchange server.

- BluetoothPbapRequestHandle.webidl
  The webidl defines a handle which is used to reply to the PBAP requests. 

- BluetoothPbapParameters.webidl
  The webidl defines few enumerations that PBAP needs.

I also added three event handlers to |BluetoothAdapter.webidl| for handling PBAP requests.

If you you need more details about PBAP, please feel free to let me know.
Thank you very much.

[1] Introduction of PBAP
https://developer.bluetooth.org/TechnologyOverview/Pages/PBAP.aspx

[2] PBAP 1.2 spec.
https://www.bluetooth.org/DocMan/handlers/DownloadDoc.ashx?doc_id=281299

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

So you know, it's going to be a few days before I get to this review.

Comment 16

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #15)
> So you know, it's going to be a few days before I get to this review.

Sure.
Thank you for your time. :)

Comment 17

[Ben Tian (inactive)]

Comment on attachment 8643593 [details] [diff] [review]
Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.1), r=btian

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

::: dom/bluetooth/BluetoothPbapRequestHandle.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_bluetooth_bluetoothpbaprequesthandle_h
> +#define mozilla_dom_bluetooth_bluetoothpbaprequesthandle_h

Please revise to |mozille_dom_bluetooth_BluetoothPbapRequestHandle_h| in the final patch as bug 1106007 has been landed on m-c.

@@ +58,5 @@
> +};
> +
> +END_BLUETOOTH_NAMESPACE
> +
> +#endif // mozilla_dom_bluetooth_bluetoothpbaprequesthandle_h
\ No newline at end of file

Ditto.

Comment 18

[Jamin Liu [:jaliu][:Jamin]]

Attachment: bug1180554_pbap_events_v4.3.patch

- Rebase
- Fix an incorrect MOZ_ASSERT statement in function BluetoothAdapter::getVCardProperties(...)

Comment 19

[Jamin Liu [:jaliu][:Jamin]]

Thank Blake for reviewing the patch.

Treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c4c52e8ba7e (ICS build was busted.)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=144cddc7877e (ICS build was fixed by #Attachment 8649650 [details] [diff] in #Bug 1180555)

Comment 20

[Pulsebot]

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

Comment 21

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.4), r=btian, r=mrbkap

Hi Blake,
I forgot to modify 'dom/tests/mochitest/general/test_interfaces.html' for the PBAP events in the previous patch.

I've added these statements to test_interfaces.html in this patch.

{name: "BluetoothPhonebookPullingEvent", b2g: true, permission: ["bluetooth"]},
{name: "BluetoothVCardListingEvent", b2g: true, permission: ["bluetooth"]},
{name: "BluetoothVCardPullingEvent", b2g: true, permission: ["bluetooth"]},

I will push this patch to try server with mochitests (all) syntax.
Would you mind to review the changes in test_interfaces.html ?
Sorry for the inconvenience caused.

Comment 22

[Jamin Liu [:jaliu][:Jamin]]

(In reply to Jamin Liu [:jaliu] from comment #21)
> I will push this patch to try server with mochitests (all) syntax.
> Would you mind to review the changes in test_interfaces.html ?
> Sorry for the inconvenience caused.

The results at treeherder looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18cc41a43a0f

P.S. There are few irrelevant mochitest failures due to the known issues (Bug 965705, Bug 1169726 and Bug 1152924).

Comment 23

[Ben Tian (inactive)]

Mark feature-b2g: 2.2r+ since PBAP is required bluetooth feature.

Comment 24

[Pulsebot]

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

Comment 25

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.5), r=btian, r=mrbkap

Correct previous patch by adding this statement to 'dom/tests/mochitest/general/test_interfaces.html'.
{name: "BluetoothPbapRequestHandle", b2g: true, permission: ["bluetooth"]},

Comment 26

[Jamin Liu [:jaliu][:Jamin]]

Comment on attachment 8650906 [details] [diff] [review]
Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.5), r=btian, r=mrbkap

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

r? again since the patch has been updated.
I'm really sorry for the inconvenience caused.

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8650906 [details] [diff] [review]
Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.5), r=btian, r=mrbkap

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

Furthermore, a diff with the changes since the push would be helpful.

::: dom/tests/mochitest/general/test_interfaces.html
@@ +216,5 @@
>      {name: "BluetoothLeDeviceEvent", b2g: true, permission: ["bluetooth"]},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      {name: "BluetoothManager", b2g: true, permission: ["bluetooth"]},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "BluetoothPhbapRequestHandle", b2g: true, permission: ["bluetooth"]},

Is that a typo? Shouldn't it be BluetoothPbapRequestHandle?

Comment 28

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.5-fix), r=btian, r=mrbkap

fix a typo

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out 3 changesets (131251625ee8 for bug 1180556, 5bdcc058e6d6 for bug 1180555, f7e0cd74c082 for bug 1180554) for B"G ICS Emulator opt M8 and debug M19 failures. r=backout
https://hg.mozilla.org/integration/b2g-inbound/rev/00b847293e70

Comment 30

[Ben Tian (inactive)]

Comment on attachment 8650921 [details] [diff] [review]
Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.5-fix), r=btian, r=mrbkap

Blake, can you review the revised patch with comment 25 change?

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/092034d8c3b1

Comment 32

[Ben Tian (inactive)]

Attachment: [2.2r] Dispatch events to PBAP event handlers when the PBAP requests comes. (v4.5-fix), r=btian, r=mrbkap

Comment 33

[Ryan VanderMeulen [:RyanVM]]

(In reply to Pulsebot from comment #31)
> https://hg.mozilla.org/integration/b2g-inbound/rev/092034d8c3b1

Who did you talk to about landing these CLOSED TREE?

Comment 34

[Ben Tian (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #33)
> (In reply to Pulsebot from comment #31)
> > https://hg.mozilla.org/integration/b2g-inbound/rev/092034d8c3b1
> 
> Who did you talk to about landing these CLOSED TREE?

I misunderstood the CLOSED TREE meaning and will wait until the tree reopens next time. Sorry for any inconvenience.

Comment 35

[Ben Tian (inactive)]

checkin-needed to land 2.2r patch in comment 32.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/092034d8c3b1

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/f442404dc0b5