Page MenuHomePhabricator

Refactor 'mobile.startup/Icon' class for performance
Closed, DuplicatePublic

Description

Disclaimer: I've only run through the code once and may've misunderstood parts.

Typical usage:

// Via icons factory
icons.spinner().toHtmlString();
// Or directly
new Icon( .. ).toHtmlString();
  • Constructor (View#initialize)
    • The Icon class in MobileFrontend JavaScript extends the mobile.startup/View class.
    • Given that Icon has a Hogan template set, View#initialize takes the if template path where possible and precompiles the template for later use.
    • The Icon class still has a tagName configured (not sure why, since it uses a template) and View creates an empty DOM element to be never used.
    • Further down the constructor it calls View#render(). I'd expect a consumer to have to all render() or indirectly by another method, not in the constructor already.
  • Render (View#render)
    • Icon has a template and isTemplateMode, so render():
    • Calls Hogan to produce an HTML string.
    • Discards the previously created DOM element.
    • Implicitly parses the HTML into a new DOM element with jQuery.
  • Embed (Icon#toHtmlString)
    • Creates another DOM element.
    • Appends the rendered node.
    • Calls on jQuery to let the browser serialise the node back to an HTML serialised string.
    • Embed the string in a larger HTML template, or call on jQuery to let the browser parse the HTML back into (another) DOM node.

This all seems quite wasteful.

Event Timeline

Yeh the Button, Icon and Anchor classes all follow this pattern. The hope was to convert these into more OOjs UI friendly widgets, however all those efforts have since stalled. We still haven't worked out how OOjs UI and the mobile template approach could play nicely together.

Would be great to revisit this next quarter and try and clean all this up.

Jdlrobson triaged this task as Medium priority.Mar 15 2016, 5:12 PM

Team should chat about this and work out a way forward.

MBinder_WMF subscribed.

JonR to review this and make actionable