The default bug view has changed. See this FAQ.

DOM Templater should include async functionality via promises

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: miker, Assigned: jwalker)

Tracking

unspecified
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 1 obsolete attachment)

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
Depends on: 684721
Will create patch
Assignee: nobody → jwalker
Created attachment 559115 [details] [diff] [review]
upload 1
Attachment #559115 - Flags: feedback?(mratcliffe)
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.
Attachment #559115 - Flags: review?(dolske)
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+.
Attachment #559115 - Flags: feedback?(mratcliffe) → feedback+
(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
Blocks: 659052
Hey Justin - do you have an ETA on this?
Thanks.
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 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
Attachment #559115 - Flags: review?(dolske) → review?(rcampbell)
I'll take a look at this on Monday. Thanks for the link!
Blocks: 656668
Created attachment 563882 [details] [diff] [review]
[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.
Attachment #559115 - Attachment is obsolete: true
Attachment #559115 - Flags: review?(rcampbell)
Attachment #563882 - Flags: review?(rcampbell)
Blocks: 691011
Blocks: 691012
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!
Attachment #563882 - Flags: review?(rcampbell) → review+
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment on attachment 563882 [details] [diff] [review]
[in-fx-team] upload 2

https://hg.mozilla.org/integration/fx-team/rev/4a06aceb7922
Attachment #563882 - Attachment description: upload 2 → [in-fx-team] upload 2
(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
thanks for the landings
https://hg.mozilla.org/mozilla-central/rev/4a06aceb7922
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
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?
(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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
How hard would it be to backout the patch, delete the 2 lines from the patch and re-apply it?
Better - I've got a fix for bug 692031, I'll include it in there.

Updated

6 years ago
Duplicate of this bug: 674548
No longer blocks: 659052
You need to log in before you can comment on or make changes to this bug.