Closed Bug 1180554 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
FxOS-S6 (04Sep)
feature-b2g 2.2r+
Tracking Status
firefox43 --- fixed
b2g-v2.2r --- fixed

People

(Reporter: jaliu, Assigned: jaliu)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 10 obsolete files)

54.44 KB, patch
ben.tian
: review+
mrbkap
: review+
Details | Diff | Splinter Review
51.27 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → jaliu
Status: NEW → ASSIGNED
Blocks: 892179
Attachment #8630414 - Flags: review?(btian)
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.
Attachment #8630414 - Flags: review?(btian)
- revise previous patch based on #comment 3
Attachment #8630414 - Attachment is obsolete: true
Attachment #8635162 - Flags: review?(btian)
Attachment #8635162 - Flags: review?(btian)
- revise previous patch based on #comment 3
Attachment #8635162 - Attachment is obsolete: true
Attachment #8635170 - Flags: review?(btian)
(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 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 ...
Attachment #8635170 - Flags: review?(btian)
- Revise previous patch based on #comment 7.
Attachment #8635170 - Attachment is obsolete: true
Attachment #8637031 - Flags: review?(btian)
(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 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.
Attachment #8637031 - Flags: review?(btian) → review+
- revise previous patch based on #comment 10.

Thank Ben for reviewing the patch.
Attachment #8637031 - Attachment is obsolete: true
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 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
Attachment #8643593 - Flags: review?(mrbkap)
So you know, it's going to be a few days before I get to this review.
(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 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.
Attachment #8643593 - Flags: review?(mrbkap) → review+
- Rebase
- Fix an incorrect MOZ_ASSERT statement in function BluetoothAdapter::getVCardProperties(...)
Attachment #8643593 - Attachment is obsolete: true
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.
Attachment #8649646 - Attachment is obsolete: true
Attachment #8650264 - Flags: review?(mrbkap)
Blocks: 1196125
(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).
Attachment #8650264 - Flags: review?(mrbkap) → review+
Mark feature-b2g: 2.2r+ since PBAP is required bluetooth feature.
feature-b2g: --- → 2.2r+
Correct previous patch by adding this statement to 'dom/tests/mochitest/general/test_interfaces.html'.
{name: "BluetoothPbapRequestHandle", b2g: true, permission: ["bluetooth"]},
Attachment #8650264 - Attachment is obsolete: true
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.
Attachment #8650906 - Flags: review?(mrbkap)
Attachment #8650906 - Flags: review?(btian)
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?
fix a typo
Attachment #8650906 - Attachment is obsolete: true
Attachment #8650906 - Flags: review?(mrbkap)
Attachment #8650906 - Flags: review?(btian)
Attachment #8650921 - Flags: review?(btian)
Attachment #8650921 - Flags: review?(btian) → review+
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 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?
Attachment #8650921 - Flags: review?(mrbkap)
Attachment #8650921 - Flags: review?(mrbkap) → review+
(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?
Flags: needinfo?(6jamin)
Flags: needinfo?(6jamin) → needinfo?(btian)
(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.
Flags: needinfo?(btian)
checkin-needed to land 2.2r patch in comment 32.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/092034d8c3b1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S6 (04Sep)
You need to log in before you can comment on or make changes to this bug.