Closed
Bug 1266826
Opened 8 years ago
Closed 8 years ago
turn Templater.jsm into a .js
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
Tracking
(firefox49 fixed)
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.
Updated•8 years ago
|
Flags: qe-verify-
Priority: -- → P2
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52025/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52025/
Attachment #8751461 -
Flags: review?(jryans)
Attachment #8751462 -
Flags: review?(jryans)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52027/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52027/
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.
Updated•8 years ago
|
Iteration: --- → 49.2 - May 23
Priority: P2 → P1
Comment 5•8 years ago
|
||
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•8 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•8 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 | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52211/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52211/
Attachment #8751783 -
Flags: review?(jryans)
Attachment #8751784 -
Flags: review?(jryans)
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52213/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52213/
Assignee | ||
Updated•8 years ago
|
Attachment #8751461 -
Attachment is obsolete: true
Assignee | ||
Updated•8 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 | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e7bb7e991f7
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/666e690baccc https://hg.mozilla.org/integration/fx-team/rev/be6dbfceb79e
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/666e690baccc https://hg.mozilla.org/mozilla-central/rev/be6dbfceb79e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•