James Burke [:jrburke] Assignee
	Description • 11 years ago

This is a dev refactoring ticket. Goals:

* Try out custom elements now that they are available in the platform (at least for certified apps for now).

* Use a custom element approach that uses mixins that help with auto-wiring DOM elements in the template to the custom element and for binding events to those sub-elements. So now the cards should not need to do domNode.getElementsByClassName or addEventListener for the simple cases.

* Take the opportunity to remove the "mode" and "tray" concepts from the Cards implementation to simplify it.

* Try out native Promises inside alameda.

* Make sure `evt` async notifies listeners on an emit. I do not expect any drastic breakage, but given that it does change the execution model a bit, good to do early as part of a bigger experiment like this.

	James Burke [:jrburke] Assignee
	Comment 1 • 11 years ago

Pointer to pull request. Just informational right now, to gather feedback. Best to leave comments in the the pull request.

	Andrew Sutherland [:asuth] (he/him)
	Comment 2 • 11 years ago

Woo!  This looks like a great clean-up refactoring that orients towards us eventually being able to do fancier custom elements stuff in the future.  I agree that it would be good to land this sooner than later.  I will do my part and finish out my long pole integration test stuff so that we don't have to be worried about landing these changes.

I love that data-prop/data-event and the mix-in first strategy will help us clean up a lot of the crappy boilerplate logic we have now.  Bonus points for the binding mechanism for those two things being very intuitive.  data-prop and data-event jump out as being manually hooked-up magic.

(Self-mooting bike-shedding: it might be slightly more obvious if mixins/data-prop and mixins/data-event weren't pushed down into cardBase(). On the flip-side, then that's more copy-and-paste stuff.  And also the files are already named mixins/data-prop.js and mixins/data-event.js, so it's almost embarrassingly easy to find out how that works.  So yeah, you definitely made the right call there!)

	James Burke [:jrburke] Assignee
	Comment 3 • 10 years ago

The pull request for the cards as custom elements switch. It is fairly large, and if the reviewer would like to switch off review until later, that is completely fine. I believe it is ready for review now though.

I need to figure out why `make test-perf` is not running for me at the moment, but also not expecting big surprises there. But I will post numbers hopefully in the next few days so that we have the data.

	James Burke [:jrburke] Assignee
	Comment 4 • 10 years ago

Comment on attachment 8477748 [details]
GitHub pull request

Clearing review for now, going to hold off on this for 2.1, likely pick it up in the next feature cycle.

	James Burke [:jrburke] Assignee
	Comment 5 • 10 years ago

Comment on attachment 8477748 [details]
GitHub pull request

Formally switching review on, although reviewer has seen the previous iteration of this code. This latest one is really just a remastered version (now with Dolby Surround Sound!). The approaches did not change though from before. 

The most notable thing from the previous iteration is likely the working HTML cache. Nothing too fancy though: if dataset.cached is set, then the template.js module does not wipe the innerHTML of the HTML template, but instead just treats the existing nodes as the template to start. The card custom components make a point to not use shadow DOM, but to allow for this sort of transparent innards so that the caching works.

test-perf results, where the items with 'branch-' refers to the pull request branch, and 'master-' is current gaia master without the branch changes. I just show the summary/average numbers for each metric:

No accounts configured:

{
  "passes": [
    {
      "title": "startup > memory",
      "branch-mozPerfMemoryAverage": {
        "app": {
          "uss": "19.000",
          "pss": "22.600",
          "rss": "33.820",
          "vsize": "99.740"
        },
        "system": {
          "uss": "56.700",
          "pss": "61.300",
          "rss": "73.840",
          "vsize": "223.080"
        }
      }
      "master-mozPerfMemoryAverage": {
        "app": {
          "uss": "19.120",
          "pss": "22.740",
          "rss": "33.780",
          "vsize": "99.960"
        },
        "system": {
          "uss": "58.500",
          "pss": "62.500",
          "rss": "74.080",
          "vsize": "225.100"
        }
      }
    },
    {
      "title": "startup > moz-chrome-dom-loaded",
      "branch-mozPerfDurationsAverage": 328,
      "mozPerfDurationsAverage": 340.8
    },
    {
      "title": "startup > moz-chrome-interactive",
      "branch-mozPerfDurationsAverage": 1510.2,
      "master-mozPerfDurationsAverage": 1465.8
    },
    {
      "title": "startup > moz-app-visually-complete",
      "branch-mozPerfDurationsAverage": 328.8,
      "master-mozPerfDurationsAverage": 341,
      "mozPerfGoal": 1000
    },
    {
      "title": "startup > moz-content-interactive",
      "branch-mozPerfDurationsAverage": 1510,
      "master-mozPerfDurationsAverage": 1465.6
    },
    {
      "title": "startup > moz-app-loaded",
      "branch-mozPerfDurationsAverage": 2201.6,
      "master-mozPerfDurationsAverage": 2243.4
    }
  ]
}

One IMAP account configure:

{
  "passes": [
    {
      "title": "startup > memory",
      "branch-mozPerfMemoryAverage": {
        "app": {
          "uss": "23.300",
          "pss": "26.780",
          "rss": "37.460",
          "vsize": "120.200"
        },
        "system": {
          "uss": "56.100",
          "pss": "59.620",
          "rss": "70.200",
          "vsize": "247.060"
        }
      },
      "master-mozPerfMemoryAverage": {
        "app": {
          "uss": "23.640",
          "pss": "27.160",
          "rss": "37.780",
          "vsize": "121.300"
        },
        "system": {
          "uss": "55.060",
          "pss": "58.760",
          "rss": "69.600",
          "vsize": "249.000"
        }
      }
    },
    {
      "title": "startup > moz-chrome-dom-loaded",
      "branch-mozPerfDurationsAverage": 370.8,
      "master-mozPerfDurationsAverage": 392.2
    },
    {
      "title": "startup > moz-app-visually-complete",
      "branch-mozPerfDurationsAverage": 371.2,
      "master-mozPerfDurationsAverage": 392.6,
      "mozPerfGoal": 1000
    },
    {
      "title": "startup > moz-content-interactive",
      "branch-mozPerfDurationsAverage": 1584
      "master-mozPerfDurationsAverage": 1551.6
    },
    {
      "title": "startup > moz-chrome-interactive",
      "branch-mozPerfDurationsAverage": 1585,
      "master-mozPerfDurationsAverage": 1552.2
    },
    {
      "title": "startup > moz-app-loaded",
      "branch-mozPerfDurationsAverage": 3776.4,
      "master-mozPerfDurationsAverage": 3412.6
    }
  ]
}

While the html cache numbers seem better generally, I think they are within a margin of error: I saw at least a 40ms swing in the cache numbers between two different runs on the same master code for instance. So I do not think the new code is necessarily consistently faster for the steps that use the cached DOM (the ones under 1000ms).

Similarly, for the "one account, moz-app-loaded" number, I saw a 200ms swing in it for same code on master. Even factoring that into play, I think we took a small hit to the later numbers in the one account case.

My conjecture is it may be due to using and evt is now async on emits, which means that could delay some of the notifications a bit if other stuff gets in the event loop, like a paint. Also could be a bit of custom element overhead, but that one is harder to do a head-to-head test without building out some very specific tests that match the custom elements with a non-custom element implementation (like the old this.domNode way we did it before).

In summary, I think this is fine for landing though, and worth just doing follow up profiling of specific pathways. One thing I did not do in this changeset was convert gelam to use the same alameda that removes the prim promise shim, so I can see digging more into optimizing the worker startup in that case when doing that change. I will file a follow-up ticket for that if this change lands.

	Andrew Sutherland [:asuth] (he/him)
	Comment 6 • 10 years ago

Comment on attachment 8477748 [details]
GitHub pull request

Super fantastic work; a most excellent and overdue spring-cleaning/spring reconstruction of a terrifying house ;)

r=asuth with requests on the pull request addressed; my primary concern is the search glitch and the suspicious repeated log entries related to that.  I expect that to end up being a shallow/trivial problem so I don't think I'll need to re-review, but feel free to request if it turns out to be an iceberg of some sort.

	James Burke [:jrburke] Assignee
	Comment 7 • 10 years ago

Merged in master:
https://github.com/mozilla-b2g/gaia/commit/7fc32b16fabbe0ec33135f8864066cbde702f1c0

from pull request:
https://github.com/mozilla-b2g/gaia/pull/23230