1203821 - [cleanup] Simplify BluetoothPbapManager functions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Ben Tian (inactive)]

Simplify BluetoothPbapManager functions as following:
- add utility function to convert big/little endianness
- revise BluetoothPbapManager with above utility functions
- integrate |PullPhonebook|, |PullvCardListing|, and |PullvCardEntry| into one function to fire PBAP request for gaia.

Comment 1

[Ben Tian (inactive)]

Attachment: PAtch 1 (v1): Add utility functions to convert big/little endianness and revise BluetoothPbapManager accordingly

Comment 2

[Ben Tian (inactive)]

Attachment: Patch 2 (v1): Fire PBAP request to gaia with one integrated function

Comment 3

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Add utility functions to convert big/little endianness and revise BluetoothPbapManager accordingly, r=shuang

Comment 4

[Ben Tian (inactive)]

Attachment: Patch 2 (v2): Notify gaia of PBAP request with one integrated function

Comment 5

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

Comment on attachment 8670671 [details] [diff] [review]
[final] Patch 1: Add utility functions to convert big/little endianness and revise BluetoothPbapManager accordingly, r=shuang

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

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +251,5 @@
>  
>        ReplyToDisconnectOrAbort();
>        AfterPbapDisconnected();
>        break;
>      case ObexRequestCode::SetPath: {

nit:Please fix indentation. Left shitf two spaces. Although BluetoothDaemonHelpers do indent 4 spaces after brace.

@@ +514,5 @@
>  
>    switch (aTagId) {
>      case AppParameterTag::Order: {
> +        using namespace mozilla::dom::vCardOrderTypeValues;
> +        uint32_t order =

nit:Please fix indentation. Left shift two spaces.

@@ +524,2 @@
>      case AppParameterTag::SearchProperty: {
> +        using namespace mozilla::dom::vCardSearchKeyTypeValues;

nit:Please fix indentation.

@@ +537,5 @@
> +      // MDN:Internal_strings suggestion
> +      AppendNamedValue(aValues, "searchText", nsCString((char *) buf));
> +      break;
> +    case AppParameterTag::MaxListCount: {
> +        uint16_t maxListCount = ReadLittleEndianUInt16(buf);

nit:Please fix indentation.

@@ +558,5 @@
> +    case AppParameterTag::Format:
> +      AppendNamedValue(aValues, "format", static_cast<bool>(buf[0]));
> +      break;
> +    case AppParameterTag::vCardSelector: {
> +        bool hasSelectorOperator = aHeader.GetAppParameter(

nit:Please fix indentation.

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +604,5 @@
>    aArray.InsertElementAt(aIndex, BluetoothNamedValue(name, aValue));
>  }
>  
> +uint16_t
> +ChangeEndiannessUInt16(uint16_t aValue)

I prefer to use 'Convert', s/Change/Convert.

Comment 6

[Ben Tian (inactive)]

Attachment: Patch 2 (v3): Notify gaia of PBAP request with one integrated function

Rebase on bug 1212725.

Comment 7

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

Comment on attachment 8672451 [details] [diff] [review]
Patch 2 (v3): Notify gaia of PBAP request with one integrated function

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

The patch looks great. :)
Thank Ben for providing this patch.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +294,5 @@
>        ReplyToSetPath();
>        break;
>      }
>      case ObexRequestCode::Get:
> +      /**

Would you mind to use '/*' instead of '/**' here ?

Mozilla coding style [1] suggests to use JavaDoc-style comments [2].
According to [2], a doc comment must precede a class, field, constructor or method declaration. 

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
[2] http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#format

@@ +455,5 @@
> +    // ... PullvCardListing function uses relative paths. An empty name header
> +    // may be sent to retrieve the vCard Listing object of the current folder.
> +    name = name.IsEmpty() ? mCurrentPath
> +                          : mCurrentPath + NS_LITERAL_STRING("/") + name;
> +

Should we remove this empty line ?

Comment 8

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

Comment on attachment 8672451 [details] [diff] [review]
Patch 2 (v3): Notify gaia of PBAP request with one integrated function

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

I would r+ this.

Comment 9

[Ben Tian (inactive)]

Attachment: [final] Patch 2: Notify gaia of PBAP request with one integrated function, f=jaliu, r=shuang

Revise per Jamin's feedback and rebase on bug 1212729.

Comment 10

[Ben Tian (inactive)]

Attachment: [final] Patch 2: Notify gaia of PBAP request with one integrated function, f=jaliu, r=shuang

Sync comment and log with 2.2r file.

Comment 11

[Ben Tian (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3f5ecfabb19

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/a3b4565c0ee5
https://hg.mozilla.org/integration/b2g-inbound/rev/4b163ef1abb2

Comment 13

[Ben Tian (inactive)]

Attachment: Patch 3 (v1): Restore missing bug 1212729 change

Comment 14

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #13)
> Created attachment 8672909 [details] [diff] [review]
> Patch 3 (v1): Restore missing bug 1212729 change

Comment 9 patch missed one section of bug 1212729 change. This patch 3 restores it.

Comment 15

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

Comment on attachment 8672909 [details] [diff] [review]
Patch 3 (v1): Restore missing bug 1212729 change

Thanks for finding this piece.

Comment 16

[Pulsebot]

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

Comment 17

[Ben Tian (inactive)]

Attachment: [2.2r] Patch 1: Add utility functions to convert big/little endianness and revise BluetoothPbapManager accordingly, r=shuang

Comment 18

[Ben Tian (inactive)]

Attachment: [2.2r] Patch 2: Notify gaia of PBAP request with one integrated function, f=jaliu, r=shuang

Comment 19

[Ben Tian (inactive)]

Set 2.2r blocker for required bluetooth feature PBAP.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3b4565c0ee5
https://hg.mozilla.org/mozilla-central/rev/4b163ef1abb2
https://hg.mozilla.org/mozilla-central/rev/1904c51c0cc3

Comment 21

[Ben Tian (inactive)]

Note for 2.2r only two patches is required (comment 17 and 18) since 2.2r patch 2 already integrates m-c patch 3.

(In reply to Carsten Book [:Tomcat] from comment #20)
> https://hg.mozilla.org/mozilla-central/rev/a3b4565c0ee5
> https://hg.mozilla.org/mozilla-central/rev/4b163ef1abb2
> https://hg.mozilla.org/mozilla-central/rev/1904c51c0cc3

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/bfb53f60821e
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/b1882fcf1a88