847406 - Refactor call log rendering algorithm

Status: VERIFIED FIXED

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

Comment 1

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Hi Etienne! This is a proposal about the refactor in call-log. Could you take a first look? Thanks!

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

leo? based on https://bugzilla.mozilla.org/show_bug.cgi?id=857398#c7

Comment 3

[Dylan Oliver [:doliver]]

Fernando, can someone else pick up this work since Borja will focused on MMS for the next couple of weeks?

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

If I am not wrong, Alberto is already working on this.

Comment 6

[Borja Salguero [:borjasalguero]]

Alberto is gonna continue with this! ;)

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Also I suggest to use caching + visibility_monitor.js for all lists. That will make it way less expensive to render a big list and even for what is rendered the work will be done once.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I opened bug 865741 as a meta btw.

Comment 9

[Dylan Oliver [:doliver]]

Alberto, can you provide current status on this bug?

Comment 10

[alberto.pastor]

Hi there, I'm working right now on this.

https://github.com/albertopq/gaia/tree/callog-refactor

The only remaining thing is related to store the contacts info in the Call-log DB. Should be ready to be reviewed later today.

Comment 11

[alberto.pastor]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9545

Pointer to Github pull-request

Comment 12

[alberto.pastor]

I finally didn't add the contacts part in this PR. Is almost ready, but given the complexity on cases when updating contacts, and the current performace, let's see if caching contacts is finally necessary. We will track that part on Bug 862385.

You can check current results with 500 groups here:

https://www.dropbox.com/s/7zuykfaveltcq0z/dialer.m4v

I think this PR + Bug 847409 (Inifinite scrolling), could be enough, but let's check with the partners

Comment 13

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

Gorgeous! ;-)

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 745231 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9545

Drive-by feedback and random nit picking.

f- mostly because the DB schema and tests need to be updated with the new 'status' field.

Thanks for working on this Alberto :) great job so far!

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 745231 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9545

Started commenting on github.

FTR, even if I won't block on it: the choice of keeping the contacts out of the backend doesn't make much sense to me.
The current contacts-related code paths are a complete mess and I was really hoping to clean these.

And more generally not end up with 400 lines of rendering code after a _complete_ rewrite of the backend...

That said, I'm both aware of the massive amount of work that went into the refactoring and the huge performance gain. So let's fix the issues reported on github and get this boost!

Comment 16

[alberto.pastor]

I agree on storing the contact details in the backend if we re-check the info after rendering it, instead of relying on the oncontactchange event and the complex use cases we could find for updating the cache (or when the app is closed).
We need to take in account the stage we are and being aware that minimize the risks is also a priority. 

That said, I already addressed the comments. :ferj is working in the remaining stuff regarding the DB (multientry id for the groupsStore). Hopefully tomorrow everything will be landeable.

Thanks!

(In reply to Etienne Segonzac (:etienne) from comment #15)
> Comment on attachment 745231 [details]
> Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9545
> 
> Started commenting on github.
> 
> FTR, even if I won't block on it: the choice of keeping the contacts out of
> the backend doesn't make much sense to me.
> The current contacts-related code paths are a complete mess and I was really
> hoping to clean these.
> 
> And more generally not end up with 400 lines of rendering code after a
> _complete_ rewrite of the backend...
> 
> That said, I'm both aware of the massive amount of work that went into the
> refactoring and the huge performance gain. So let's fix the issues reported
> on github and get this boost!

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 745231 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9545

Thanks Alberto! This looks great! I left just a few minor comments in the PR.

I would really love to see the CallLogDBManager.deleteAll() method used in this PR, but it is ok if you want to do it as part of bug 860612.

Comment 18

[Etienne Segonzac (:etienne)]

Comment on attachment 745231 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9545

Comments on github, ping me when it's ready!

Comment 19

[Etienne Segonzac (:etienne)]

Comment on attachment 745231 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9545

Thanks for all the work!

r=me (c44b7cd06fb6e3ef196ad2a2cf8c5d72fe1403fd)

We still need:
- to squash the commits
- to make sure we have a nice QA round on this before uplifiting
- to open a bug for the proper contact caching

Comment 20

[alberto.pastor]

Thanks etienne for the hard review!

- commits squashed
- I don't know who in QA could help us with this, but I'll find out
- The caching contacts par will be tracked in Bug 862385

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

Comment 21

[Borja Salguero [:borjasalguero]]

+1!!!

Comment 22

[Gareth Aye [:gaye] (back from PTO)]

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 -m1 a6bd75d7c26c1589cd8b6d2ea4859cde0b636cba
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 23

[Etienne Segonzac (:etienne)]

Flagging Naoki for a QA check as discussed during the gaia weekly meeting.

Comment 24

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

There is only one bug that I definitely could say is due to this code change:
Bug 874671 - First time going into the call log after reboot or reopening the dialer app doesn't show focus on All.

Here are the list of other bugs that I ran across:
New bugs:
Bug 874665 - getting a call from a phone that gets interrupted via the telecom, will leave the b2g phone in a wierd pick up state

bug 874639 	If you have the same number for 2 different contacts, dialer will always connect you through the last created contact. 

bug 874568 	Crash occurs w/ MC when getting hung up right after answering a call. 

Bug 874668 - Phone does not wake by itself when receiving a call

Old Bugs :
bug 857484 Proximity sensor does not work on Ikura

bug 868207

Bug 834530 - Emergency dialer DTMF tones aren't regulated by the volume button (and are different volume from Dialer App's DTMF tones)

Bug 855040 - [Crash Reporter] Crash reporter is not sending system crashes after b2g restart itself.

There's some instability and I would not like to push this to v1.1 just yet until we figure out what's causing some of the crashing... it looks like several different crashes, 1 of which is graphic related.

Comment 25

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

There looks like some more changes to the dialer that landed today, so I will have to check tomorrow's build as well.

Comment 26

[Dylan Oliver [:doliver]]

Naoki, do you have time to re-evaluate this today?

Comment 27

[Gareth Aye [:gaye] (back from PTO)]

So is this already landed on v1-train?

Comment 28

[Dylan Oliver [:doliver]]

No, it's just on master. We have asked Naoki to do some testing & validation there before uplifting to v1-train.

Comment 29

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I found a new bug:  bug 875581

I'm still in the midst of testing, but that's enough to block it from landing on v1-train.

Comment 30

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Turns out that the bug I found isn't related, it happened just recently.

There's still some crashing w/ the phone, though I heard it was more gecko related than the changes here.

Please let me know as soon as this lands in v1-train so I can do further testing.

Comment 31

[alberto.pastor]

v1-train: e94cfe9923e550b17ed4acca3c3b90aa1493f8ce

Comment 32

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Marking as verified:

Had to build my own build for gaia.
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/762354023009
Gaia   4d10e1297b859cacc174c0a54af61a7678d7c32d
BuildID 20130522070207
Version 18.0

Ran into :
https://bugzilla.mozilla.org/show_bug.cgi?id=875901
https://bugzilla.mozilla.org/show_bug.cgi?id=875913
https://bugzilla.mozilla.org/show_bug.cgi?id=875920

I think this needs to be push to b2g18:
https://bugzilla.mozilla.org/show_bug.cgi?id=875227

No crashing occured.  Looks like the crashing is master related.