Closed Bug 1199548 Opened 4 years ago Closed 4 years ago

Do not append vCard Body payload in PBAP replies when |MaxListCount| is zero

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
FxOS-S7 (18Sep)
blocking-b2g 2.2r+
Tracking Status
firefox43 --- fixed
b2g-v2.2r --- fixed

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

Attachments

(2 files, 3 obsolete files)

According to PBAP 1.2 spec 2.1.4.3
"When MaxListCount = 0, the PSE shall ignore all other application parameters in the request except for ResetNewMissedCalls, vCardSelector and vCardSelectorOperator. The response shall include the PhonebookSize application parameter (see Section 5.1.4.5). The response shall not contain any Body header."
Assignee: nobody → btian
Attachment #8653868 - Flags: review?(shuang)
Remove trailing space.
Attachment #8653868 - Attachment is obsolete: true
Attachment #8653868 - Flags: review?(shuang)
Attachment #8653869 - Flags: review?(shuang)
Add assertion for |mVCardDataStream|.
Attachment #8653869 - Attachment is obsolete: true
Attachment #8653869 - Flags: review?(shuang)
Attachment #8653870 - Flags: review?(shuang)
Blocks: 1200124
Depends on: 1199107
Comment on attachment 8653870 [details] [diff] [review]
[2.2r] Patch 1 (v3): Do not append vCard Body payload in PBAP replies when |MaxListCount| is zero

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

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +794,5 @@
>  
> +  // Open vCard input stream only if |mPhonebookSizeRequired| is false,
> +  // according to section 2.1.4.3 "MaxListCount", PBAP 1.2:
> +  // "When MaxListCount = 0, ... The response shall not contain any
> +  //  Body header."

nit: Change single-line comment style to multiple-line.

@@ +823,5 @@
>    }
>  
> +  // Open vCard input stream only if |mPhonebookSizeRequired| is false,
> +  // according to section 5.3.4.4 "MaxListCount", PBAP 1.2:
> +  // "When MaxListCount = 0, ... The response shall not contain a Body header."

Ditto.

@@ +871,5 @@
> +   * - Part 3: [headerId:1][length:2][EndOfBody:0]
> +   * Otherwise,
> +   * - Part 2: [headerId:1][length:2][Body:var]
> +   * - (optional) Part 3: [headerId:1][length:2][EndOfBody:0]
> +   */

Great job! This is much easier to read.

@@ +908,5 @@
>      index += AppendHeaderEndOfBody(&res[index]);
>  
> +    mPhonebookSizeRequired = false;
> +  } else {
> +    MOZ_ASSERT(mVCardDataStream);

If we directly access data member |mVcardDataStream|, why do we need to pass mVcardDataStream as the 1st parameter? It seems overall we pass mVcardDataStream to |ReplyToGet|.
Attachment #8653870 - Flags: review?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> @@ +908,5 @@
> >      index += AppendHeaderEndOfBody(&res[index]);
> >  
> > +    mPhonebookSizeRequired = false;
> > +  } else {
> > +    MOZ_ASSERT(mVCardDataStream);
> 
> If we directly access data member |mVcardDataStream|, why do we need to pass
> mVcardDataStream as the 1st parameter? It seems overall we pass
> mVcardDataStream to |ReplyToGet|.

This is done on 2.2R in bug 1199107.
Attachment #8653870 - Flags: review?(shuang)
(In reply to Ben Tian [:btian] from comment #5)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> > @@ +908,5 @@
> > >      index += AppendHeaderEndOfBody(&res[index]);
> > >  
> > > +    mPhonebookSizeRequired = false;
> > > +  } else {
> > > +    MOZ_ASSERT(mVCardDataStream);
> > 
> > If we directly access data member |mVcardDataStream|, why do we need to pass
> > mVcardDataStream as the 1st parameter? It seems overall we pass
> > mVcardDataStream to |ReplyToGet|.
> 
> This is done on 2.2R in bug 1199107.

I see. I forgot I reviewed that patch before.
Revise based on reviewer's comment.
Attachment #8653870 - Attachment is obsolete: true
Mark blocking-b2g:2.2r+ since this blocks 2.2r bluetooth feature PBAP. Also set checkin-needed to land comment 7 patch into 2.2r.
blocking-b2g: --- → 2.2r+
Keywords: checkin-needed
can we get a try run for this change ? thanks!
Flags: needinfo?(btian)
Keywords: checkin-needed
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1888676ac246

Try run with this 2.2R patch.
Flags: needinfo?(btian)
(In reply to Ben Tian [:btian] from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1888676ac246
> 
> Try run with this 2.2R patch.

Not sure whether this try result helps. Please let me know if you want more specific try run.
Flags: needinfo?(cbook)
(In reply to Ben Tian [:btian] from comment #11)
> (In reply to Ben Tian [:btian] from comment #10)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1888676ac246
> > 
> > Try run with this 2.2R patch.

The failure are irrelative to my bluetooth patch. Set checkin-needed for 2.2R branch again.
Keywords: checkin-needed
Ryan, can you help land comment 7 patch into 2.2R branch? Let me know if you require further testing.
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Flags: needinfo?(cbook)
Restore checkin-needed for 2.2R patch in comment 7.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b6c7e2b00a77
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)
Set checkin-needed for 2.2R branch per comment 17.
This bug appears in "Ready to uplift to mozilla-b2g37r (Gecko)" [1] on the wiki, instead of "Blocker Queries" [2].

[1] https://wiki.mozilla.org/Release_Management/B2G_Landing#v2.2r_2
[2] https://wiki.mozilla.org/Release_Management/B2G_Landing#Blocker_Queries_2

Any further condition is required for 2.2R landing?
Flags: needinfo?(ryanvm)
My point is that this bug shows up in the automatic uplift queries already, so checkin-needed isn't needed (and adds noise to other queries).
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> My point is that this bug shows up in the automatic uplift queries already,
> so checkin-needed isn't needed (and adds noise to other queries).

I see. I expected 2.2R uplift to be done with m-c together in comment 18 since the flag was already set. Thanks for correcting me. Just wait for now.
You need to log in before you can comment on or make changes to this bug.