GCLI and DOMTemplate could use a Promise implementation

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

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.
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
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.
Attachment #565615 - Flags: review?(mihai.sucan)
Depends on: 690822
Blocks: 691012
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.
Attachment #565615 - Flags: review?(mihai.sucan) → review+
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.
Created attachment 566237 [details] [diff] [review]
upload 2
Attachment #565615 - Attachment is obsolete: true
Try log:
https://tbpl.mozilla.org/?tree=Try&rev=f2eedda104b1
upload 2: https://hg.mozilla.org/integration/fx-team/rev/5727a8f457c1
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5727a8f457c1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
(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 :)
Blocks: 707674
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?
No longer blocks: 659052
You need to log in before you can comment on or make changes to this bug.