1266826 - turn Templater.jsm into a .js

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

client/inspector/markup/Templater.jsm is used by the inspector.
For the de-chrome-ification project it should be turned into a .js.

Comment 1

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1266826 - make Templater.jsm eslint-clean; r?jryans

Review commit: https://reviewboard.mozilla.org/r/52025/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52025/

Comment 2

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1266826 - turn Templater.jsm into a .js; r?jryans

Review commit: https://reviewboard.mozilla.org/r/52027/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52027/

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8751461 [details]
MozReview Request: Bug 1266826 - make Templater.jsm eslint-clean; r?jryans

https://reviewboard.mozilla.org/r/52025/#review48909

::: devtools/shared/gcli/Templater.jsm:19
(Diff revision 1)
>   * limitations under the License.
>   */
>  
> +"use strict";
> +
> +/* globals document */

Huh, I did not think JSMs had access to a document... but I guess they must?  Or do we just not use those code paths...?

::: devtools/shared/gcli/Templater.jsm:206
(Diff revision 1)
> -    var childNodes = Array.prototype.slice.call(node.childNodes);
> -    for (var j = 0; j < childNodes.length; j++) {
> +    let childNodes = Array.prototype.slice.call(node.childNodes);
> +    for (let j = 0; j < childNodes.length; j++) {
>        processNode(state, childNodes[j], data);
>      }
>  
> -    if (node.nodeType === 3 /*Node.TEXT_NODE*/) {
> +    /* 3 === Node.TEXT_NODE*/

A space before the ending */ seems good for balance

::: devtools/shared/gcli/Templater.jsm:244
(Diff revision 1)
> -    var recurse = true;
> +    let recurse = true;
>      try {
> -      var reply = envEval(state, value, data, originalValue);
> +      let reply = envEval(state, value, data, originalValue);
>        recurse = !!reply;
> -    }
> -    catch (ex) {
> +    } catch (ex) {
> +      handleError(state, "Error with '" + value + "'", ex);

Could use a template string if you like

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8751462 [details]
MozReview Request: Bug 1266826 - turn Templater.jsm into a .js; r?jryans

https://reviewboard.mozilla.org/r/52027/#review48913

This looks good, but it's not recorded as a move.  Is it possible we can nudge things to do so?  It would be best to keep the history of the file if we can.

Comment 5

[Joe Walker [:jwalker]]

So Templater.jsm is the same as domtemplate.js [1] in GCLI.

There's some magic in the branch that's synced with m-c to avoid code duplication [2][3]. And when I say magic, it's only with an intense feeling of self loathing.

It's like that because we wanted to use the template engine directly from console code and it felt wrong to 'deep require' into GCLI. I strongly suspect that GCLI is now the only user of Templater.jsm, in which case the best thing might be to remove the magic and have just one implementation in domtemplate.js.

[1]: https://github.com/joewalker/gcli/blob/master/lib/gcli/util/domtemplate.js
[2]: https://github.com/mozilla/gcli/blob/mozmaster/lib/gcli/util/domtemplate.js
[3]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/source/lib/gcli/util/domtemplate.js

Comment 6

[Tom Tromey :tromey]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)

> > +/* globals document */
> 
> Huh, I did not think JSMs had access to a document... but I guess they must?
> Or do we just not use those code paths...?

I am not really certain but my impression is that the code was intended to also
run in content.  The code in question is:

function processNode(state, node, data) {
  if (typeof node === "string") {
    node = document.getElementById(node);
  }

... so I'm thinking the devtools just don't use this path.

> A space before the ending */ seems good for balance

Thanks, I meant to do that.

Comment 7

[Tom Tromey :tromey]

(In reply to Joe Walker [:jwalker] (needinfo me or ping on irc) from comment #5)

> It's like that because we wanted to use the template engine directly from
> console code and it felt wrong to 'deep require' into GCLI. I strongly
> suspect that GCLI is now the only user of Templater.jsm, in which case the
> best thing might be to remove the magic and have just one implementation in
> domtemplate.js.

There's a use in devtools/client/inspector/markup/markup.js, which is how this
ended up on the de-chrome-ification shortlist.
My patch changes domtemplate.js to require the newly-renamed template.js.

Comment 8

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1266826 - make Templater.jsm eslint-clean; r=jryans

Review commit: https://reviewboard.mozilla.org/r/52211/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52211/

Comment 9

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1266826 - turn Templater.jsm into a .js; r?jryans

Review commit: https://reviewboard.mozilla.org/r/52213/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52213/

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8751783 [details]
MozReview Request: Bug 1266826 - make Templater.jsm eslint-clean; r=jryans

https://reviewboard.mozilla.org/r/52211/#review49113

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8751784 [details]
MozReview Request: Bug 1266826 - turn Templater.jsm into a .js; r?jryans

https://reviewboard.mozilla.org/r/52213/#review49115

Comment 12

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e7bb7e991f7

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/666e690baccc
https://hg.mozilla.org/integration/fx-team/rev/be6dbfceb79e

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/666e690baccc
https://hg.mozilla.org/mozilla-central/rev/be6dbfceb79e