1014686 - [Messages][Refactoring] Improve "html template string" to "dom node\fragment" conversion

Status: RESOLVED FIXED

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

Description

[Oleg Zasypkin [:azasypkin]]

*** Follow-up from bug 963013 ***

Currently we use "ThreadUI._htmlStringToDomNode" method to convert string result of "Template.interpolate" into DOM node. We should get red of it from ThreadUI to make code cleaner and safer.

One of the ideas would be to extend "Template.interpolate" so that it'll allow to use either DocumentFragment or HTML string as result and hide that complexity from consumer code.

Comment 1

[Oleg Zasypkin [:azasypkin]]

Hey Julien,

Just idea with template tag, what if we do Template.interpolate like this?

Template.prototype.interpolate = function(data, options) {
    // This _should_ be rewritten to use Firefox's support for ES6
    // default parameters:
    // ... = function(data, options = { safe: [] }) {
    //
    options = options || {};
    options.safe = options.safe || [];

    var html = priv.get(this).tmpl.replace(rmatcher, function(match, property) {
      property = property.trim();
      // options.safe is an array of properties that can be ignored
      // by the "suspicious" html strategy.
      return options.safe.indexOf(property) === -1 ?
        // Any field that is not explicitly listed as "safe" is
        // to be treated as suspicious
        Template.escape(data[property]) :
        // Otherwise, return the string of rendered markup
        data[property];
    });

    var documentFragment = null;

    return {
      toHTML: function t_toHTML() {
        return html;
      },

      toDocumentFragment: function t_toDocumentFragment() {
        if (!documentFragment) {
          // This node can be extracted to upper scope to always use the same
          // intermediate node
          var templateNode = document.createElement('template');
          templateNode.innerHTML = html;
          documentFragment = templateNode.content;
        }
        
        return documentFragment.cloneNode(true);
      }
    };
  };

Comment 2

[Julien Wajsberg [:julienw]]

Why not :)

I'd keep the 'interpolate' we have currently (so that we don't need to change all apps) but have another method that would do what you're suggesting (we could call it 'prepare'). And 'interpolate' would just call 'this.prepare().toHTML()'.

We can use "toMarkup" or "toString" instead of "toHTML". I'm not sure. :)

Comment 3

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL v1

Hey Julien,

Here is patch for this change, could you please take a look?

Thanks!

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8492871 [details] [review]
GitHub pull request URL v1

r=me for the SMS part, feedback+ for the template.js part, but requesting a formal review from David Flanagan who's supposed to own /shared.

David, Bocoup team wrote most of this library (I reviewed a good part of it back at the time when it was in the SMS app), but now that it's in /shared I feel you should own this file too.

The goal of this patch is to be able to get a DocumentFragment from the template interpolation step if the calling code needs it.

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8492871 [details] [review]
GitHub pull request URL v1

Oleg,

Sorry that this took me so long to review.

I'm giving r- because you've made the existing interpolate() function call the new prepare() function, making it (slightly) less efficient. I'd prefer a solution that left interpolate() alone and created a new toDocumentFragment() (or prepare(), if you want) based on the unmodified interpolate().

I don't actually believe that there is a use case for calling prepare() once and then calling toString or toDocumentFragment() multiple times on that prepared template. So it seems to me that just having interpolate() and toDocumentFragment() methods would suffice. Though if you've got a use case you could convince me of the need for a prepare() method.

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8492871 [details] [review]
GitHub pull request URL v1

Hey David,

Thanks for review! 

(In reply to David Flanagan [:djf] from comment #5)
> Comment on attachment 8492871 [details] [review]
> GitHub pull request URL v1
> 
> Oleg,
> 
> Sorry that this took me so long to review.
> 
> I'm giving r- because you've made the existing interpolate() function call
> the new prepare() function, making it (slightly) less efficient. I'd prefer
> a solution that left interpolate() alone and created a new
> toDocumentFragment() (or prepare(), if you want) based on the unmodified
> interpolate().

Done!

> 
> I don't actually believe that there is a use case for calling prepare() once
> and then calling toString or toDocumentFragment() multiple times on that
> prepared template. So it seems to me that just having interpolate() and
> toDocumentFragment() methods would suffice. Though if you've got a use case
> you could convince me of the need for a prepare() method.

Julien mentioned use case from Messages app at GitHub (that is actually included in this patch), please tell me if it makes sense for you.

Thanks!

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8492871 [details] [review]
GitHub pull request URL v1

r+ if you remove the code that caches the html string and template element, or if you get Julien to say that he wants you to keep those.

Comment 8

[Oleg Zasypkin [:azasypkin]]

(In reply to David Flanagan [:djf] from comment #7)
> Comment on attachment 8492871 [details] [review]
> GitHub pull request URL v1
> 
> r+ if you remove the code that caches the html string and template element,
> or if you get Julien to say that he wants you to keep those.

Thanks for review, caching code is removed, Treeherder is green.

Master: https://github.com/mozilla-b2g/gaia/commit/16b17e7af2d711329de4581a5fca6614aa6c717b