862385 - Cache contact names for the call log

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect, P2)

Description

[Gregor Wagner [:gwagner]]

With bug 847406 we see call log entries much quicker but the actual contact matching can take a long time depending on the size of the log.
We should cache the names or put them into the DB as well.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Blocks a blocker, so leo?

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: wip

Almost there...

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

I still need to review and clean the patch, fix tests, add more tests, file bugs for the TODO comments and test some edge cases, but this patch contains most of what I'd like to have in the final patch. Etienne, could you start taking a look at it, please?

I am also asking f? to Alberto and German, since the patch is kind of risky and they are also familiar with this code.

Sorry for the huge patch :\...

Thanks!

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 759238 [details] [diff] [review]
WIP

I am clearing the feedback flags cause I need to change the patch considerably.

With the current approach, while invalidating the contacts cache, we will show the old version of the cache until the sync with the contacts API data is done and the cache is properly updated. During that time, the user might click on an deleted contact triggering unexpected behaviors. I am afraid that we will need to query the Contacts API db while rendering (as we currently do) if we notice that the contacts cache is not valid. That way, we will always be showing valid information.

Sorry for the noise and the inconvenience. This code was needing some attention.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: wip

Again, sorry for the huge patch.

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: wip

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 10

[Alex Keybl [:akeybl]]

No longer blocking a blocker.

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

We decided to split the work required for bug 847406 to ease the review process and lower the regression risk, that's why we removed the dependency (1 month ago) but we kept working on this and didn't nominate it again for triage as we still considered this as a blocker for the performance improvements required for the call log.

As it is no longer possible to ask for approval for uplift to 1.1, I would like to ask you to reconsider this for leo+ again.

Please, note that apart from introducing the contacts cache for the call log, the attached patch fixes some other issues that would be hard and time consuming to identify and fix again.

Comment 12

[bhavana bajaj [:bajaj]]

needsinfo on mlee/:ferjm to understand if the performance improvements on contacts cache are in scope for 1.1  ?

Comment 13

[Wayne Chang [:wchang]]

Triage - leaving as is waiting for mlee's response to make decision.

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 763470 [details] [diff] [review]
v1

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

With the comments addressed we should be in good shape to start the review!
Given the size of the patch I think it's worth having both German and me as reviewers :)

::: apps/communications/dialer/js/call_log.js
@@ +51,5 @@
> +          });
> +          self._contactCache = false;
> +        } else {
> +          self._contactCache = true;
> +        }

will look better with 2 early returns :)

@@ +443,5 @@
> +    if (contact && contact.matchingTel) {
> +      var phoneNumberAdditionalInfo =
> +        Utils.getPhoneNumberAdditionalInfo(contact.matchingTel);
> +      if (phoneNumberAdditionalInfo && phoneNumberAdditionalInfo.length) {
> +        var additInfo = document.createElement('p');

nit -> |addInfo| looks better :)

@@ +689,5 @@
> +                                                    phoneNumbers, target) {
> +    var self = this;
> +
> +    // We need these aux functions to keep the correct references to the log
> +    // DOM element.

should'nt we make those methods on the CallLog singleton?

@@ +727,5 @@
>  
> +    function _removeContact(log, contactId, updateDb) {
> +      // If the cache is valid, we also need to remove the contact from the
> +      // cache
> +      if (CallLog._contactCache && updateDb) {

it won't look like you're accessing a private var on CallLog if _removeContact is a method on CallLog :)

@@ +755,5 @@
> +        logs = container.querySelectorAll('li[class="log-item"]');
> +        break;
> +    }
> +
> +    for (var i = 0, l = logs.length; i < l; i++) {

|l| is indeed better than |j| :)

@@ +780,5 @@
> +   * param matchingTel
> +   *        Object containing the phone number associated with the group of
> +   *        calls. It contains 'type', 'carrier' and 'value' parameters.
> +   *
> +   */

I think we could get some pretty decent coverage for this method (and we can probably focus on this one).

What to you think?

(I know it's dom-heavy but I think it's worth it.)

@@ +785,5 @@
> +  updateContactInfo: function cl_updateContactInfo(element, contact,
> +                                                   matchingTel) {
> +    var primInfoCont = element.getElementsByClassName('primary-info-main')[0];
> +    var contactPhoto = element.querySelector('.call-log-contact-photo');
> +    var additInfo = element.getElementsByClassName('call-additional-info');

same, `addInfo` or `addlInfo` (like the additional review on bugzilla :))

@@ +867,5 @@
> +  var options = {
> +    filterBy: ['id'],
> +    filterOp: 'equals',
> +    filterValue: event.contactID
> +  };

out of the scope of the patch, but I wonder if it would make sense to get the contact object back from the API on the event.

::: apps/communications/dialer/js/call_log_db.js
@@ +21,5 @@
> +
> +  /**
> +   * Add a observer of the 'upgradedone' event, which is fired as soon as we
> +   * push the latest upgraded record into the upgraded object stores.
> +   */

it may be a tiny bit overkill (since we don't have the use case for multiple observers).
but if we add unit-tests for it it's fine :)

@@ +69,5 @@
>        callback(null, this._db);
>        return;
>      }
>  
> +    LazyLoader.load(['/dialer/js/utils.js', '/dialer/js/contacts.js']);

note sure I get it... no callback?

@@ +367,5 @@
> +          groupCount++;
> +        }
> +        // Once we built all the group of calls we can push them to the
> +        // groups object store and notify the UI about it.
> +        if (cursorDone && !waitForAsyncCall) {

isn't it technically possible that the last contact request comes in before the last cursor onsuccess (where we set cursorDone to true)?

maybe we should extract this in a function and call it in each mozContact request callback _and_ in the last cursor tick.
since it will start with
```
if (!cursorDone || waitForAsyncCall) {
  return;
}
```
it won't cost much to be safe :)

@@ +715,5 @@
>     *                        error message if needed.
>     */
> +  _getList: function getList(storeName, callback, sortedBy, prev,
> +                             getCursor, limit) {
> +    if (!callback || !callback instanceof Function) {

idem, precedence question

@@ +954,5 @@
>                   function(error, txn, store) {
> +      if (error) {
> +        self._asyncReturn(callback, error);
> +        return;
> +      } else if (!contact) {

we're early returning just before, no need for the else

::: apps/communications/dialer/js/contacts.js
@@ +186,5 @@
>      };
>    },
>  
> +  _mergeFbContacts: function _mergeFbContacts(contacts, callback) {
> +    if (!callback || !callback instanceof Function) {

not sure, but I think |!| may have a higher precedence than |instanceof|.

Anyway, parenthesis probably won't hurt :)

@@ +201,5 @@
> +        if (fb.isFbLinked(contacts[i])) {
> +          var fbReq = fb.contacts.get(fb.getFriendUid(contacts[i]));
> +          fbReq.onsuccess = function() {
> +            contacts[i] = fb.mergeContact(contacts[i], fbReq.result);
> +            if (i === j - 1) {

yeah this one works, but parenthesis won't hurt either :)

@@ +209,5 @@
> +          fbReq.onerror = function() {
> +            console.error('Could not merge Facebook data');
> +            callback(contacts);
> +          };
> +        } else if (i === j - 1) {

same.

while nit picking, we could probably rename `j` into `length` or something like that.

::: apps/communications/dialer/js/utils.js
@@ +56,5 @@
> +   * phone number subject of the call consists in the type and carrier
> +   * associated with this phone number.
> +   *
> +   * Each call is associated with an *unique number* and this phone number can
> +   * belong to an specific contact(s). We don't care about the contact having

to n specific contact(s) maybe?

Comment 15

[Wayne Chang [:wchang]]

Triage- Agree to minus for now. Please renom when landed on Master and showing low or no risk for uplifting.

Comment 16

[Mike Lee [:mlee]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #11)
>
> ...
> 
> Please, note that apart from introducing the contacts cache for the call
> log, the attached patch fixes some other issues that would be hard and time
> consuming to identify and fix again.

Fernando,

What's the performance timing impact of not uplifting this patch to 1.1? How slow is the call log without your patch and how much improvement (in ms/s) do we see with it?

Also, what're the "other issues that would be hard and consuming to identify and fix"?

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Thanks for the feedback Etienne! It'll take a few days more for me to have a new patch since this got leo-'ed and I have a few other leo+ to attend first.

(In reply to Mike Lee [:mlee] from comment #16)
> (In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC,
> please) from comment #11)
> >
> > ...
> > 
> > Please, note that apart from introducing the contacts cache for the call
> > log, the attached patch fixes some other issues that would be hard and time
> > consuming to identify and fix again.
> 
> Fernando,
> 
> What's the performance timing impact of not uplifting this patch to 1.1? How
> slow is the call log without your patch and how much improvement (in ms/s)
> do we see with it?

Unfortunately, I don't have any numeric measures :(

Without this patch, the call log is rendered pretty fast thanks to bug 847406 *but* only the number list, once it is first rendered, the user can notice how the list is traversed to add the contact names (and other info like pictures) one by one. This makes scrolling slow if the call log is big and there are many contacts associated to the numbers in the list.

> Also, what're the "other issues that would be hard and consuming to identify
> and fix"?

I am afraid that I can't say the whole list of small (and not that small) issues that I found while working on this patch, but IIRC some of them were:

- The time of the calls were always 12:00
- Some use cases for contacts updates, like a contact deletion or a contact with different numbers which primary number is deleted that didn't trigger a proper call log update.
- The additional information were shown as ", <number>" instead of "<type>, <number> if there were no type associated to the number.

I don't know if any of these issues have already been fixed in other bugs though.

Sorry for not been able to write a more detailed list of issues, but this was supposed to be leo+ (it was for a month at least) as a complement of the complete call log rewrite for bug 847406. That's why I didn't filed separated bugs for these issues that I found on my way to fix this bug.

In any case, I'm ok with comment 15 and I'll work based on that :)

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

I've only rebased the patch. I'll be addressing Etienne's feedback next week.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

This patch should address Etienne's feedback.

I'd like to improve the tests for the call log UI (there were 0 before this patch :(...), but I really have not much time to enjoy that task right now, so I'll file another bug for it.

Sorry again for the 2.6K lines patch :(

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 772767 [details] [diff] [review]
v1

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

Actually, I'd like to do a few more local testing before asking for a real review :) Sorry for the noise.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Blocking a blocker, again.

Comment 24

[Dietrich Ayala (:dietrich)]

Based on erroneous testing, we've flipped to minus on bug 889724.

So minusing this one for now.

Given the test scenario and 1.5s requirement on bug 889724, can you please test with 250 calls, and ensure load time is under 1.5s?

If it's greater than 1.5s, please renominate this bug for blocking. Thanks!

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

https://bugzilla.mozilla.org/show_bug.cgi?id=889724#c5

Comment 26

[Wayne Chang [:wchang]]

Blocking a leo+ blocker. Set to leo+

Comment 27

[Etienne Segonzac (:etienne)]

Comment on attachment 773373 [details] [diff] [review]
v1

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

Boom, when the call log goes boom,
everything is here, boom,
no weird rendering glitches...

To the tune of: http://www.youtube.com/watch?v=dYzEOaqazdM#t=00m17s

r=me with the "upgrading" label and the unit tests nits.
Great great work!

I played with it a lot and didn't find any issue.
Wasn't set up to go through the bluetooth redial scenario though, but we could mitigate this by adding a test for the getLastCall method ;)

::: apps/communications/dialer/js/call_log.js
@@ +112,5 @@
> +
> +    // Listen for database upgrade events.
> +    CallLogDBManager.onupgradeneeded = function onupgradeneeded() {
> +      // TODO: Show UI letting the user know that the database is being
> +      //       upgraded or whatever. Bug XXXXXX

After testing it a bit, I think it's worth displaying a simple "upgrading..." string for this patch.

::: apps/communications/dialer/js/call_log_db.js
@@ +984,5 @@
> +   *        needed.
> +   *
> +   * return (via callback) the last call or an error message if needed.
> +   */
> +  getLastCall: function getLastCall(callback) {

looks like we don't have tests for this method.

::: apps/communications/dialer/test/unit/calllog_test.js
@@ +19,5 @@
> +      get: function get(cb) {
> +        cb(function l10n_get(key) {
> +          return key;
> +        });
> +      }

note: we actually already have a mock ready for LazyL10n.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Etienne Segonzac (:etienne) from comment #27)
> Comment on attachment 773373 [details] [diff] [review]
> v1
> 
> Review of attachment 773373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Boom, when the call log goes boom,
> everything is here, boom,
> no weird rendering glitches...
> 
> To the tune of: http://www.youtube.com/watch?v=dYzEOaqazdM#t=00m17s

lol :D

> 
> r=me with the "upgrading" label and the unit tests nits.
> Great great work!
> 

Thanks! :)

> I played with it a lot and didn't find any issue.
> Wasn't set up to go through the bluetooth redial scenario though, but we
> could mitigate this by adding a test for the getLastCall method ;)
> 
> ::: apps/communications/dialer/js/call_log.js
> @@ +112,5 @@
> > +
> > +    // Listen for database upgrade events.
> > +    CallLogDBManager.onupgradeneeded = function onupgradeneeded() {
> > +      // TODO: Show UI letting the user know that the database is being
> > +      //       upgraded or whatever. Bug XXXXXX
> 
> After testing it a bit, I think it's worth displaying a simple
> "upgrading..." string for this patch.
> 

I'm not sure that adding a new string to v1-train is still allowed :(... Axel, is it still possible?

Comment 29

[Axel Hecht [:Pike]]

Please ask triage about schedule and approvals, https://groups.google.com/forum/#!topic/mozilla.dev.gaia/_T_BTvJ4fJk.

That said, we're way past string-freeze.

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Axel Hecht [:Pike] from comment #29)
> Please ask triage about schedule and approvals,
> https://groups.google.com/forum/#!topic/mozilla.dev.gaia/_T_BTvJ4fJk.
> 

This bug already has the leo+ approval.

> That said, we're way past string-freeze.

I'll take that as a "no".

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

This patch includes the tests suggestions and a progress bar for the upgrade process that includes a percentage so we avoid adding new strings.

Comment 32

[Etienne Segonzac (:etienne)]

Comment on attachment 776499 [details] [diff] [review]
v2

\o/

Comment 33

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! Sorry for the delay, many bugs in the oven :-p

OK, I think I found a misbehavior which may be related to this bug and more concretely to call_log.js's updateContactInfo(). I cannot assure it since we may had it from the past. To reproduce it:
1.- Have or create a contact for a phone number you may be able to call your DuT back to. Let's say 666666666.
1.- With no entries in the the Call Log (at least that was my case), from the DuT make an outgoing call to 666666666.
2.- Make a couple of calls from 666666666 to the DuT. At this point you'll have 2 entries in your Call Log with contacts associated: 1 including the 2 incoming (missed in my case) calls and 1 including the outgoing call.
3. Delete the contact.
Expected result:
  - The entries in the Call Log have no contact associated but maintain the number of calls in the group, this is (2) for the incoming (missed) calls.
Observed result:
  - The entries in the Call Log have no contact associated but the number of calls in the group is lost. No (2) in the third row of the group including the 2 incoming (missed) calls just made.

BTW, updating the contact seems to work fine. For example, deleting the image in case it had one.

Do you reproduce it, Fernando? Has it something to do with your patch? Thanks!

Comment 34

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

Thanks Germán! Nice catch!

Comment 35

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 777675 [details] [diff] [review]
v3

r=etienne from comment 32

Comment 36

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 777675 [details] [diff] [review]
v3

If this is the same patch you sent to me via Skype, I r+ it after testing that the previous problem is solved and everything seems to work just fine... Although bug will come... :-p Thanks Fernando! Awesome patch! ;-)

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Thanks Germán!

https://github.com/mozilla-b2g/gaia/commit/fc7a7cd5a593bea612706eee9449400374da4b39

Comment 38

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  fc7a7cd5a593bea612706eee9449400374da4b39
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

v1-train: https://github.com/mozilla-b2g/gaia/commit/41d10fb10be6916e6554eb440d9a97130ef23ce0

Comment 40

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 41d10fb10be6916e6554eb440d9a97130ef23ce0