Add few member functions of ObexHeaderSet for PBAP

RESOLVED FIXED in Firefox 41, Firefox OS v2.2r

Status

Firefox OS
Bluetooth
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jaliu, Assigned: jaliu)

Tracking

unspecified
2.2 S13 (29may)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

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

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
In order to support PBAP in FxOS, ObexHeaderSet has to provide functions to get  'ConnectionID', 'AuthChallenge', 'MaxListCount', 'vCardSelector' and 'DatabaseIdentifer'.
(Assignee)

Updated

3 years ago
Blocks: 892179
(Assignee)

Updated

3 years ago
Assignee: nobody → jaliu
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8610342 [details] [diff] [review]
Add few member functions of ObexHeaderSet for PBAP. (v1)
Attachment #8610342 - Flags: review?(shuang)
(Assignee)

Updated

3 years ago
Attachment #8610342 - Flags: review?(shuang)
(Assignee)

Comment 2

3 years ago
Created attachment 8610356 [details] [diff] [review]
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.
Attachment #8610342 - Attachment is obsolete: true
Attachment #8610356 - Flags: review?(shuang)
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.
Attachment #8610356 - Flags: review?(shuang)
(Assignee)

Comment 4

3 years ago
(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.
(Assignee)

Updated

3 years ago
See Also: → bug 1168298
(Assignee)

Comment 5

3 years ago
Created attachment 8610400 [details] [diff] [review]
Add few member functions of ObexHeaderSet for PBAP. (v3)

- Remove |GetAuthCallenge|
- Change the function name from |GetConnectionID| to |GetConnectionId|.
Attachment #8610356 - Attachment is obsolete: true
Attachment #8610400 - Flags: review?(shuang)
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?
Attachment #8610400 - Flags: review?(shuang) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8610477 [details] [diff] [review]
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.
Attachment #8610400 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/79774d2512e1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S13 (29may)

Comment 12

3 years ago
Mark feature-b2g:2.2r+ since PBAP is required bluetooth feature.
feature-b2g: --- → 2.2r?

Updated

3 years ago
feature-b2g: 2.2r? → 2.2r+
(Assignee)

Comment 13

3 years ago
Created attachment 8650829 [details] [diff] [review]
(for 2.2r) Add few member functions of ObexHeaderSet for PBAP. (v4), r=shuang
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → fixed
You need to log in before you can comment on or make changes to this bug.