Closed Bug 1200091 Opened 5 years ago Closed 5 years ago

PBAP Gaia Reference Design

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
feature-b2g 2.2r+
Tracking Status
b2g-v2.2r --- fixed

People

(Reporter: selee, Assigned: selee)

References

Details

Attachments

(3 files)

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 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. :)
Attachment #8654695 - Flags: review?(gsvelto)
Assignee: nobody → selee
Status: NEW → ASSIGNED
Depends on: 1180554
Blocks: 892179
feature-b2g: --- → 2.2r+
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.
Attachment #8654695 - Flags: review?(gsvelto) → review?(francisco)
WIP patch only for further usage.
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.
Attachment #8654695 - Flags: review?(francisco) → review?(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
Attachment #8654695 - Flags: review?(gsvelto) → review-
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! :)
Attachment #8654695 - Flags: review- → review?(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.
Attachment #8654695 - Flags: review?(gsvelto) → review-
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!
Attachment #8654695 - Flags: review- → review?(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.
Attachment #8654695 - Flags: review?(gsvelto) → review+
Gabriele, thanks for your review. :) 
landed on v2.2r: https://github.com/mozilla-b2g/gaia/commit/bbf3bf4c8eaab837c220f2c4ea3e3088dc075d57
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
See Also: → 1225757
Comment on attachment 8693995 [details] [review]
[gaia] weilonge:seanlee/BT/master/Bug1200091 > mozilla-b2g:master

This patch is only for reference in master branch.
Flags: needinfo?(wiwang)
Hi Sean,

Thank you,
your patch works perfectly :)
Flags: needinfo?(wiwang)
You need to log in before you can comment on or make changes to this bug.