It is hoped that this code will be short lived since there are probably better ways to handle asyncronisity, however having a single place for this code is better than having them both smuggle the idea in.

In the week we discussed extracting the promise code from GCLI so it can be shared between GCLI and DOMTemplate. This is that code, replete with tests.

Comment on attachment 565615 [details] [diff] [review] upload 1 Review of attachment 565615 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks fine, but it needs clean ups. Giving it r+ with the comments below addressed. Thank you! ::: browser/devtools/shared/Makefile.in @@ +21,4 @@ > # > # Contributor(s): > # Mike Ratcliffe <mratcliffe@mozilla.com> (Original author) > +# Joe Walker <jwalker@mozilla.com> Please align both names with the "n" in "Contributor(s)". @@ +46,4 @@ > > ifdef ENABLE_TESTS > ifneq (mobile,$(MOZ_BUILD_APP)) > + DIRS += test Please remove the mobile check, while we are here. That's not needed. ::: browser/devtools/shared/Promise.jsm @@ +22,5 @@ > + Promise._outstanding[this._id] = this; > +}; > + > +/** > + * We give promises and ID so we can track which are outstanding Nit: Some descriptions have a period at the end, some don't. Please keep the style consistent through the file. @@ +162,5 @@ > + delete Promise._outstanding[this._id]; > + // The original code includes this very useful debugging aid, however there > + // is concern that it will create a memory leak, so we leave it out here. > + /* > + Promise._recent.push(this); If this is unused, then please remove the code. We can always add it back, later, when it's needed. ::: browser/devtools/shared/test/Makefile.in @@ +11,5 @@ > +# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License > +# for the specific language governing rights and limitations under the > +# License. > +# > +# The Original Code is Style Inspector code. This is not the Inspector code. @@ +14,5 @@ > +# > +# The Original Code is Style Inspector code. > +# > +# The Initial Developer of the Original Code is > +# Mozilla Corporation. The Mozilla Foundation. @@ +15,5 @@ > +# The Original Code is Style Inspector code. > +# > +# The Initial Developer of the Original Code is > +# Mozilla Corporation. > +# Portions created by the Initial Developer are Copyright (C) 2010 2011 @@ +19,5 @@ > +# Portions created by the Initial Developer are Copyright (C) 2010 > +# the Initial Developer. All Rights Reserved. > +# > +# Contributor(s): > +# Joe Walker <jwalker@mozilla.com> (Original author) Please align with "n". ::: browser/devtools/shared/test/browser_promise_basic.js @@ +7,5 @@ > + > +function test() > +{ > + waitForExplicitFinish(); > + addTab("http://example.com/browser/browser/devtools/shared/test/browser_promise_basic.html"); The content of the page seems to be unused? You can just open a blank tab, or don't open any tab at all. ::: browser/devtools/shared/test/head.js @@ +14,5 @@ > + * > + * The Original Code is DevTools test code. > + * > + * The Initial Developer of the Original Code is Mozilla Foundation. > + * Portions created by the Initial Developer are Copyright (C) 2011 *The* Mozilla Foundation. @@ +18,5 @@ > + * Portions created by the Initial Developer are Copyright (C) 2011 > + * the Initial Developer. All Rights Reserved. > + * > + * Contributor(s): > + * Mike Ratcliffe <mratcliffe@mozilla.com> Align with "n". @@ +54,5 @@ > + } > + } > +} > + > +let tab, browser, hudId, hud, hudBox, filterBox, outputNode, cs; This file holds a good deal of HUDService-specific head.js code. Please remove/clean up, and keep only what you need for your tests.

Thanks, some good spots. Some comments inline, if I've snipped it out, consider it done. (In reply to Mihai Sucan [:msucan] from comment #2) > Comment on attachment 565615 [details] [diff] [review] [diff] [details] [review] > > ... > > @@ +162,5 @@ > > + delete Promise._outstanding[this._id]; > > + // The original code includes this very useful debugging aid, however there > > + // is concern that it will create a memory leak, so we leave it out here. > > + /* > > + Promise._recent.push(this); > > If this is unused, then please remove the code. We can always add it back, > later, when it's needed. The trouble is, it's 'needed' now. This code is useful in any environment that doesn't have firefoxes memleak requirements. We keep this code in sync with the same thing elsewhere, so having the commented out code and the explanation helps us keep things in sync properly. > ::: browser/devtools/shared/test/browser_promise_basic.js > @@ +7,5 @@ > > + > > +function test() > > +{ > > + waitForExplicitFinish(); > > + addTab("http://example.com/browser/browser/devtools/shared/test/browser_promise_basic.html"); > > The content of the page seems to be unused? You can just open a blank tab, > or don't open any tab at all. Done - with a hack. Since this creates a new test directory, if the HTML isn't there, then make fails. I could invent a nasty hack of make or simply leave the html in and remove it in bug 691012 which will land with this bug. Thanks, Joe.

Try log: https://tbpl.mozilla.org/?tree=Try&rev=f2eedda104b1

upload 2: https://hg.mozilla.org/integration/fx-team/rev/5727a8f457c1

(In reply to Joe Walker from comment #0) > It is hoped that this code will be short lived since there are probably > better ways to handle asyncronisity, however having a single place for this > code is better than having them both smuggle the idea in. As for me, I rather hope that this code is standardized. I had originally developed my own implementation of promises (slightly more powerful, as it also handled "progress" callbacks, but otherwise similar in spirit and implementation). Rather than having a second (at least) implementation on mozilla-central, I am ditching mine and refactoring OS.File to use yours. Just so that you know :)

TBH, I suggested David to try using a common Promise implementation accross Mozilla codebase. I didn't new one has already landed on m-c! I'd really like to see Mozilla universe working together with an unique API. If Devtools and Perf teams can end up using same API, I'd really like to see Jetpack joining Mozilla teams! You may all know that, but just in case, the ecmascript proposal: http://wiki.ecmascript.org/doku.php?id=strawman:concurrency I think we are polluting this bug, it may make sense to start discussing this somewhere else?