921683 - dynamic creation of contact list groups causes reflow

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Profiling during bug 914913 shows that there is a small reflow after the dynamic creation of the list group DOM structures in list.js's renderGroupHeader().

While we might get a small win by making this static, we can probably defer the creation of the group list DOM until we have a contact that needs to go in that list.  This should avoid both the reflow and help reduce the DOM.

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

triage: koi+ for improving contact performacne

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Working code is here:

  https://github.com/wanderview/gaia/compare/contacts-dynamic-group?expand=1

I need to fix the tests, though, as they are all pretty much written to expect the group list DOM to already exist.  Sadness.  :-(

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Also, I believe this profile confirms that this change does indeed prevent the reflow after the contacts init:

  http://people.mozilla.org/~bgirard/cleopatra/#report=07992ac7fde9d1820d89782faae5d026a3f5a42c

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

My contacts-dynamic-group branch linked in comment 2 has been updated with fixes for most of the tests.

One test is still failing.  Debugging suggests this might actually be a bug in tag_visibility_monitor, so it will take longer to figure out.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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

Finally tracked down that last test issue.  It was not a bug in visibility monitor, but instead a problem due to the test DOM missing some CSS which forced scrolling to occur.

Here is the pull request.  This defers the group creation until its first needed.  Profiling shows that this finally removes the reflow after the contacts.List.init() function.

Again, it currently hardcodes the expected names of the groups in order to provide ordering.  I did this partly in anticipation of adding more scripts.

Comment 6

[Jose Manuel Cantera]

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

good work Ben

thanks!

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Finally got travis green.  Landed:

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

Thanks for the review Jose!

Comment 8

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

Uplifted cbb6174cd51222fbc3d3b1d4e12620666aceca38 to:
v1.2: e9533a66b441b902a77966da87144f89e1db808e

Comment 9

[Fabien Cazenave [:kaze]]

We also need to uplift this patch to v1-train in order to uplift bug 914942.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Do we need to nom for leo? then?

Comment 11

[Jose Manuel Cantera]

is v1-train still an active branch?

Comment 12

[Fabien Cazenave [:kaze]]

Bug 914942 is leo+ and it depends on this patch now…

I think we should uplift this patch if it’s straight-forward; otherwise I can still work on a leo-specific patch for bug 914942.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I think this should probably uplift cleanly.  I'm more wondering who can give us leo+ ?  Is there a manager you are working with aware of the effort that needs this uplift?

Comment 14

[Fabien Cazenave [:kaze]]

John, are you the right person to ask?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

I'll go ahead an nom for leo? based on comment 9.  Just need someone to make a call on it.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Also needinfo Preeti about whether we can leo+ this bug since it blocks bug 914942.

Comment 17

[Preeti Raghunath(:Preeti)]

Hi Ben,

leo has already shipped so we cannot leo+ this bug.

We can however 1.1HD+ it. Let me know.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Preeti Raghunath(:Preeti) from comment #17)
> leo has already shipped so we cannot leo+ this bug.
> 
> We can however 1.1HD+ it. Let me know.

Would that let us uplift it to v1-train?  Is there going to be a 1.1.1 release and which branch/flag would that use?

Thanks for the quick reply.

Comment 19

[Fabien Cazenave [:kaze]]

See bug 914942 comment 20, I think I should probably write another patch rather than request a bunch of dependency uplifts.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Ok. Dropping leo? nom based on comment 19.  Note, I could not put the flag back at koi+.