Closed Bug 921683 Opened 6 years ago Closed 6 years ago

dynamic creation of contact list groups causes reflow

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v1.2 fixed)

RESOLVED FIXED
1.2 C3(Oct25)
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

(Keywords: perf, Whiteboard: [c= p=3 s=2013.10.04 u=1.2],QARegressExclude)

Attachments

(1 file)

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.
blocking-b2g: --- → koi?
Whiteboard: [c= p=3 s= u=] → [c= p=3 s=2013.10.04 u=]
triage: koi+ for improving contact performacne
blocking-b2g: koi? → koi+
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.  :-(
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
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.
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.
Attachment #813806 - Flags: review?(jmcf)
Attachment #813806 - Attachment mime type: text/plain → text/html
Comment on attachment 813806 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/12673

good work Ben

thanks!
Attachment #813806 - Flags: review?(jmcf) → review+
Finally got travis green.  Landed:

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

Thanks for the review Jose!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Uplifted cbb6174cd51222fbc3d3b1d4e12620666aceca38 to:
v1.2: e9533a66b441b902a77966da87144f89e1db808e
Whiteboard: [c= p=3 s=2013.10.04 u=] → [c= p=3 s=2013.10.04 u=],QARegressExclude
Target Milestone: --- → 1.2 C3(Oct25)
Whiteboard: [c= p=3 s=2013.10.04 u=],QARegressExclude → [c= p=3 s=2013.10.04 u=1.2],QARegressExclude
Blocks: 914942
We also need to uplift this patch to v1-train in order to uplift bug 914942.
Do we need to nom for leo? then?
is v1-train still an active branch?
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.
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?
John, are you the right person to ask?
Flags: needinfo?(jhford)
I'll go ahead an nom for leo? based on comment 9.  Just need someone to make a call on it.
blocking-b2g: koi+ → leo?
Also needinfo Preeti about whether we can leo+ this bug since it blocks bug 914942.
Flags: needinfo?(praghunath)
Hi Ben,

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

We can however 1.1HD+ it. Let me know.
(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.
See bug 914942 comment 20, I think I should probably write another patch rather than request a bunch of dependency uplifts.
Ok. Dropping leo? nom based on comment 19.  Note, I could not put the flag back at koi+.
blocking-b2g: leo? → ---
Flags: needinfo?(praghunath)
Flags: needinfo?(jhford)
You need to log in before you can comment on or make changes to this bug.