Last Comment Bug 684958 - DOM Templater should include async functionality via promises
: DOM Templater should include async functionality via promises
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
: 674548 (view as bug list)
Depends on: 684721
Blocks: 656668 691011 691012
  Show dependency treegraph
 
Reported: 2011-09-06 13:01 PDT by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2012-01-13 10:37 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upload 1 (17.25 KB, patch)
2011-09-08 05:19 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mratcliffe: feedback+
Details | Diff | Review
[in-fx-team] upload 2 (18.11 KB, patch)
2011-09-30 16:19 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
rcampbell: review+
Details | Diff | Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-06 13:01:58 PDT
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] (needinfo me or ping on irc) 2011-09-06 13:31:36 PDT
Will create patch
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-08 05:19:02 PDT
Created attachment 559115 [details] [diff] [review]
upload 1
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-08 05:20:47 PDT
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 Michael Ratcliffe [:miker] [:mratcliffe] 2011-09-08 14:18:54 PDT
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] (needinfo me or ping on irc) 2011-09-09 06:58:10 PDT
(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] (needinfo me or ping on irc) 2011-09-20 13:10:58 PDT
Hey Justin - do you have an ETA on this?
Thanks.
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-22 13:31:48 PDT
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] (needinfo me or ping on irc) 2011-09-23 09:29:13 PDT
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) 2011-09-23 12:15:42 PDT
I'll take a look at this on Monday. Thanks for the link!
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-09-30 16:19:40 PDT
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.
Comment 11 Rob Campbell [:rc] (:robcee) 2011-10-03 06:37:50 PDT
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) 2011-10-03 08:23:02 PDT
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] (needinfo me or ping on irc) 2011-10-03 12:09:33 PDT
(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] (needinfo me or ping on irc) 2011-10-03 12:10:20 PDT
thanks for the landings
Comment 15 Rob Campbell [:rc] (:robcee) 2011-10-04 05:23:23 PDT
https://hg.mozilla.org/mozilla-central/rev/4a06aceb7922
Comment 16 :Ms2ger 2011-10-05 01:00:58 PDT
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] (needinfo me or ping on irc) 2011-10-05 02:33:21 PDT
(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) 2011-10-05 06:08:50 PDT
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] (needinfo me or ping on irc) 2011-10-05 06:40:28 PDT
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] (needinfo me or ping on irc) 2011-10-05 07:12:02 PDT
Better - I've got a fix for bug 692031, I'll include it in there.
Comment 21 Dave Camp (:dcamp) 2011-10-27 09:28:38 PDT
*** Bug 674548 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.