1168266 - Add few member functions of ObexHeaderSet for PBAP

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

In order to support PBAP in FxOS, ObexHeaderSet has to provide functions to get  'ConnectionID', 'AuthChallenge', 'MaxListCount', 'vCardSelector' and 'DatabaseIdentifer'.

Comment 1

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

Attachment: Add few member functions of ObexHeaderSet for PBAP. (v1)

Comment 2

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

Attachment: Add few member functions of ObexHeaderSet for PBAP. (v2)

- Revise previous patch by making 'GetAuthChallenge' return false when the header doesn't exist in OBEX package.

Comment 3

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

Comment on attachment 8610356 [details] [diff] [review]
Add few member functions of ObexHeaderSet for PBAP. (v2)

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

I have some question regarding Authenticate Challenge. I will review again after you provide further explanation.

::: dom/bluetooth/ObexBase.h
@@ +255,5 @@
> +    int length = mHeaders.Length();
> +
> +    for (int i = 0; i < length; ++i) {
> +      if (mHeaders[i]->mId == ObexHeaderId::AuthChallenge) {
> +        uint8_t* ptr = mHeaders[i]->mData.get();

I'm not sure what the data will be? Can you explain this part? What's the content of mData.get()?
I checked IrObex1.2 Spec, 2.2.13 Authenticate Challenge, an 'Authenticate Challenge' header may contain more one tag-length-value triplet.
So I just wonder if you can explain why you can directly cast to bool. It would be great you can add some comments for |GetAuthCallenge|.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.h
@@ +33,5 @@
> +  DatabaseIdentifier      = 0x0D,
> +  vCardSelectorOperator   = 0x0E,
> +  ResetNewMissedCalls     = 0x0F,
> +  PbapSupportedFeatures   = 0x10
> +};

This looks good.

Comment 4

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #3)
> Comment on attachment 8610356 [details] [diff] [review]
> I checked IrObex1.2 Spec, 2.2.13 Authenticate Challenge, an 'Authenticate
> Challenge' header may contain more one tag-length-value triplet.
> So I just wonder if you can explain why you can directly cast to bool. It
> would be great you can add some comments for |GetAuthCallenge|.

You're right. I should parse the value from 'AuthChallenge' header.
Thank you for pointing it out.

I will remove this function from this patch and rewrite it at Bug 1168298.

Comment 5

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

Attachment: Add few member functions of ObexHeaderSet for PBAP. (v3)

- Remove |GetAuthCallenge|
- Change the function name from |GetConnectionID| to |GetConnectionId|.

Comment 6

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

Comment on attachment 8610400 [details] [diff] [review]
Add few member functions of ObexHeaderSet for PBAP. (v3)

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

::: dom/bluetooth/ObexBase.h
@@ +259,5 @@
> +   * @param aBufferSize [in]  The size of the given buffer.
> +   *
> +   * @return a boolean value to indicate whether the given paramter exists.
> +   */
> +  bool GetAppParameter(uint8_t aTagId, uint8_t* aRetBuf, int aBufferSize) const

nit: it should be GetAppParameter's'.

@@ +273,5 @@
> +        // An application parameters header may contain more than one
> +        // [tag]-[length]-[value] triplet. The [tag] and [length] fields are
> +        // each one byte in length.
> +        uint8_t tagId;
> +        int offset = 0;

uint8_t offset?

Comment 7

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

Attachment: Add few member functions of ObexHeaderSet for PBAP. (v4), r=shuang

- Revise previous patch based on #comment 6
- Add a comment
- Fix a bug in |GetAppParameter|

Thank Shawn for reviewing the patch.

Comment 8

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> Comment on attachment 8610400 [details] [diff] [review]
> Add few member functions of ObexHeaderSet for PBAP. (v3)
> 
> Review of attachment 8610400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/ObexBase.h
> @@ +259,5 @@
> > +   * @param aBufferSize [in]  The size of the given buffer.
> > +   *
> > +   * @return a boolean value to indicate whether the given paramter exists.
> > +   */
> > +  bool GetAppParameter(uint8_t aTagId, uint8_t* aRetBuf, int aBufferSize) const
> 
> nit: it should be GetAppParameter's'.
I prefer to keep using |GetAppParameter| as the function name since it only get one parameter in one time.

> @@ +273,5 @@
> > +        // An application parameters header may contain more than one
> > +        // [tag]-[length]-[value] triplet. The [tag] and [length] fields are
> > +        // each one byte in length.
> > +        uint8_t tagId;
> > +        int offset = 0;
> 
> uint8_t offset?
Got it. Thanks.

Comment 9

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

Treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bd4a32847a4

Comment 10

[Pulsebot]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/79774d2512e1

Comment 12

[Ben Tian (inactive)]

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

Comment 13

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

Attachment: (for 2.2r) Add few member functions of ObexHeaderSet for PBAP. (v4), r=shuang

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/1a3e3e8ce0f7