Closed Bug 1265885 Opened 4 years ago Closed 4 years ago

add promise.defer

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: tromey, Assigned: jlast)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
Attached patch defer.1.patch (obsolete) — Splinter Review
Attachment #8746587 - Flags: review?(ttromey)
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.
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 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.
Attachment #8746587 - Flags: review?(ttromey) → review?(jryans)
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
Attachment #8746587 - Flags: review?(jryans) → review-
Attached patch defer.2.patch (obsolete) — Splinter Review
Attachment #8746587 - Attachment is obsolete: true
Attachment #8749414 - Flags: review?(jryans)
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...?
Attachment #8749414 - Flags: review?(jryans) → feedback+
Iteration: 49.1 - May 9 → 49.2 - May 23
Attached patch defer.3.patch (obsolete) — Splinter Review
Attachment #8749414 - Attachment is obsolete: true
Keywords: checkin-needed
Is there a Try link handy for this patch?
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Is there a Try link handy for this patch?
Flags: needinfo?(jlaster)
This has some eslint warnings and one error.
I'll upload a new version momentarily and push it to try.
Flags: needinfo?(jlaster)
MozReview-Commit-ID: 1Paf8cfkh5Q
Attachment #8753833 - Attachment is obsolete: true
Attachment #8756432 - Flags: review+
set author on the commit

MozReview-Commit-ID: 1Paf8cfkh5Q
Attachment #8756432 - Attachment is obsolete: true
Attachment #8756494 - Flags: review+
Keywords: checkin-needed
Iteration: 49.2 - May 23 → 49.3 - Jun 6
https://hg.mozilla.org/mozilla-central/rev/1a8b7c916daf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.