1265885 - add promise.defer

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

We need promise.defer in devtools, so it will have to be reimplemented
for the de-chrome-ification project.
There's already a copy in devtools.html, but it needs tests at least.
Also some loader magic may be involved here.

Comment 1

[Jason Laster [:jlast]]

Attachment: defer.1.patch

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8746587 [details] [diff] [review]
defer.1.patch

Review of attachment 8746587 [details] [diff] [review]:
-----------------------------------------------------------------

I am not sure about this...  This uses DOM promises internally whereas we still use Promise.jsm throughout DevTools.

I realize you can't use Promise.jsm from content code, so should we do something to avoid proliferating another promises module?  Not sure of my opinions yet, just want to raise the issue.

Also should this module be available to all DevTools code?  It might make more since under /devtools/shared then, instead of /devtools/client.

Comment 3

[Jason Laster [:jlast]]

Thanks jryans for bringing up these questions. I meant to raise them in my review.

> should we do something to avoid proliferating another promises module?
I'm not sure of the answer, except that Promise.jsm will obviously not work for *.html projects.

> should this module be available to all DevTools code
At this point I was operating under the assumption that the server would still use Promise.jsm. Does that make sense?

Comment 4

[Tom Tromey :tromey]

Comment on attachment 8746587 [details] [diff] [review]
defer.1.patch

Review of attachment 8746587 [details] [diff] [review]:
-----------------------------------------------------------------

I'm switching this review to :jryans because he definitely knows a lot more about this area than I do.

I tend to think this should go in devtools/shared.  Looking for uses there I see, e.g.

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/DevToolsUtils.js#53

I think this can be run on the client.

I wonder if the defer function should only be installed if it isn't already available.
Also, I wonder if this should only install the "defer" replacement function if no such
function is already available.  And, perhaps maybe this should all be hidden in the loader,
also not sure.

::: devtools/client/shared/promise.js
@@ +1,1 @@
> +Promise.defer = function defer() {

There's some boilerplate that should go at the top of new files.

Also I think it would be good if new files were born eslint-clean and had an inclusion
in .eslintignore (like !devtools/x/y/z.js).

Finally, I think a jsdoc comment is needed.

::: devtools/client/shared/test/unit/test_promise.js
@@ +1,1 @@
> +var Cu = Components.utils;

Standard comment header.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8746587 [details] [diff] [review]
defer.1.patch

Review of attachment 8746587 [details] [diff] [review]:
-----------------------------------------------------------------

Since "Promise.defer" doesn't seem to be part of a future JS version, I think we should avoid installing it onto Promise directly like the approach here.

Instead, let's expose it as a utility function.  Since it has a clear, independent purpose I think it should just be a "defer" module that exposes this.  I think it's okay for it to use Promise internally (instead of Promise.jsm) since that's where we want to head in general anyway.

Let's also move to devtools/shared for general access.  So, that would be devtools/shared/defer.js

Comment 6

[Jason Laster [:jlast]]

Attachment: defer.2.patch

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8749414 [details] [diff] [review]
defer.2.patch

Review of attachment 8749414 [details] [diff] [review]:
-----------------------------------------------------------------

Seems right, but still contains a file from the last version.

::: devtools/client/shared/promise.js
@@ +1,1 @@
> +Promise.defer = function defer() {

I think this file should have been removed...?

Comment 8

[Jason Laster [:jlast]]

Attachment: defer.3.patch

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for this patch?

Comment 10

[Marco Mucci [:MarcoM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Is there a Try link handy for this patch?

Comment 11

[Tom Tromey :tromey]

This has some eslint warnings and one error.
I'll upload a new version momentarily and push it to try.

Comment 12

[Tom Tromey :tromey]

Attachment: add devtools module to replace promise.defer

MozReview-Commit-ID: 1Paf8cfkh5Q

Comment 13

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd358ae13a95

Comment 14

[Tom Tromey :tromey]

Attachment: add devtools module to replace promise.defer

set author on the commit

MozReview-Commit-ID: 1Paf8cfkh5Q

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1a8b7c916daf

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a8b7c916daf