934429 - [Messages] Optimizing the app's startup path

Status: RESOLVED FIXED

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

Description

[lecky]

Attachment: Screen-1.png

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; aff-kingsoft-ciba; .NET4.0C; .NET4.0E)

Steps to reproduce:

Description:
    This is a performance issue. 
    When user enter "Messages" at first time, it takes 2.35 seconds.

* Precondition:
  N/A

* Reproduction steps:
  1. Power on the Helix phone.
  2. Select "Messages" in the homescreen.

* Expected result:
  Messages come out in 1 second.

* Actual result:
  Messages come out after 2.35 seconds.

* Analyze:
  When user select "Messages" at first time, B2G show 3 screens from screen-1 thought screen-3. But if user enter "Messages" again, B2G show screen-3 immediately.

* Question:
  Could B2G show screen-3 immediately at the first time when user select "Messages"?

Comment 1

[lecky]

Attachment: Screen-2.png

Add screen-2

Comment 2

[lecky]

Attachment: Screen-3.png

Add screen-3

Comment 3

[Julien Wajsberg [:julienw]]

Can you tell which FxOS version you're using ?

I think we're not showing screen-1 in version 1.3 (I'm not sure for 1.2).
It's difficult to show screen-3 directly because loading the db takes some time.

At the very least we need to profile what happens between screen-2 and screen-3, for sure this would be nice to lower this time.

I'm gonna ask a needinfo on Gabriele to help with profiling this.

Comment 4

[Gabriele Svelto [:gsvelto]]

I'll give it a spin in the profile though I'm a bit surprised we didn't spot this in the automated performance test.

Comment 5

[Julien Wajsberg [:julienw]]

The automated performance test only measures time until screen-2. For screen-3, it would be bug 837666, but it's still pending...

Comment 6

[Julien Wajsberg [:julienw]]

And I'll add that I think we always had this delay :) This is not a regression.

Comment 7

[Gabriele Svelto [:gsvelto]]

This is a profile of the start-up process I've taken with today's build:

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

The total measured time for start-up is ~1.6s, I'm not sure how the 2.35s duration was measured in comment 1 but it may include the time to handle and dispatch the touch event in the Homescreen app which we obviously can't see in a profile.

Now regarding the glitch described in comment 1 I've been able to reproduce it on my phone and I think that it's caused by a spurious reflow we do before having loaded all the styles. I believe this is happening because we're reading one of the layout properties of an element during startup:

http://is.gd/TI22t7

In the profile I can see a synchronous reflow triggered in thread_list_ui.js:18 which corresponds to the ThreadListUI.init() function so I guess that's what causing it.

I'll try to remove / delay that statement and see if it has any effect on the startup time and more importantly if it fixes the glitch.

Comment 8

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Remove a synchronous reflow from the startup path

Here's a WIP patch that removes the synchronous reflow I described in comment 7. Unfortunately this is not saving us much (10s of ms) and the intermediate screen / flash still appears during startup so more work is needed.

I've taken a more granular profile of this and the problem seems to be slightly different than what I anticipated.

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

TL;DR We're lazy-loading stuff that we probably should load right away and this is causing us to do two repaints instead of just one. Here's the whole analysis of the profile:

With this patch applied there's no uninterruptible reflow happening in the startup path yet the screen is repainted twice anyway. The first paint is caused by a refresh driver tick: this should happen only if the DOM has settled down and the main process is waiting for other stuff to happen. The second paint happens after we add stuff to the main view like the "no messages" icon or the threads. Now, if you look at the samples in-between the two paints you can see l10n resources being loaded and the lazy-loader in action, more precisely this is what happens:

- We load the page and all the non-lazy-loaded JS code and styles (samples 2500-2800)

- We fire off "something" that causes I/O to happen and thus causes the main thread to go idle, I believe this is related to l10n and the lazy loader (more about this below)

- The refresh driver gets a chance to schedule a tick (because the main thread is idle, waiting for I/O) and paints the screen (you can see the first interruptible reflow during this first tick, samples 2800-3050). If you look carefully l10n.js begins initialization during that period and it's also triggered by a timer tick which suggests to me that it was lazily loaded or scheduled asynchronously

- Once the screen has been painted some stuff that we were waiting for is coming back and it's apparently an l10n resource (samples 3050-3200, loadResource/xhr.onreadystatechange() l10n.js)

- After that we execute a whole bunch of Javascript (samples 3250-3500)

- Finally the lazy loader comes back and executes most of the scripts (lazy_loader.js perFileCallback() loading all our main files .init() functions, samples 3500-3700)

- Afterwards we get a minimal amount of IndexedDB action (just 12 samples)

- Finally the refresh driver kicks in again, re-processes the CSS styles (because we lazy-loaded some more?) and finally re-paints the whole screen

Comment 9

[lecky]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Can you tell which FxOS version you're using ?

We use HD v1.1 on helix phone.

(In reply to Gabriele Svelto [:gsvelto] from comment #7)
> The total measured time for start-up is ~1.6s, I'm not sure how the 2.35s duration was measured in
> comment 1 but it may include the time to handle and dispatch the touch event
> in the Homescreen app which we obviously can't see in a profile.

We use a high speed camera to capture our operation. We measured from the "messages" was touched to screen-3 was shown. Then we analyze the frames to calculate the time.

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to lecky from comment #9)
> We use a high speed camera to capture our operation. We measured from the
> "messages" was touched to screen-3 was shown. Then we analyze the frames to
> calculate the time.

OK, thanks. That's explain why the number I get is shorter than what you get. You probably have some 100s of ms caused by the touch handling and the homescreen app launching the new app besides what I can see in the profile.

Comment 11

[Julien Wajsberg [:julienw]]

lecky: as far as I understand correctly, no more work will be done on the 1.1HD branch. How is it working in 1.2?

Gabriele: I think we already looked at the reflows caused by the lazy loader of the CSS files. Maybe we should not lazy load the CSS file at all...

Comment 12

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #11)
> Gabriele: I think we already looked at the reflows caused by the lazy loader
> of the CSS files. Maybe we should not lazy load the CSS file at all...

I think that would be a good idea, it might remove the second paint event I've seen in the profile. However I'd like to go even further and lazy-load only things we're sure we don't need immediately: from the looks of the profile we're lazy-loading stuff which is in the critical path. Loading that stuff directly is likely to be faster though that needs a bit of experimentation and profiling.

Comment 13

[lecky]

Hi Gabriele,

I did some more test and got the following info. 
I test for 10 times and found one is 2.366s, and others are all about 1.6s. 

The detail info:(s)
1.6
1.4
1.667
1.767
1.9
2.366
1.5
1.534
1.866
1.6

For the 2.366s, here is the detail info:
0 ~ 0.2s : launch the messages app. The messages icon show highlight and then resume normal.
0.3s : begin to show screen-1
0.533s : complete screen-1
1.666s : show screen-2 immediately (step 4, take 1.133s, about 500ms more than normal)
2.366s : show screen-3 immediately (step 5, take 0.7s, about 600ms more than normal)

For the 1.5s, here is the detail info:
0 ~ 0.2s : launch the messages app. The messages icon show highlight and then resume normal.
0.233s : begin to show screen-1
0.467s : complete screen-1
1.067s : show screen-2 immediately (step 4)
1.5s : show screen-3 immediately (step 5)

We find when the issue happen(2.366s), the step 4 and step 5 takes too much time.

Comment 14

[Julien Wajsberg [:julienw]]

Gabriele, I wonder whether the 1s delay is the Lazyloading delay.

1. we load the app
2. we render the initial markup+css
3. we lazy load the js+css files
4. when the css files are loaded and inserted, we have a new reflow
5. when the lazy load is finished, we call init
6. init calls the Mobile Messaging API and this triggers the indexeddb loading
7. the threads are retrieved and sent through the IPC
8. the SMS app displays the thread one thread at a time using setTimeout

A lot of things are bad in this sequence.
* you already said it, but we probably should not lazy load the css files.
* we could try to load the threads earlier even if we don't display them yet
* we should rework this setTimeout-based thread rendering
* we should probably not lazy load everything, especially if we init only after loading everything... we could try to move some things in a defer script tag instead, and lazy load some things in the objects. For example, the files compose.js and recipients.js could probably be lazy loaded elsewhere. This is a big subject though and maybe another bug could be filed for this specific issue.

Comment 15

[Julien Wajsberg [:julienw]]

Lecky, is 1.5s a good time for you?

Is it possible that the device is busy with other things when the 2.3s case is happening?

Comment 16

[lecky]

Hi Julien,
Yes, 1.5s is a good time.

I test the 10 times continuously without running any other apps. So the device can't be busy with other things.

Comment 17

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Gabriele, I wonder whether the 1s delay is the Lazyloading delay.

Yes, that's the most likely cause.

> 1. we load the app
> 2. we render the initial markup+css
> 3. we lazy load the js+css files

2 starts because we've scheduled the lazy-loading of js+css.

> 4. when the css files are loaded and inserted, we have a new reflow
> 5. when the lazy load is finished, we call init
> 6. init calls the Mobile Messaging API and this triggers the indexeddb
> loading
> 7. the threads are retrieved and sent through the IPC
> 8. the SMS app displays the thread one thread at a time using setTimeout

Yup, 4 actually happens around here in fact which is the second reflow/repaint.

> * you already said it, but we probably should not lazy load the css files.

Absolutely.

> * we could try to load the threads earlier even if we don't display them yet
> * we should rework this setTimeout-based thread rendering

I think that these points are not so important for startup; I would focus on the lazy-loading issues first.

> * we should probably not lazy load everything, especially if we init only
> after loading everything... we could try to move some things in a defer
> script tag instead, and lazy load some things in the objects. For example,
> the files compose.js and recipients.js could probably be lazy loaded
> elsewhere. This is a big subject though and maybe another bug could be filed
> for this specific issue.

Yes, my idea for a quick fix here is to load directly everything and forget about lazy-loading for the time being unless you can find something that we know we won't need immediately and in that case we can lazy-load it only when we actually need it. If you look at how this works in startup.js right now it goes like this:

https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/startup.js

1. load the L10n stuff
2. wait for the localization to finish
3. lazy-load *everything*
4. wait for that to finish and only then start loading the threads we need to draw the main screen

There's nothing lazy about this right now because all this stuff sits right in the critical path. To add insult to injury we're actually loading stuff we know we won't need in the main screen like SMIL.js and WBMP.js.

Unfortunately I don't have time to work on this right now; if you have some spare cycles however you can start from my patch in attachment 827575 [details] [diff] [review] and I can help you out with the profiling when you have something ready.

Comment 18

[Julien Wajsberg [:julienw]]

Lecky: are you waiting 7 seconds between your tries? The device is launching a prefork process in background 5 seconds after an app is launched.

Gabriele: The main problem with putting everything in index.html again, is marketing: the onload event will be delayed, and this will show up in datazilla, and fingers will be pointed at us. Nevertheless we need to try this and see if this feels better.

Comment 19

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> Gabriele: The main problem with putting everything in index.html again, is
> marketing: the onload event will be delayed, and this will show up in
> datazilla, and fingers will be pointed at us. Nevertheless we need to try
> this and see if this feels better.

OK, since this is not a koi+ we have time to do it right. As a first stage I would propose to do the following:

- start lazy-loading the JS code in parallel with L10n and not after it, this might help preventing the first repaint
- don't lazy-load the CSS files to prevent the screen flash and the double reflow

If this doesn't get us enough improvement we can try to go for a real fix which would call for proper lazy-loading of everything we don't need for the main screen.

Comment 20

[Julien Wajsberg [:julienw]]

I'll take it to try to work on it in background.

Please ask if there is no update and you want to work on this.

Comment 21

[Gabriele Svelto [:gsvelto]]

Julien, I've resumed working on this in bug 947234, shall we leave this open or DUP it?

Comment 22

[Julien Wajsberg [:julienw]]

That doesn't have much importance :)

Comment 23

[Gabriele Svelto [:gsvelto]]

OK, taking this, I'll finish my no-spurious reflow/restyle here.

Comment 24

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Remove a synchronous reflow from the startup path

Refreshed patch, next I'll make sure the CSS styles that are needed immediately will not be lazy-loaded.

Comment 25

[Julien Wajsberg [:julienw]]

Just move all CSS files back to index.html, we don't need them lazyloaded and it has negative influence (CSS refresh style, reflow).

Comment 26

[Gabriele Svelto [:gsvelto]]

Nominating for 1.3+ as this blocks a blocker.

Comment 27

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Remove layout/style reflows from the startup path

This patch removes all reflows induced by style changes as well as a synchronous reflow caused by reading an element's |offsetHeight| field in the startup path. The CSS styles are now loaded all at once instead of lazily; this forces them to be computed only once. I've taken the liberty of sorting the CSS files by name since I was touching a few of them already. I've also removed redundant inclusions.

Overall the patch is worth between 30ms and 100ms of startup time depending on certain conditions: due to how the startup process works the style reflow might happen or might not. When it happened it would use 20-50ms, the synchronous reflow would usually take another 10ms and finally lazy loading the CSS files incurred an overhead ~20ms.

Comment 28

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Remove layout/style reflows from the startup path

Comment 29

[Rick Waldron [:rwaldron]]

(In reply to Gabriele Svelto [:gsvelto] from comment #27)
> Created attachment 8355077 [details] [diff] [review]
> [PATCH] Remove layout/style reflows from the startup path
> 
> This patch removes all reflows induced by style changes as well as a
> synchronous reflow caused by reading an element's |offsetHeight| field in
> the startup path. The CSS styles are now loaded all at once instead of
> lazily; this forces them to be computed only once. I've taken the liberty of
> sorting the CSS files by name since I was touching a few of them already.
> I've also removed redundant inclusions.
> 
> Overall the patch is worth between 30ms and 100ms of startup time depending
> on certain conditions: due to how the startup process works the style reflow
> might happen or might not. When it happened it would use 20-50ms, the
> synchronous reflow would usually take another 10ms and finally lazy loading
> the CSS files incurred an overhead ~20ms.

I've marked the changes to the fullHeight property as unnecessary—can you provide the startup difference without that change?

Comment 30

[Gabriele Svelto [:gsvelto]]

(In reply to Rick Waldron [:rwaldron] from comment #29)
> I've marked the changes to the fullHeight property as unnecessary—can you
> provide the startup difference without that change?

The difference is ~10ms which amounts to the sync reflow caused when reading the |offsetHeight| property here:

http://is.gd/mfN36v

Do you have a better approach to remove that particular reflow? With my change the |offsetHeight| property is not touched in the startup path so the element it pertains to needs not be laid out in turn saving us that short reflow.

Comment 31

[Rick Waldron [:rwaldron]]

(In reply to Gabriele Svelto [:gsvelto] from comment #30)
> (In reply to Rick Waldron [:rwaldron] from comment #29)
> > I've marked the changes to the fullHeight property as unnecessary—can you
> > provide the startup difference without that change?
> 
> The difference is ~10ms which amounts to the sync reflow caused when reading
> the |offsetHeight| property here:
> 
> http://is.gd/mfN36v
> 
> Do you have a better approach to remove that particular reflow? With my
> change the |offsetHeight| property is not touched in the startup path so the
> element it pertains to needs not be laid out in turn saving us that short
> reflow.


My remaining concern was here: https://github.com/mozilla-b2g/gaia/pull/14978/files#diff-af7a3fb259f614cb38f498ee6deedb0dL107 but I cannot reproduce a scenario where that condition is met.

Comment 32

[Gabriele Svelto [:gsvelto]]

(In reply to Rick Waldron [:rwaldron] from comment #31)
> My remaining concern was here:
> https://github.com/mozilla-b2g/gaia/pull/14978/files#diff-
> af7a3fb259f614cb38f498ee6deedb0dL107 but I cannot reproduce a scenario where
> that condition is met.

I see... well, in my patch I removed that bit because whenever we call ThreadListUI.fullHeight() we now always read the |ThreadListUI.container.offsetHeight| field so whenever it changes all callers will get the right value anyway. I think it's even more robust this way in which we don't have to set it explicitly on multiple code-paths.

Comment 33

[Rick Waldron [:rwaldron]]

(In reply to Gabriele Svelto [:gsvelto] from comment #32)
> (In reply to Rick Waldron [:rwaldron] from comment #31)
> > My remaining concern was here:
> > https://github.com/mozilla-b2g/gaia/pull/14978/files#diff-
> > af7a3fb259f614cb38f498ee6deedb0dL107 but I cannot reproduce a scenario where
> > that condition is met.
> 
> I see... well, in my patch I removed that bit because whenever we call
> ThreadListUI.fullHeight() we now always read the
> |ThreadListUI.container.offsetHeight| field so whenever it changes all
> callers will get the right value anyway. I think it's even more robust this
> way in which we don't have to set it explicitly on multiple code-paths.

Agreed. Like I said, I couldn't even repro the conditions with an actual device receiving messages :)

Comment 34

[Julien Wajsberg [:julienw]]

(In reply to Gabriele Svelto [:gsvelto] from comment #32)
> (In reply to Rick Waldron [:rwaldron] from comment #31)
> > My remaining concern was here:
> > https://github.com/mozilla-b2g/gaia/pull/14978/files#diff-
> > af7a3fb259f614cb38f498ee6deedb0dL107 but I cannot reproduce a scenario where
> > that condition is met.
> 
> I see... well, in my patch I removed that bit because whenever we call
> ThreadListUI.fullHeight() we now always read the
> |ThreadListUI.container.offsetHeight| field so whenever it changes all
> callers will get the right value anyway. I think it's even more robust this
> way in which we don't have to set it explicitly on multiple code-paths.

Except it's wrong, that's exactly what we don't want to do :) See comments on github.

Comment 35

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] (PTO until January 6th) from comment #34)
> Except it's wrong, that's exactly what we don't want to do :) See comments
> on github.

From Rick's notes in comment 33 it seems that the original situation where the ThreadListUI.fullHeight was set to 0 is not happening anymore. I'll try and reproduce that situation myself to see what happens.

Comment 36

[Julien Wajsberg [:julienw]]

I think Rick did not try with the app forcibly closed.

But even if this is not happening anymore, the original goal of this "fullHeight" var is to know whether the keyboard is displayed, and your current code is not doing this.

Comment 37

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Remove layout/style reflows from the startup path

(In reply to Julien Wajsberg [:julienw] (PTO until January 6th) from comment #36)
> But even if this is not happening anymore, the original goal of this
> "fullHeight" var is to know whether the keyboard is displayed, and your
> current code is not doing this.

It took me a while to realize that we were relying on the size of the container taken when the keyboard was not visible to check later if it was visible; what an ugly hack :-P

In the revised patch I'm proposing another hack which is just as ugly but at least is self-contained so I can completely get rid of |fullHeight|.

Comment 38

[Rick Waldron [:rwaldron]]

Latest tests:

1. the FxOS phone screen off, no apps open, rec'd message, tap icon: **path entered**, value: 0
2. the FxOS phone screen off, no apps open, rec'd message, tap notification: **path entered**, value: 0
3. the FxOS phone screen off, but Messages app open and "minimized", rec'd message, tap notification: **path not entered**, value: 410
4. the FxOS phone screen on, but Messages app closed, rec'd message, tap notification: **path not entered**, value: 410
5. the FxOS phone screen on, but Messages app open and "minimized", rec'd message, tap icon: **path not entered**, value: 410

So it looks like this path is definitely still necessary, so it needs to be restored in the patch.

Comment 39

[Gabriele Svelto [:gsvelto]]

(In reply to Rick Waldron [:rwaldron] from comment #38)
> So it looks like this path is definitely still necessary, so it needs to be
> restored in the patch.

Check the updated patch, I've modified the keyboard-detection code to use window.inner/outerHeight instead of relying on that value so we don't need it anymore. It's still a hack but it doesn't depend anymore on checking the container height at the right time which makes the whole thing self-contained and more robust.

Comment 40

[Julien Wajsberg [:julienw]]

Comment on attachment 8355544 [details] [diff] [review]
[PATCH v2] Remove layout/style reflows from the startup path

Review of attachment 8355544 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't tried it on the device yet but your change of fullHeight is actually a good idea.

Some preliminary comments, I'll test it monday.

::: apps/sms/index.html
@@ +21,2 @@
>      <link rel="stylesheet" type="text/css" href="style/report_view.css">
> +    <link rel="stylesheet" type="text/css" href="style/root.css">

IIRC "root.css" should be the first one (and imo we should try to get rid of it as most of its CSS is overriden in the other CSS files)

@@ +21,3 @@
>      <link rel="stylesheet" type="text/css" href="style/report_view.css">
> +    <link rel="stylesheet" type="text/css" href="style/root.css">
> +    <link rel="stylesheet" type="text/css" href="style/sms.css">

I'd use this order:

* building blocks

* root
* common
* sms

* notification
* report_view
* attachment

Basically, from the most general to the most specific. And crossing fingers that we don't regress.

::: apps/sms/js/thread_ui.js
@@ +837,5 @@
>      }
>    },
>  
>    isKeyboardDisplayed: function thui_isKeyboardDisplayed() {
> +    // XXX: Detect if the keyboard is visible, its minimal height is 150px

can you add that the reduced attention screen is smaller than 150px ?

@@ +838,5 @@
>    },
>  
>    isKeyboardDisplayed: function thui_isKeyboardDisplayed() {
> +    // XXX: Detect if the keyboard is visible, its minimal height is 150px
> +    return ((window.outerHeight - window.innerHeight) > 150);

I'd use `window.screen.height` instead of `window.outerHeight`

Comment 41

[Rick Waldron [:rwaldron]]

Gabriele, can you make PRs on github and link them as an attachment here? Thanks!

Comment 42

[Gabriele Svelto [:gsvelto]]

(In reply to Rick Waldron [:rwaldron] from comment #41)
> Gabriele, can you make PRs on github and link them as an attachment here?
> Thanks!

I've updated the PR in attachment 8355078 [details] [review], I always do when I attach a new patch to the bug. I still do the patches-on-the-bug thing because I've started on Gecko and thus I'm more inclined to use that workflow :)

Comment 43

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

triage: 1.3+ , blocks a blocker

Comment 44

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Remove layout/style reflows from the startup path

Addressed the comments in comment 40, I'll also refresh the PR right away.

Comment 45

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8356119 [details] [diff] [review]
[PATCH v3] Remove layout/style reflows from the startup path

Double-review for this one because we're touching the startup path.

Comment 46

[Rick Waldron [:rwaldron]]

Comment on attachment 8356119 [details] [diff] [review]
[PATCH v3] Remove layout/style reflows from the startup path

Review of attachment 8356119 [details] [diff] [review]:
-----------------------------------------------------------------

No tests were harmed by this change :)

The issues I've outlined here: https://github.com/mozilla-b2g/gaia/pull/14978/files#r8637613 are all addressed by this patch, thank you!

Comment 47

[Julien Wajsberg [:julienw]]

Comment on attachment 8356119 [details] [diff] [review]
[PATCH v3] Remove layout/style reflows from the startup path

Review of attachment 8356119 [details] [diff] [review]:
-----------------------------------------------------------------

apart from the following change, this looks fine.

I'm so glad to get rid of the fullHeight var.

r=me with adding back the style link in the iframe template and slightly changing the comment. Please check that attachments handling works fine in the composer after this. Also, please merge with a green Travis.

Thanks!

::: apps/sms/index.html
@@ -405,5 @@
>          <base href="${baseURL}">
>          <style type="text/css">
>            html, body { font-size: 10px; }
>          </style>
> -        <link rel="stylesheet" type="text/css" href="style/attachment.css">

you need to keep this because this is loaded in an iframe and this breaks the display of an attachment.

::: apps/sms/js/thread_ui.js
@@ +839,5 @@
>  
>    isKeyboardDisplayed: function thui_isKeyboardDisplayed() {
> +    /* XXX: Detect if the keyboard is visible, the keyboard minimal height is
> +     * 150px; when in reduced attention screen mode the window height will also
> +     * be less than this. */

nit: less than `window.screen.height` but more than `window.screen.height - 150` and therefore this will appropriately return false.

Comment 48

[Julien Wajsberg [:julienw]]

BTW, did you measure the startup gain with this patch? I'm curious :)

Comment 49

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v4] Remove layout/style reflows from the startup path r=rwaldron,julienw

Here's an updated patch with all comments addressed. I checked that the attachment panel still works and it seems fine.

I've done some profiling and the benefit seems to be up to 100ms. There's some variability in the results mostly because the end of the startup procedure ends in the IndexedDB calls which introduce some unpredictable latencies. Manually measuring the time it takes to get there on a profile yields figures between 50-100ms.

The travis run is here:

https://travis-ci.org/mozilla-b2g/gaia/builds/16579729

I'll wait for it to turn green before landing.

Comment 50

[Gabriele Svelto [:gsvelto]]

Pushed to gaia/master 98dc7ad6621abfb76b7870356c492a69c210b6a7

Uplifting this to v1.3 requires some changes so I'll post an updated patch.

Comment 51

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH gaia/v1.3] Remove layout/style reflows from the startup path

Updated patch for gaia/v1.3, this requires some testing before I land it as some changes were required to make the patch apply cleanly.

Comment 52

[Gabriele Svelto [:gsvelto]]

I've tested various scenarios on 1.3 and everything looks fine, pushed attachment 8357077 [details] [diff] [review] to gaia/v1.3 b56fc82c75f15b94598cee37c844186a7b9c147f.