Closed
Bug 1200091
Opened 9 years ago
Closed 9 years ago
PBAP Gaia Reference Design
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(feature-b2g:2.2r+, 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 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8654695 [details]
[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 | ||
Updated•9 years ago
|
Assignee: nobody → selee
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
Updated•9 years ago
|
feature-b2g: --- → 2.2r+
Comment 4•9 years ago
|
||
Comment on attachment 8654695 [details]
[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)
Assignee | ||
Comment 5•9 years ago
|
||
WIP patch only for further usage.
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8654695 [details]
[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 7•9 years ago
|
||
Comment on attachment 8654695 [details]
[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-
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8654695 [details]
[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 9•9 years ago
|
||
Comment on attachment 8654695 [details]
[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-
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8654695 [details]
[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 11•9 years ago
|
||
Comment on attachment 8654695 [details]
[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+
Assignee | ||
Comment 12•9 years ago
|
||
Gabriele, thanks for your review. :)
landed on v2.2r: https://github.com/mozilla-b2g/gaia/commit/bbf3bf4c8eaab837c220f2c4ea3e3088dc075d57
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
status-b2g-v2.2r:
--- → fixed
Updated•9 years ago
|
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8693995 [details]
[gaia] weilonge:seanlee/BT/master/Bug1200091 > mozilla-b2g:master
This patch is only for reference in master branch.
Flags: needinfo?(wiwang)
You need to log in
before you can comment on or make changes to this bug.
Description
•