841251 - Implement a shared lazy loader

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Kevin Grandon :kgrandon]

I've seen at least six different lazy loader implementations within apps - it's time to consolidate these.

Just something simple that allows us to lazy load scripts/stylesheets and that safeguards against double script insertion should be fine.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

As this touches several places, I'm going to be asking a few people for reviews of this. Thanks!

Comment 2

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

Comment on attachment 713737 [details]
Github pull request pointer

Thanks for the simple loader. I don't think we need something more complicated at the moment.

Comment 3

[Jose Manuel Cantera]

Attachment: Alternative Design Based on DOMRequest

Alternative Design Based on DOMRequest and other improvements see GH PR description

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 713737 [details]
Github pull request pointer

Just looking for a few peers to review this one. Thanks!

Comment 5

[Rick Waldron [:rwaldron]]

The implementation commits need to be decoupled from the integration commits.

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 713737 [details]
Github pull request pointer

Have a review from vivien on this. Please continue to take a look though and see if you find any red flags.

Comment 7

[Kevin Grandon :kgrandon]

I landed this so I can move onto other things: https://github.com/mozilla-b2g/gaia/commit/ab1d2c275643769a1854c49bfe5a3db2aa972542. I'm going to mark this as fixed, and we can always iterate on the loader from here on out.

@Jose - More than happy to work with getting some of your features into this one or refactoring it. I'm also open to landing a different lazy loader and seeing which one is used more, (if we deem necessary). Honestly though, I'm happy with my implementation as it does everything I think developers need to do and is ~70 lines vs ~250. Also:

- Per-file callbacks is trivial as it's easy to just call the loader multiple times.
- I don't think onerror support is needed as we just load local scripts (though we could easily add support).

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 713737 [details]
Github pull request pointer

This would be nice to have in v1 because there are several other apps which can make better use of lazy loading. If we want to have those performance fixes in v1 - then we should probably have this framework support.

User impact if declined: None - this is only nice to have for additional perf work.

Testing completed: Lots of manual testing, unit tests are in place. Lots of eeyes on github.

Risk to taking this patch (and alternatives if risky): Assuming there are no merge conflicts - there should be none. This does touch dialer/calendar/email apps though - all of which have passed manual testing.

Comment 9

[Jim Porter (:squib)]

(In reply to Kevin Grandon from comment #7)
> Honestly though, I'm happy with my implementation as it does everything I think developers
> need to do and is ~70 lines vs ~250.

I agree. Shorter is better in this case. We should make sure the lazy loader stays as short as possible so as to minimize the time it takes to load on startup.

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Kevin Grandon from comment #8)
> Risk to taking this patch (and alternatives if risky): Assuming there are no
> merge conflicts - there should be none. This does touch
> dialer/calendar/email apps though - all of which have passed manual testing.

Has this patch gotten r+? Perhaps the review? clear in comment 2 was a mistake?

Comment 11

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Kevin, your commit broken unit test of calendar.

  ✖ 2 of 744 tests failed:

  1) [calendar] dependencies #loadChildrenSuccess:
     ReferenceError: LazyLoader is not defined
      at appendScript (http://calendar.gaiamobile.org:8080/js/app.js:473)
      at processScripts (http://calendar.gaiamobile.org:8080/js/app.js:507)
      at processRemaining (http://calendar.gaiamobile.org:8080/js/app.js:496)
      at processScripts (http://calendar.gaiamobile.org:8080/js/app.js:494)
      at loadResource (http://calendar.gaiamobile.org:8080/js/app.js:510)
      at view (http://calendar.gaiamobile.org:8080/js/app.js:557)
      at (anonymous) (http://calendar.gaiamobile.org:8080/test/unit/dependency_loader_test.js:39)
      at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:60)
      at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709)
      at runTest (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4081)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4127)
      at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4007)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4016)
      at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3964)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3979)
      at done (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3700)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3712)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:46)
      at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:73)
      at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709)
      at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3973)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3984)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4932)
  

  2) [calendar] dependencies #loadChildrenFailure:
     ReferenceError: LazyLoader is not defined
      at appendScript (http://calendar.gaiamobile.org:8080/js/app.js:473)
      at processScripts (http://calendar.gaiamobile.org:8080/js/app.js:507)
      at processRemaining (http://calendar.gaiamobile.org:8080/js/app.js:496)
      at processScripts (http://calendar.gaiamobile.org:8080/js/app.js:494)
      at loadResource (http://calendar.gaiamobile.org:8080/js/app.js:510)
      at view (http://calendar.gaiamobile.org:8080/js/app.js:557)
      at (anonymous) (http://calendar.gaiamobile.org:8080/test/unit/dependency_loader_test.js:60)
      at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:60)
      at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709)
      at runTest (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4081)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4127)
      at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4007)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4016)
      at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3964)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3979)
      at done (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3700)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3712)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:46)
      at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:73)
      at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709)
      at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3973)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3984)
      at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4932)

Comment 12

[Kevin Grandon :kgrandon]

A fix for the tests landed here: https://github.com/mozilla-b2g/gaia/commit/110094c5f9e3a7bd6b236d71bf310b18c7c3d055

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 714326 [details]
Alternative Design Based on DOMRequest

I am fine with having ~2 lazy loaders if we want to - but would recommend that this ship as part of an individual app, and we can see how it fares, or see how adoption works. The current lazy loader was already being adopted by ~3 apps before we moved it to shared. Nothing against multiple solutions if there is a clear need for it.

I'm going to clear this feedback flag for now.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 714326 [details]
Alternative Design Based on DOMRequest

This strikes me as more than what we need now. I was happy with my own very simple loadScript() file, and I don't think we need to do the whole hierarchical namespace thing.

The only piece of interesting feedback I really have is that I don't care for the 'concurrent' or 'sequential' arguments; they're too hard to remember. I think if I were trying to write a really general script loader that could handle multiple files, I'd write a function that accepted multiple arguments. The arguments would be loaded sequentially. But if any argument was an array, then all elements of the array would be loaded in parallel.

Comment 15

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

Comment on attachment 714326 [details]
Alternative Design Based on DOMRequest

Let's fix the lazy loader that has landed and help it to grow on the right way. 

This is a bit out of scope of this bug but TBH I'm convinced the strong need for a lazy loader could be reduce by an architectural change in the app and I would like to push in this direction instead of maintaining a new big part of code. 

CSS/JS/HTML are expensive if they are not used in both startup time and memory consumption. There is also no clean way to preload a whole panel and to free the memory afterward. 

I'm not 100% sure of the solution right now but I wonder if seamless iframes combined with prefetching/prerendering won't be one of the key here. That would logically split view and that will make them easy to load/unload when needed. I'm still not sure about a proper way to have common things to not took multiple time the memory though.

All that to say - let's no focus too much on a lazy loader because it hides other weakness and this could be something we want to use less in the future. (Not sure we can get rid completely of it though).

Comment 16

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

Comment on attachment 713737 [details]
Github pull request pointer

It has been tested and that would be easier to maintain/scope bugs on 1.0.1 / v1-train with it.

Comment 17

[Jose Manuel Cantera]

(In reply to David Flanagan [:djf] from comment #14)
> Comment on attachment 714326 [details]
> Alternative Design Based on DOMRequest
> 
> This strikes me as more than what we need now. I was happy with my own very
> simple loadScript() file, and I don't think we need to do the whole
> hierarchical namespace thing.
> 
> The only piece of interesting feedback I really have is that I don't care
> for the 'concurrent' or 'sequential' arguments; they're too hard to
> remember. I think if I were trying to write a really general script loader
> that could handle multiple files, I'd write a function that accepted
> multiple arguments. The arguments would be loaded sequentially. But if any
> argument was an array, then all elements of the array would be loaded in
> parallel.

I like this idea. When I have time I will craft it in my component which I'm maintaining at https://github.com/telefonicaid/B2G-Utility-Libraries.JS/tree/master/script-loader

Comment 18

[Jose Manuel Cantera]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #15)
> Comment on attachment 714326 [details]
> Alternative Design Based on DOMRequest
> 
> Let's fix the lazy loader that has landed and help it to grow on the right
> way. 
> 
> This is a bit out of scope of this bug but TBH I'm convinced the strong need
> for a lazy loader could be reduce by an architectural change in the app and
> I would like to push in this direction instead of maintaining a new big part
> of code. 
> 
> CSS/JS/HTML are expensive if they are not used in both startup time and
> memory consumption. There is also no clean way to preload a whole panel and
> to free the memory afterward. 
> 
> I'm not 100% sure of the solution right now but I wonder if seamless iframes
> combined with prefetching/prerendering won't be one of the key here. That
> would logically split view and that will make them easy to load/unload when
> needed. I'm still not sure about a proper way to have common things to not
> took multiple time the memory though.
> 
> All that to say - let's no focus too much on a lazy loader because it hides
> other weakness and this could be something we want to use less in the
> future. (Not sure we can get rid completely of it though).

thanks for the feedback. I would be interested in knowing more about the iframe thing and how it could work

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking tef+ to get this uplifted to branches, as comment 16 states it would be a benefit to those branches and it has gotten a fair amount of testing. If regressions do arise from this uplift it will have to be backed out.

Comment 21

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

Commit ab1d2c275643769a1854c49bfe5a3db2aa972542 does not apply to v1-train or v1.0.1.  This means that there are merge conflicts which need to be resolved.  If there are dependencies that are not approved for branch landing, or have yet to land on master, please let me know

If a manual merge is required, a good place to start might be:
  cd gaia
  git checkout v1-train
  git cherry-pick -x -m1 ab1d2c275643769a1854c49bfe5a3db2aa972542
  <RESOLVE MERGE CONFLICTS>
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 22

[Julien Wajsberg [:julienw]]

2 parts have to be merged :
  * one small part in the gallery app, that is quite easy
  * one big part in the Email app.

I'm waiting for the answer to :asuth mail on this, and depending on the result, I'll go on with making this merge.

FTR this patch is very important for other patches to land.

Comment 23

[Aki Sasaki (not active)]

It appears that not uploading this bug to v1-train has broken the b2g18 builds.
https://tbpl.mozilla.org/?tree=Mozilla-B2g18

Comment 24

[Julien Wajsberg [:julienw]]

Yes, I've seen several bugs being uplifted that were depending on this, I've already warned :jhford bout the ones I saw.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

FWIW, b2g18 is currently closed until this gets resolved.

Comment 26

[Julien Wajsberg [:julienw]]

v1-train: 9d95495833bfefa5e536e16b087f4a92b5cc17ee

\o/

Comment 27

[Julien Wajsberg [:julienw]]

v1.0.1: 379f35db945fc4194a7f381bd381af7eeca449f9

Comment 28

[amiller]

No need to create a test case in Moztrap for this issue.

Comment 29

[amiller]

No way for the QA team to Verify fixed without needed tools.