turn Templater.jsm into a .js

RESOLVED FIXED in Firefox 49

Status

enhancement
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 49
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

3 years ago
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

Updated

3 years ago
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+
Attachment #8751462 - Flags: review?(jryans)
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
Assignee

Comment 6

3 years ago
(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.
Assignee

Comment 7

3 years ago
(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.
Assignee

Updated

3 years ago
Attachment #8751461 - Attachment is obsolete: true
Assignee

Updated

3 years ago
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+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/666e690baccc
https://hg.mozilla.org/mozilla-central/rev/be6dbfceb79e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Duplicate of this bug: 1271360

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.