Last Comment Bug 692424 - GCLI and DOMTemplate could use a Promise implementation
: GCLI and DOMTemplate could use a Promise implementation
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
Depends on: 690822
Blocks: 691012 707674
  Show dependency treegraph
 
Reported: 2011-10-06 06:12 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-01-13 10:37 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
upload 1 (22.13 KB, patch)
2011-10-07 12:50 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: review+
Details | Diff | Splinter Review
upload 2 (19.59 KB, patch)
2011-10-11 08:47 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-06 06:12:27 PDT
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.
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-07 12:50:26 PDT
Created attachment 565615 [details] [diff] [review]
upload 1

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 2 Mihai Sucan [:msucan] 2011-10-10 04:58:14 PDT
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.
Comment 3 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-11 08:45:58 PDT
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.
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-11 08:47:07 PDT
Created attachment 566237 [details] [diff] [review]
upload 2
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-12 07:52:19 PDT
Try log:
https://tbpl.mozilla.org/?tree=Try&rev=f2eedda104b1
Comment 6 Rob Campbell [:rc] (:robcee) 2011-10-12 10:14:18 PDT
upload 2: https://hg.mozilla.org/integration/fx-team/rev/5727a8f457c1
Comment 7 Rob Campbell [:rc] (:robcee) 2011-10-13 09:51:49 PDT
https://hg.mozilla.org/mozilla-central/rev/5727a8f457c1
Comment 8 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-07 07:06:17 PST
(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 :)
Comment 9 Alexandre Poirot [:ochameau] PTO, back on 1st 2011-12-07 07:18:49 PST
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?

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