973407 - [Messages] Reduce reflows when element heights are updated

Status: RESOLVED FIXED

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

Description

[Yan Or]

When element heights need to be updated, several reflows are triggered.
This happens when opening the Thread screen and composing a new message.

Analyzing the code, there are a few specific tweaks possible:

1. updateSubjectHeight() already calls updateElementsHeight() so messageComposerInputHandler() does not need to call updateElementsHeight().

2. cancelEdit() should call updateElementsHeight() only if necessary, i.e., don't call if not in Edit mode

3. updateInputMaxHeight() is usually called before updateElementsHeight() so we should rearrange the code in updateElementsHeight to avoid an extra reflow

Comment 1

[Julien Wajsberg [:julienw]]

Yan, I'm fine with any quick win here, but please also look at bug 949457.

Comment 2

[Yan Or]

Attachment: Pull request

Julien, bug 949457 certainly is a better fix.  With the proposed fix here, the number of reflows in updateElementsHeight() went from between 2-5 (5 when opening and closing composer, 2-3 when bringing up the keyboard) to 1.

Will let you decide if it's worth landing this patch.

Comment 3

[Julien Wajsberg [:julienw]]

If we have only 1 reflow and it's ready, then it's definitely worth it.

reviewing now.

Comment 4

[Julien Wajsberg [:julienw]]

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

good stuff, let's land this

please fix the nits and regressions, and add one or 2 unit tests if possible.

Thanks a lot, I can really feel the difference!

Comment 5

[Yan Or]

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

Fixed regression and made changes based on feedback.

Comment 6

[Julien Wajsberg [:julienw]]

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

r=me, thanks

I think this patch enters in the "perf improvement" use case for landing stuff, so let's land this!

Comment 7

[Julien Wajsberg [:julienw]]

Note that there is a nit, so please land with a green travis.

Also, please file another bug to do an integration tests about reflows.

Comment 8

[Julien Wajsberg [:julienw]]

Please rewrite the commit log before merging, as Rick asked you already.
The commit log should read:

Bug 973407 - [Messages] Reduce reflows when element heights are updated r=julien

Comment 9

[Yan Or]

Fixed jshint issues.  Updated commit log.  Filed ticket for integration tests - Bug 975499.

Comment 10

[Julien Wajsberg [:julienw]]

Then just land it, you got r+ and it's a performance-related patch :) Do you have the rights to do it yourself?

Comment 11

[Yan Or]

No I don't have permission.

Comment 12

[Julien Wajsberg [:julienw]]

merged in master: c95013dfe4c40e4d93bc7c5641936b981cf649a8