Closed Bug 1266826 Opened 4 years ago Closed 4 years ago

turn Templater.jsm into a .js

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.2 - May 23
Tracking Status
firefox49 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files, 2 obsolete files)

client/inspector/markup/Templater.jsm is used by the inspector.
For the de-chrome-ification project it should be turned into a .js.
Flags: qe-verify-
Priority: -- → P2
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
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
Attachment #8751461 - Flags: review?(jryans) → review+
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.
Iteration: --- → 49.2 - May 23
Priority: P2 → P1
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
(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.
(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.
Attachment #8751461 - Attachment is obsolete: true
Attachment #8751462 - Attachment is obsolete: true
Comment on attachment 8751783 [details]
MozReview Request: Bug 1266826 - make Templater.jsm eslint-clean; r=jryans

https://reviewboard.mozilla.org/r/52211/#review49113
Attachment #8751783 - Flags: review?(jryans) → review+
Comment on attachment 8751784 [details]
MozReview Request: Bug 1266826 - turn Templater.jsm into a .js; r?jryans

https://reviewboard.mozilla.org/r/52213/#review49115
Attachment #8751784 - Flags: review?(jryans) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/666e690baccc
https://hg.mozilla.org/mozilla-central/rev/be6dbfceb79e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Duplicate of this bug: 1271360
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.