684958 - DOM Templater should include async functionality via promises

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

DOM Templater should include async functionality via promises. A working implementation can be found at:
https://github.com/joewalker/domtemplate/tree/async

DOM Templater can be found in the Firefox source at:
/browser/devtools/shared/Templater.jsm

Comment 1

[Joe Walker [:jwalker]]

Will create patch

Comment 2

[Joe Walker [:jwalker]]

Attachment: upload 1

Comment 3

[Joe Walker [:jwalker]]

You can view the patch here if you like pain, or you can get the broken down version here https://github.com/joewalker/gcli/pull/2
Comments either here or on github.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 559115 [details] [diff] [review]
upload 1

>+ * 'typeof thing.cloneNode !== "function"' (is there a better way that will
>+ * work in all environments, including a .jsm?)
>+ * Non DOM elements are converted to a string and wrapped in a TextNode.
>+ */
>+Templater.prototype._toNode = function(thing, document) {
>+  if (thing == null) {
>+    thing = '' + thing;
>+  }
>+  // if (isDOMElement(reply)) { ... }
>+  if (typeof thing.cloneNode !== 'function') {
>+    thing = document.createTextNode(thing.toString());
>+  }
>+  return thing;
>+};
>+
>+/**
>+ * A function to handle the fact that some nodes can be promises, so we check

I am not sure that // if (isDOMElement(reply)) { ... } is needed otherwise everything else looks fine. I don't see any obvious opportunities to optimize. feedback+.

Comment 5

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe from comment #4)
> Comment on attachment 559115 [details] [diff] [review]
> upload 1
> 
> >+ * 'typeof thing.cloneNode !== "function"' (is there a better way that will
> >+ * work in all environments, including a .jsm?)
> >+ * Non DOM elements are converted to a string and wrapped in a TextNode.
> >+ */
> >+Templater.prototype._toNode = function(thing, document) {
> >+  if (thing == null) {
> >+    thing = '' + thing;
> >+  }
> >+  // if (isDOMElement(reply)) { ... }
> >+  if (typeof thing.cloneNode !== 'function') {
> >+    thing = document.createTextNode(thing.toString());
> >+  }
> >+  return thing;
> >+};
> >+
> >+/**
> >+ * A function to handle the fact that some nodes can be promises, so we check
> 
> I am not sure that // if (isDOMElement(reply)) { ... } is needed otherwise
> everything else looks fine. I don't see any obvious opportunities to
> optimize. feedback+.

FWIW, I fixed the comment
https://github.com/joewalker/gcli/commit/4a848758795640c0773140019b7a5416dce803a6

Comment 6

[Joe Walker [:jwalker]]

Hey Justin - do you have an ETA on this?
Thanks.

Comment 7

[Joe Walker [:jwalker]]

Hi - I'm going to assume that you've not got time to review this right now, and ask for review from Robcee. Please shout if you would like to do it still. Thanks

Comment 8

[Joe Walker [:jwalker]]

Comment on attachment 559115 [details] [diff] [review]
upload 1

Clearly, Rob - other things take precedence.
The easy way to view this is https://github.com/joewalker/gcli/pull/2

Comment 9

[Rob Campbell [:rc] (:robcee)]

I'll take a look at this on Monday. Thanks for the link!

Comment 10

[Joe Walker [:jwalker]]

Attachment: [in-fx-team] upload 2

See comments in bug 684721. i.e. this patch includes the s/var/const fix.

This (or at leave the var version) is part of this try build https://tbpl.mozilla.org/?tree=Try&rev=c5a5cfe100ae which I'm claiming is clean, but see bug 656666 comment 34 for more.

I'm keen to land this as soon as we can so we can land GCLI.

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 563882 [details] [diff] [review]
[in-fx-team] upload 2

+Templater.prototype._envEval = function(script, data, frame) {

not super-keen on these argument names. "data" doesn't scream "the environment" to me. Nor does "frame" equate to a debugging string, though maybe the contents of that param are typically information about the originating frame, in which case, it's ok!

looks good. Send the tests along promptly!

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 563882 [details] [diff] [review]
[in-fx-team] upload 2

https://hg.mozilla.org/integration/fx-team/rev/4a06aceb7922

Comment 13

[Joe Walker [:jwalker]]

(In reply to Rob Campbell [:rc] (robcee) from comment #11)
> Comment on attachment 563882 [details] [diff] [review] [diff] [details] [review]
> [in-fx-team] upload 2
> 
> +Templater.prototype._envEval = function(script, data, frame) {
> 
> not super-keen on these argument names. "data" doesn't scream "the
> environment" to me. Nor does "frame" equate to a debugging string, though
> maybe the contents of that param are typically information about the
> originating frame, in which case, it's ok!

https://bugzilla.mozilla.org/show_bug.cgi?id=691011#c2

Comment 14

[Joe Walker [:jwalker]]

thanks for the landings

Comment 15

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/4a06aceb7922

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 563882 [details] [diff] [review]
[in-fx-team] upload 2

>diff --git a/browser/devtools/shared/Templater.jsm b/browser/devtools/shared/Templater.jsm
>old mode 100644
>new mode 100755

Why?

Comment 17

[Joe Walker [:jwalker]]

(In reply to Ms2ger from comment #16)
> Comment on attachment 563882 [details] [diff] [review] [diff] [details] [review]
> [in-fx-team] upload 2
> 
> >diff --git a/browser/devtools/shared/Templater.jsm b/browser/devtools/shared/Templater.jsm
> >old mode 100644
> >new mode 100755
> 
> Why?

Because cygwin.

I'm open for suggestions as to how to fix this. Does it need a new bug?

Comment 18

[Rob Campbell [:rc] (:robcee)]

I'm sorry, what?

The mode changed on this file on cygwin systems isn't really worth reopening this bug, imo. Cygwin is not a supported platform anymore.

Open a new bug if really necessary.

Comment 19

[Joe Walker [:jwalker]]

How hard would it be to backout the patch, delete the 2 lines from the patch and re-apply it?

Comment 20

[Joe Walker [:jwalker]]

Better - I've got a fix for bug 692031, I'll include it in there.