969996 - Reduce reflows on call log

Status: RESOLVED FIXED

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

Description

[Yan Or]

Currently the call log generates one reflow per call log group,
as each group is rendered separately.

Reduce reflows by grouping multiple call log groups into a single
render.  Initial group size will be smaller to ensure responsive user
experience.

Comment 1

[Yan Or]

Attachment: Pull request

Comment 2

[Anthony Ricaud (:rik)]

Comment on attachment 8372983 [details] [review]
Pull request

It obviously makes the call log so much more reactive, good low hanging fruit!

I had a hard time understanding the logic because of the 'chunk' word having no meaning (and actually two). Could you please replace it with 'day'?

Here are some new name suggestions:
renderChunkArray -> renderSeveralDays
renderChunks -> daysToRender
FIRST_CHUNK_SIZE -> MAX_GROUPS_FOR_FIRST_RENDER
INCREMENTAL_RENDER_SIZE -> MAX_DAYS_TO_BATCH_RENDER

I'd also like to see some unit tests. And I left one comment in the Pull Request.

Comment 3

[Anthony Ricaud (:rik)]

Comment on attachment 8372983 [details] [review]
Pull request

This looks very nice (and feels very good to get that tested).

I've left comments in the pull request.
One more thing needed is to check in each test that we are calling CallLog.renderSeveralDays the exact number of times we expect. You can spy that method with sinon. You have an exemple in the notifications tests in call_log_test.js.

Comment 4

[Yan Or]

Attachment: Pull request

Comment 5

[Anthony Ricaud (:rik)]

Comment on attachment 8376046 [details] [review]
Pull request

Bugzilla process notes for next time: Since we're still looking at the same pull request, you could have reused the first attachment when asking for review. Also, if you need to send a new patch, you should mark the previous iteration as obsolete.

Excellent! I've left two comments on small details in the pull request. Before landing this, we'll also need to squash all the commits.

Comment 6

[Yan Or]

Attachment: Squashed pull request

Comment 7

[Yan Or]

I tried to squash the commits using the existing branch but couldn't do it.
I had to start a new branch to make it work, hence the new patch.  It has
exactly the same commits (but squashed into a single one).

Also, I don't have merge access to gaia.  Can you please do that once you
are ok with the patch?  Thanks!

Comment 8

[Yan Or]

Was able to squash the commits on the original patch.  Please merge.

Comment 9

[Anthony Ricaud (:rik)]

Sorry for the delay, I had lots of other stuff to look at. Please use needinfo when you want someone to act on a bug, it sends reminders so people don't forget.

Actually, I'm not gonna merge yet because I still have some comments. I don't know why I didn't write those before, sorry :(

Comment 10

[Yan Or]

Updated unit tests based on your last comments.

Comment 11

[Anthony Ricaud (:rik)]

Sorry for this, I was very busy last week and wasn't sure we could land this with the new landing policy.

I'll merge this since it greatly improves performance and it adds a bunch of unit tests for something that wasn't tested before.

Comment 12

[Anthony Ricaud (:rik)]

https://github.com/mozilla-b2g/gaia/commit/09519de9fc717be692a3983b041f24ad4fbda78c