1200091 - PBAP Gaia Reference Design

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect)

Description

[Sean Lee [:seanlee][:weilonge]]

PBAP is a partner-request feature in v2.2r, so any PBAP reference design in Gaia part will be implemented and discussed in this bug.
This reference implementation is for partner to verify Gecko PBAP API.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:pbap > mozilla-b2g:v2.2r

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8654695 [details] [review]
[gaia] weilonge:pbap > mozilla-b2g:v2.2r

Hi Gabriele,

Could you help to review PBAP codes for the device branch v2.2r?

All PBAP related codes are placed in apps/communications/pbap folder.
PBAP module has to handle the following three events:
    adapter.onpullphonebookreq = pullphonebook;       // Pull Phonebook request
    adapter.onpullvcardentryreq = pullvcardentry;     // Pull vCard entry
    adapter.onpullvcardlistingreq = pullvcardlisting; // Pull vCard listing request

https://github.com/mozilla-b2g/gaia/pull/31594/files#diff-bcf171e725b33a2276a0c0137a2c7bfcR91

Before doing Pull vCard entry, Pull vCard listing MUST be performed for creating the caching table first, so the vCard entry can be mapped to Contacts API. Besides, there will be a following bug to implement the call log feature for PBAP.

Thank you. :)

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Reference:

PBAP  https://developer.bluetooth.org/TechnologyOverview/Pages/PBAP.aspx

Comment 4

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8654695 [details] [review]
[gaia] weilonge:pbap > mozilla-b2g:v2.2r

This looks like a change that affect contacts and I'm the owner for the dialer part of the communications app so I'm handing over the review to the contacts owner.

That being said I've quickly glanced at the code and noticed that it contains quite a few calls to console.log() used for debugging as well as commented-out code. I suggest removing both before asking for review as debugging-only bits should not land once the patch is working.

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Attachment: PBAP reference design - skeleton version with debugging feature.

WIP patch only for further usage.

Comment 6

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8654695 [details] [review]
[gaia] weilonge:pbap > mozilla-b2g:v2.2r

Hi Gabriele,

Thanks for your response.
I think the patch will need the call log feature so PBAP feature will be launched when Dialer is opended.

Could you help to review it again? Thank you very much.

Comment 7

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8654695 [details] [review]
[gaia] weilonge:pbap > mozilla-b2g:v2.2r

The code looks generally good but I've left a few review comments to address. Besides those comments I'd like to see:

- Some in-code comments to describe what each function does

- Some unit-tests if it makes sense to do so. This code is completely devoid of tests and as such it wouldn't be able to land on master but I don't know what are the landing rules or constraints for v2.2r

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8654695 [details] [review]
[gaia] weilonge:pbap > mozilla-b2g:v2.2r

Hi Gabriele,

Based on your suggestions, the codes had been improved and added comments.
It's a reference design for our partner in the current stage, so I would like to implement the test which must be implemented when the PBAP feature starts to land to master.
Could you help to review my patch again? Thank you! :)

Comment 9

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8654695 [details] [review]
[gaia] weilonge:pbap > mozilla-b2g:v2.2r

The patch is looking good save for some minor nits I've commented on in the PR but this needs at very least some basic unit-tests covering the three event handlers before landing. Once those are done ask for review again and we can land this.

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8654695 [details] [review]
[gaia] weilonge:pbap > mozilla-b2g:v2.2r

Hi Gabriele,

Sorry for my late reply. :$ Busy on too much things.
Could you help to review the patch again?
I added some unit test for this.

Thank you!

Comment 11

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8654695 [details] [review]
[gaia] weilonge:pbap > mozilla-b2g:v2.2r

Thanks for adding unit-tests, this is good to land on v2.2r. If you intend to port this on master too though we should remove the mozContacts mock you added and rely on the shared one instead.

Comment 12

[Sean Lee [:seanlee][:weilonge]]

Gabriele, thanks for your review. :) 
landed on v2.2r: https://github.com/mozilla-b2g/gaia/commit/bbf3bf4c8eaab837c220f2c4ea3e3088dc075d57

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:seanlee/BT/master/Bug1200091 > mozilla-b2g:master

Comment 14

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8693995 [details] [review]
[gaia] weilonge:seanlee/BT/master/Bug1200091 > mozilla-b2g:master

This patch is only for reference in master branch.

Comment 15

[Will Wang [:WillWang]]

Hi Sean,

Thank you,
your patch works perfectly :)