836027 - [Cost Control] Cost control app loads all the HTML on startup

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Cost Control, defect)

Description

[Michał Budzyński (:michalbe)]

What is bad for performance - it takes the app more than 3 seconds to start. We should use same pattern as in Settings app (lazy-load the html and needed css & js files)

Comment 1

[David Scravaglieri [:scravag]]

We need a patch to estimate the risk on this before deciding if it's tef+

Comment 2

[Michał Budzyński (:michalbe)]

Attachment: patch

This patch improves startup time of CostControl application from ~2270ms to ~1390ms (880ms startup boost). 
It divides all the HTML content into separate <section> tags with content wrapper inside a comment. When we need to load the given view loadPanel method of ViewManager strips tag in the right section (if it has 'hidden' attribute - on the first run the argument is removed so running the same view again will just display it) and put the content again into the parent tag. After rendering HTML loadPanel takes all the script & style files and append them into the head element. If the element already exists (we check the 'id' of it, equal to src/link value) we skip this step, because different modules could use the same files. 
Because of strange modules separation (everything was loaded at the beginning, even if some of the views would never be displayed) this patch introduces also basic changes to the logic of most of the modules - now they are initialized when needed, not on startup.

Comment 3

[Michał Budzyński (:michalbe)]

I tried to test if I didn't break any possible case, but :SALVA please test it again since you know the app better. I used FreeMobile patch (https://github.com/lissyx/gaia/commit/5cfa638f3e4d1b85da1a5aa7cdce07aa33b930c4) to test telephony view.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Not going to block but please land and get more feedback testing this on trunk, nominate for uplift if this can be shown to be a low risk landing.

Comment 5

[Salvador de la Puente González [:salva]]

(In reply to Michal Budzynski (:michalbe) from comment #2)
> Because of strange modules separation (everything was loaded at the
> beginning, even if some of the views would never be displayed) this patch
> introduces also basic changes to the logic of most of the modules - now they
> are initialized when needed, not on startup.

The HTML is not divided in any way but the behaviors bound to the HTML are only started when required. So certain HTML sections could never be shown but this does not imply they are initialized.

Comment 6

[Salvador de la Puente González [:salva]]

Attachment: Links in telephony view and telephony view not usable

STR

With a operator-valid SIM

1 - Open Cost Control from desktop*
2 - Go to settings
3 - Configure as postpaid and close settings*
4 - Long press to home
5 - Close Cost Control application
6 - Re-open Cost Control from desktop

*The issue can reproduce at this point

Expected:
An operative telephony view

Actual:
Inoperative telephony view with no tabs to select functionallity. See attached.

Comment 7

[Salvador de la Puente González [:salva]]

Attachment: Weird square when updating and lack of icons

STR:

Whith an operator valid SIM

1 - Open Cost Control from desktop*
2 - Go to settings
3 - Configure as postpaid and close settings*
4 - Long press to home button
5 - Close Cost Control
6 - Re-open Cost Control from desktop
7 - Go to settings
8 - Configure as prepaid and close settings

*The issue could reproduce here

Expected:
The balance view with icons in the buttons

Actual:
No icons in buttons and a strange square after pressing update. See attachment.

Comment 8

[Michał Budzyński (:michalbe)]

by 'desktop' you mean 'homescreen' ? Or desktop build of B2G?

Comment 9

[Salvador de la Puente González [:salva]]

Comment on attachment 710221 [details]
patch

Sorry Michal but I can not grant you the review+. This patch has several problems:

1st - It introduces regressions as those pointed in the attachments.
2nd - It introduce a lot of non related changes with lint errors
3rd - And most important, the patch does not meet architectural constrains and introduces a lot of hard to follow dependencies.

Although the idea is good, Cost Control is a very large application and a good separation of responsibilities is preferred rather than quick optimization.

We are planning to move into a strategy pattern to improve modularity and organization. Maybe, at that moment we could integrate some ideas you are providing here.

Comment 10

[Salvador de la Puente González [:salva]]

(In reply to Michal Budzynski (:michalbe) from comment #8)
> by 'desktop' you mean 'homescreen' ? Or desktop build of B2G?

Yes, homescreen, I did not know how to say... I wanted to say "not from the widget"!

Comment 11

[Michał Budzyński (:michalbe)]

Salva - so what do you propose? Should I fix issues you wrote about and then you'll land it, or do you have any other ideas on improving performance? Regressions & linting errors are quite easy to repair, I'm not sure if rebuilding dependencies should be part of this PR or should we land it and work on those dependencies as a separate bug.

Comment 12

[Salvador de la Puente González [:salva]]

I would like to explore some kind of explicit separation instead of comments in mark-up. A true division in modules with a couple of files with JavaScript and CSS dependencies. Maybe refactoring JS to be included as AMD modules. What do you think?

Comment 13

[Salvador de la Puente González [:salva]]

In order to address the performance issues by lazy loading the HTML and best suit the changes involved, I'm going to split this task starting from here and continuing with some few follow-ups.

Taking from Michal.

Comment 14

[Michał Budzyński (:michalbe)]

Salva, but I wrote you that I'm working on it all the time, this week as well. I'll send final version later today. 

Unfortunately - implementing your solutions on put additional code on posto-load callbacks made a lot of problems - we need to fire callbacks when all the js files will execute, together with additional methods in files loaded at startup this approach costs us around 300ms.

Comment 15

[Salvador de la Puente González [:salva]]

As you did not answer to the comment 12 I was not aware about what did you plan to do after the review.

Have you seen after JavaScript optimization the open time become about 1.5 seconds? I don't know if we gain a lot by modifying the architecture of Cost Control.

Comment 16

[Michał Budzyński (:michalbe)]

I've seen it. With my patch I'm around 940ms now.

Comment 17

[Salvador de la Puente González [:salva]]

Attachment: Tabs look bad when accessing by touching the data module in the widget

STR:

1 - Pull down the utility tray
2 - Tap on the data usage module

Expected:
Data Usage tab is shown

Current:
Data Usage tab is shown but the tab area is not properly rendered

Comment 18

[Salvador de la Puente González [:salva]]

(In reply to Michal Budzynski (:michalbe) from comment #16)
> I've seen it. With my patch I'm around 940ms now.

How are you measuring the time? I can not reproduce your times. I'm using the ime to load` debug feature of the pohe getting times around 1s when prepaid mode and around 2s in postpaid mode.

Furthermore note we are trying to avoid some load time but we need to load a default view (balance / telephony or data usage) as soon as the application opens. Then, actually we are not improving as much as we expected. The best improvement comes from lazy loading settings so we should focus on this.

Comment 19

[Salvador de la Puente González [:salva]]

Comment on attachment 710831 [details]
Weird square when updating and lack of icons

This is still happening.

Comment 20

[Salvador de la Puente González [:salva]]

Comment on attachment 710831 [details]
Weird square when updating and lack of icons

We have recovered icons but not the loading animation. The square is still there.

Comment 21

[Salvador de la Puente González [:salva]]

Comment on attachment 710221 [details]
patch

Summary:

- The weird square is still shown instead of updating icon
- The UI does not load in a proper way when accessing data usage tab by tapping on the right section of the widget
- The new patch introduces inter-module dependencies we should avoid. I'm trying to solve this with...
- I'm not able to reproduce times reported. Undoubtedly they are shorter than original ones but not as shorter so I'm more reluctant to allow new dependencies if we don't get more in return. I simply think, new times do not worth the additional complexity.

Comment 22

[Salvador de la Puente González [:salva]]

Attachment: Expected NO SIM dialog

This is the expected appearance of the NO SIM dialog. Compare with the attachment about the actual appearance.

Comment 23

[Salvador de la Puente González [:salva]]

Attachment: After patch NO SIM dialog

This is the appearance after applying the patch.

Comment 24

[Salvador de la Puente González [:salva]]

Attachment: Settings are being build while apearing causing a weird effect to the user

STR:

With the patch applied:

1 - Ensure Cost Control is closed
2 - Open Cost Control
3 - Show settings

Expected:
Settings smoothly appears.

Actual:
Settings appears while is being build causing a strange effect of crash. (Only the first time).

Comment 25

[Michał Budzyński (:michalbe)]

Attachment: In case we will preload the CSS of the settings before showing the view, regression is smaller.

Comment 26

[Michał Budzyński (:michalbe)]

As I expected, issue in Comment #23 & Comment #22 was just a simple CSS addition: https://github.com/michalbe/gaia/commit/f471c55b7edc89fb45f12d5ac238b62497042553

Comment 27

[Michał Budzyński (:michalbe)]

About the settings view issue we talk on IRC - what we want to achieve is to display something to the user after taping the settings button, but because of the size of js & css files it looks a little bit weird on the first run. What we can do:

* We can load CSS of the settings view at startup (I did it in my yesterday's commit - https://github.com/michalbe/gaia/commit/f2f2cd31983b52c0030220b78badad701c1674d1). It looks better, but we don't have sliding animation on the first time we enter settings. It's because of 'hidden' html attribute, it changes the 'display' css property to 'none', so there is no transition between the bottom and final positions. We can change it by adding:
.view[hidden] {
  display: block;
}
in app.css, then the elements are still invisible, but we have the transition when needed. Unfortunately - hidden attribute is removed from the settings-tab at the end of loadPanel, and because of the size of JS & CSS we load in this panel, 0.3s is not enough to see the animation. You can simply check it by adding the part I mentioned already to app.css, and setting the transition to 2 or 3 second - then everything is displayed corectly, with slow sliding of the settings view. 

* We can hide the content of the settings-tab inside another container with hidden attribute and show it when scripts will load, as we discussed today. unfortunately, we still have big files to load, even without showing the content (we just show nothing instead of weird styled settings options), so it doesn't solve our problem.

* I think it's possible also to load the settings file after loading the main screen, when everything is loaded. Then, if user will enter settings tab, everything will be already loaded (but HTML - it will be parsed on the first time we activate settings).

* If sliding animation is more important than usable settings tab, we can skip the settings file on loading the panel for the first time, and load it in changeViewTo callback. In this case user will see nice animation of sliding new view, but it will be useless for the time we need to load big files inside.

* We can also load everything before showing the view to the user - but there will be no reaction on clicking 'settings' button for the time we need to load everything. I don't think it's a good approach, because application build in this way seems laggy.

Comment 28

[Michał Budzyński (:michalbe)]

Also - I don't think if sliding is really important in here. When we hit 'Done' in settings the view is sliding down, but the screen behind is blank - main view is shown after the animation stops what seems weird. Also, according to https://bugzilla.mozilla.org/show_bug.cgi?id=834712#c6 Josh changed the behavior of system dialogs from sliding up to fading in. I think we should follow the same pattern in Cost Control app for consistency, not only in views, also in our custom dialogs (imho we should use system dialogs instead, but it's not related to this bug). Josh?

Comment 29

[Salvador de la Puente González [:salva]]

(In reply to Michal Budzynski (:michalbe) from comment #28)
> Also - I don't think if sliding is really important in here. When we hit
> 'Done' in settings the view is sliding down, but the screen behind is blank

This is the bug #811844

You can see, is almost done. Just waiting for review.

> - main view is shown after the animation stops what seems weird. Also,
> according to https://bugzilla.mozilla.org/show_bug.cgi?id=834712#c6 Josh
> changed the behavior of system dialogs from sliding up to fading in. I think
> we should follow the same pattern in Cost Control app for consistency, not
> only in views, also in our custom dialogs (imho we should use system dialogs
> instead, but it's not related to this bug).

We can not use system dialogs for all our views. In warnings, we can and you are right, we are not implementing the correct animation here but this is not our concern in this bug. Anyway, just to clarify, we can not use system dialogs when displaying choosing dialogs (<select>) because they lack of a cancel button and we need them according to the wireframes.

I'm going to add Marco to the loop since he is in charge of user's experience for the Cost Control application.

Comment 30

[Salvador de la Puente González [:salva]]

(In reply to Michal Budzynski (:michalbe) from comment #27)
> About the settings view issue we talk on IRC - what we want to achieve is to
> display something to the user after taping the settings button, but because
> of the size of js & css files it looks a little bit weird on the first run.
> What we can do:

I was thinking about this. Do you think we cauld load in background in **another** iframe? Bypassing global object to point to the parent window. I'm going to try this approach.

> * We can load CSS of the settings view at startup (I did it in my
> yesterday's commit -
> https://github.com/michalbe/gaia/commit/
> f2f2cd31983b52c0030220b78badad701c1674d1). It looks better, but we don't
> have sliding animation on the first time we enter settings. It's because of
> 'hidden' html attribute, it changes the 'display' css property to 'none', so
> there is no transition between the bottom and final positions. We can change
> it by adding:
> .view[hidden] {
>   display: block;
> }
> in app.css, then the elements are still invisible, but we have the
> transition when needed. Unfortunately - hidden attribute is removed from the
> settings-tab at the end of loadPanel, and because of the size of JS & CSS we
> load in this panel, 0.3s is not enough to see the animation.

Why not removing the hidden attribute when calling `changeViewTo()`. We can use data-* instead of hidden to prevent CSS side effects.

> * We can hide the content of the settings-tab inside another container with
> hidden attribute and show it when scripts will load, as we discussed today.
> unfortunately, we still have big files to load, even without showing the
> content (we just show nothing instead of weird styled settings options), so
> it doesn't solve our problem.

And we need to hanlde the settings case apart, we add messy markup and it is not a very clear solution. Let's try other approaches...

> 
> * I think it's possible also to load the settings file after loading the
> main screen, when everything is loaded. Then, if user will enter settings
> tab, everything will be already loaded (but HTML - it will be parsed on the
> first time we activate settings).

I though so but it is not possible. The module autosettings.js reads the markup and binds it to Configuration. But it needs to read the markup so we can not `initialize()` settings before having the markup ready.

> * If sliding animation is more important than usable settings tab

Be careful here, I'm not saying sliding is more important the usable settings tab. I'm saying this is a regression and we should land patches **without** regressions.

> we can skip the settings file on loading the panel for the first time, and load it
> in changeViewTo callback. In this case user will see nice animation of
> sliding new view, but it will be useless for the time we need to load big
> files inside.

This leads me to an idea! Why don't include a `data-lazy-loadtime=<milliseconds>` attribute on panels, and show them after <milliseconds> only the firs time. The absence of this attribute is like saying data-lazy-loadtime=<milliseconds>. Ok, this delays the apparition of the settings for first time and it is very device dependant but while loading HTML, JS and CSS we could change the icon of settings with a spinner (in a UX follow up bug) indicating we are effectively loading the settings. It's just the first time.

> * We can also load everything before showing the view to the user - but
> there will be no reaction on clicking 'settings' button for the time we need
> to load everything. I don't think it's a good approach, because application
> build in this way seems laggy.

This is similar to my former proposal an we can use the spinner trick again but the previous solution seems less laggy.

Comment 31

[Salvador de la Puente González [:salva]]

For UX guys:

The problem here is now we are loading resources (CSS, JavaScript) when they are really needed. Settings is a very big module with big resources. The current patch do this:

See timeline:

|- User press settings                    |- The settings view has appeared
|                                         |
X ------ the view appears after 0.3s ---- X
X ----x---------------x-------------------X
      |               |
      |- Load HTML    |- Load JS that configures and finish to build settings

So, while appearing transition, the view is configuring itself and the user is able to see this configuration process which lead to situations as those commented in c24 and c25

Before the patch we don't have this issue as we load **all** the resources before the application was even usable (leading us to a long start time which we are trying to solve here).

Now after the patch we have this unexpected behavior.

Hope it helps.

Comment 32

[Josh Carpenter [:jcarpenter]]

(In reply to Michal Budzynski (:michalbe) from comment #28)
> Also - I don't think if sliding is really important in here. When we hit
> 'Done' in settings the view is sliding down, but the screen behind is blank
> - main view is shown after the animation stops what seems weird. Also,
> according to https://bugzilla.mozilla.org/show_bug.cgi?id=834712#c6 Josh
> changed the behavior of system dialogs from sliding up to fading in. I think
> we should follow the same pattern in Cost Control app for consistency, not
> only in views, also in our custom dialogs (imho we should use system dialogs
> instead, but it's not related to this bug). Josh?

Consistency is good, where appropriate :) Per Salva, I defer to Marco (as the owner of Cost Control) on the right approach here.

Comment 33

[Salvador de la Puente González [:salva]]

Here is a video of how settings view behaves before applying the patch:
http://youtu.be/WM0NVXPHtmI

Now, here is a video after applying the patch:
http://youtu.be/XTU78t6KHac

Hope it helps.

Comment 34

[Salvador de la Puente González [:salva]]

I was testing the loading time with the *current patch* [1] (I don't get b2gperf working so I tested manually) and with a *modified* one that includes loading settings in a different in background using an iframe [2]:

# Current patch (10 times after a reboot):
Loading settings markup and resources on demand.

Run 1: 1092
Run 2: 1065
Run 3: 1039
Run 4: 1170
Run 5: 1036
Run 6: 1059
Run 7: 1153
Run 8: 1045
Run 9: 1110
Run 10: 1172
Average: 1094ms

# Modified patch (10 times after a reboot):
Loading settings markup and resources in parallel using an iframe.

Run 1: 1225
Run 2: 1116
Run 3: 1108
Run 4: 1114
Run 5: 1174
Run 6: 1150
Run 7: 1114
Run 8: 1264
Run 9: 1251
Run 10: 1181
Average: 1169

Second approach is a 6% (75ms) slower than the first one but avoid the settings regression.

Michal, can you repeat the testing, please?

[1] https://github.com/michalbe/gaia/commit/f471c55b7edc89fb45f12d5ac238b62497042553
[2] https://github.com/lodr/gaia/commit/9e61d8dcfccb2de87d732449d2d9ffae1a9fb3c8

Comment 35

[Michał Budzyński (:michalbe)]

# Current:

1: 1050
2: 1054
3: 1040
4: 1033
5: 1059
6: 1051
7: 1031
8: 1091
9: 1059
10: 1078
AVG: 1055

# Modified:
1: 1125,
2: 1112,
3: 1092,
4: 1106,
5: 1103,
6: 1106,
7: 1104,
8: 1117,
9: 1120,
10:1168
AVG: 1115

so it's a little bit more than 5%.

Somehow the 'Done' button in settings is "Do.." after applying your patch.

Comment 36

[Salvador de la Puente González [:salva]]

Ok, I wanted to check times were similar. The "Do..." thing is probably caused by an unexpected overflow. What are your thoughts about the solution?

Comment 37

[marcoc]

(In reply to Salvador de la Puente González [:salva] from comment #29)
> (In reply to Michal Budzynski (:michalbe) from comment #28)
> > Also - I don't think if sliding is really important in here. When we hit
> > 'Done' in settings the view is sliding down, but the screen behind is blank
> 
> This is the bug #811844
> 
> You can see, is almost done. Just waiting for review.
> 
> > - main view is shown after the animation stops what seems weird. Also,
> > according to https://bugzilla.mozilla.org/show_bug.cgi?id=834712#c6 Josh
> > changed the behavior of system dialogs from sliding up to fading in. I think
> > we should follow the same pattern in Cost Control app for consistency, not
> > only in views, also in our custom dialogs (imho we should use system dialogs
> > instead, but it's not related to this bug).
> 
> We can not use system dialogs for all our views. In warnings, we can and you
> are right, we are not implementing the correct animation here but this is
> not our concern in this bug. Anyway, just to clarify, we can not use system
> dialogs when displaying choosing dialogs (<select>) because they lack of a
> cancel button and we need them according to the wireframes.
> 
> I'm going to add Marco to the loop since he is in charge of user's
> experience for the Cost Control application.

I think the transitions should be consistent with the rest of system as Josh says. 
But i'm not sure if that is the issue anymore, it seems like there has been progress on this recently.

Comment 38

[Michał Budzyński (:michalbe)]

I resolved the merge conflicts we had with the new updates that landed in master, and changed 'async' to 'defer' in all the scripts we load. With all those changes we still have around 1300ms on startup, what is not so good again (but still better than 3000+ we had at the beginning), we need to figure out how to go below 1000. After improving the settings loading close buttons in Topup dialogs are not working anymore - we don't have 'this._currentView', so closeCurrentView() returns in the first 'if'. Any thoughts on that? Also - 'Done' button in settings is truncated now, we change nothing in there but somehow, loading settings in an iframe, change the label of the button to 'Do...'.

Comment 39

[Salvador de la Puente González [:salva]]

(In reply to Michal Budzynski (:michalbe) from comment #38)
> I resolved the merge conflicts we had with the new updates that landed in
> master

Great! Thank you.

> and changed 'async' to 'defer' in all the scripts we load.

Why is better to do `defer` loads instead of `async` ones? I followed this guide to use `async` or `defer` when it fits better.
http://peter.sh/experiments/asynchronous-and-deferred-javascript-execution-explained/

> With all those changes we still have around 1300ms on startup, what is not so good
> again (but still better than 3000+ we had at the beginning), we need to
> figure out how to go below 1000.

One second seems as a reasonable target time, maybe when replacing custom selects by system selects we could reach those numbers. Let's go step by step.

> After improving the settings loading close
> buttons in Topup dialogs are not working anymore - we don't have
> 'this._currentView', so closeCurrentView() returns in the first 'if'. Any
> thoughts on that? Also - 'Done' button in settings is truncated now, we
> change nothing in there but somehow, loading settings in an iframe, change
> the label of the button to 'Do...'.

I solved the "Do..." regression and the close top up dialog. Thanks for the warning here. I'll make you a PR.

Comment 40

[Michał Budzyński (:michalbe)]

Great, thanks. 
I think that we can also call loadSettings() in setupApp() after a timeout - in this case it will not be loaded on startup, but it will still be fast enough for user who goes to settings immediately after the app loads. I'll test this approach today.

Comment 41

[Salvador de la Puente González [:salva]]

Perfect, I was trying to do something like that with not very impressive results but feel free to experiment and try all you want. x)

Comment 42

[Michał Budzyński (:michalbe)]

So I've tested different ways of loading settings after timeouts and unfortunately you were right - we don't gain much with this approach. I'll try to figure out something else tomorrow.

Comment 43

[Michał Budzyński (:michalbe)]

Salva, if you don't have any more comments to this PR, we should land it now - every new features makes it harder to resolve merge conflicts. We have around 1300ms on startup, what is still better than 3s at the beginning, so we can think about other ways to speedup the app after this patch will be merged. Also - I'm on PTO next week.

Comment 44

[Salvador de la Puente González [:salva]]

Ok, I have no more commentaries. I'm going to review the code again and ask for another point of view (Vivien or Francisco) since I have code there and it's not very fair to review myself.

Comment 45

[Salvador de la Puente González [:salva]]

Please, fix the comment on GitHub and I will grant you the r+
Anyway asking for Vivien's review in order to have another point of view.

Comment 46

[Salvador de la Puente González [:salva]]

Comment on attachment 710221 [details]
patch

There are additions I made so I'm asking for another point of view.

Comment 47

[Michał Budzyński (:michalbe)]

Thanks Salva, issue fixed.

Comment 48

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The patch needs to be rebased before landing though. Salva can you do it? (Michal is on vacation).

Comment 49

[Salvador de la Puente González [:salva]]

Attachment: Lazy HTML loading for Cost Control app r=vingtetun

Please, review the patch again because the rebase was not trivial.
Thanks.

Comment 50

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 717896 [details]
Lazy HTML loading for Cost Control app r=vingtetun

Sounds good. Most of changes are removals of custom dialogs afaict which is for good!

Comment 51

[Salvador de la Puente González [:salva]]

Master: 90696cc82cfc27426ad268e4d6ee7c2de5bcf421

Comment 52

[Salvador de la Puente González [:salva]]

Performance bug with several changes, please consider it for v1.0.1

Comment 53

[Lukas Blakk [:lsblakk] use ?needinfo]

Given the timeframe to v1.0.1 and the amount of change in this patch (unknown risk) triage feels this is best pushed to v1.1 where it can have some bake time.

Comment 54

[Salvador de la Puente González [:salva]]

Please, reconsider it. This is a performance bug. It implies partial code restructuring and I'm going to continue solving bugs on master where changes have been applied. If a future change must land on v1.0.1 it will be applied over these changes and to land them separated from this bug could be hard.

Although the risk is moderate, Michal and I were testing it and avoiding regressions during the long review process. It has been reviewed by Vivien and me and we don't detect any regression but an important performance improvement.

We could ask for QA testing before landing in v1.0.1 if you want.

Renominating as tef? to be retriaged.

Comment 55

[Lukas Blakk [:lsblakk] use ?needinfo]

We have reconsidered and still believe that since this is not a hardware or customization support issue, nor does it fix critical functionality or certification blockers we have to hold off on taking this until v1.1.  There will not be future changes landed to v1.0.1 to interact poorly with the lack of this patch (same with some email perf). We'll have to work with what got in on time.

Comment 56

[Salvador de la Puente González [:salva]]

Comment on attachment 717896 [details]
Lazy HTML loading for Cost Control app r=vingtetun

[Approval Request Comment]
Bug caused by (feature/regressing bug #): several
User impact if declined: medium
Testing completed: yes
Risk to taking this patch (and alternatives if risky): moderate
String or UUID changes made by this patch:

Comment 57

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

Uplifted commit 90696cc82cfc27426ad268e4d6ee7c2de5bcf421 as:
v1-train: b50f89c4accce14a3137f667f0e77e91544b5a03

Comment 58

[Julien Wajsberg [:julienw]]

asking tef? for this.

See https://datazilla.mozilla.org/b2g/?branch=v1-train&range=30&test=cold_load_time&app_list=usage&app=usage&gaia_rev=99a78752f3c99518&gecko_rev=b614a50859684fa6 to see the 900ms improvement in launch time.

Comment 59

[Daniel Coloma:dcoloma]

We did not do this uplift 1 month ago, doing this right now sounds really scary, Salva, what is the risk?

mvines, what is your feeling about the performance win?

Comment 60

[Michael Vines [:m1] [:evilmachines]]

IMO, too scary unless the fix is demanded from downstream folks and they're ok with the risk.  This isn't exactly  a core app so the extra delay is clunky but not fatal for v1.

Comment 61

[Salvador de la Puente González [:salva]]

The risk of the change-set is the same but I have no idea about the risk of patching v1.0.1.

Let's try. Let me cherry pick the commit in v1-train and try to merge into v1.0.1
The I could estimate the risk of patching.

Comment 62

[Salvador de la Puente González [:salva]]

Ok, I tried the cherry pick. This bug has a dependency with bug 841439. Although the dependency can be cherry picked cleanly, this one does not merge.

Merging risk is moderate-high. It needs some manual fixing. The behavior seems correct but deep testing is required.

Should we go further?

Comment 63

[Daniel Coloma:dcoloma]

tef- based on comment 60 and comment 62