921685 - contacts auto-detection of "rows per screen" causes sync reflow

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

While profiling bug 914913 I noticed that we are triggering a sync reflow during load due to the code that auto-detects the number of rows that will fit in a single page on the device.  This is due to getBoundingClientRect() in appendToList().  The reflow is fairly small and contained, taking about 10ms to 20ms, but it would be nice to remove it if we could.

I'm not sure what to do about this one.  It seems detecting the page size is a legitimate thing to do given that we have devices with different dimensions.  The most reliable way to do this is based on the final calculated CSS style.

Perhaps we could set a "minimum page size" reflecting our smallest handsets and defer calculating the true rowsPerPage value until we've rendered at least that many contacts.  This could at least help our time to display the first page of contacts on these smaller handsets.

I'm open to other suggestions too.

Comment 1

[Jose Manuel Cantera]

could we do this with window.innerHeight or via a prefilled value at build time?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jose M. Cantera from comment #1)
> could we do this with window.innerHeight or via a prefilled value at build
> time?

So its not the determining the viewport size that is the issue, but instead determining how large each row is.  We can either get the row size specifically or the full list size and then divide by number of rows, but either way we cause a sync flush in order to calculate the actual size.  (Row vs full list should be pretty equivalent at the moment, by the way, because we do it when we only have one item in the row.)

Prefilled at build time would be possible, but would be kind of a pain for testing.

I suppose we could cache it in the cookie along with the list ordering.  On its own that would seem a bit over the top, but since we're looking at the cookie anyway...

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Taking this one to try to further improve 914913.  I'm going to pursue the cookie path since we already have a config value stored there.  Since the geometry of the device is fixed we should only have to pay the sync reflow price once.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Work in progress on this branch.

  https://github.com/wanderview/gaia/compare/contacts-size-reflow?expand=1

So far I have just refactored the cookie code a bit to allow me to insert additional values.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Nom koi? since this koi+ block 914913.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Work-in-progress branch has been updated.  This appears to be working and I've verified in the profiler that the sync reflow is avoided.  Just need to clean up for review.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Pull request at https://github.com/mozilla-b2g/gaia/pull/13043

This pull request avoids the sync reflows from rowsPerScreen detection by storing detected value in the cookie.

Comment 8

[Jose Manuel Cantera]

Hi Ben,

The PR is in good shape, though I left some comments on Github.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Thanks Jose.  I've updated the pull request based on your suggestions.  Let me know what you think.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 821205 [details] [review]
Pull request at https://github.com/mozilla-b2g/gaia/pull/13043

Moving the r? to Francisco since Jose is out on PTO.

Comment 11

[Jose Manuel Cantera]

Just to note I'm back and could resume the review if you agree

thanks

Comment 12

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 821205 [details] [review]
Pull request at https://github.com/mozilla-b2g/gaia/pull/13043

Great job, as always.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jose M. Cantera from comment #11)
> Just to note I'm back and could resume the review if you agree

I was out yesterday, so didn't see this till now.  I'm going to go ahead and run with Francisco's r+ unless you have any objections.  Thanks to you both!

Merged:

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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Uplifted to v1.2 branch:

  https://github.com/mozilla-b2g/gaia/commit/1bc6de4f21f66bd8e931be36fc2ba9f99dae3732