Closed Bug 1273941 Opened 8 years ago Closed 8 years ago

Convert client promise.jsm defer uses to promise_defer

Categories

(DevTools :: Framework, defect, P1)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.2 - Jul 4
Tracking Status
firefox50 --- fixed

People

(Reporter: jlast, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(9 files, 1 obsolete file)

58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
48 bytes, text/x-phabricator-request
Details | Review
There are several places in the client where promise.jsm is used for defer and we should start using promise_defer
Whiteboard: [devtools-html]
Flags: qe-verify?
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Priority: -- → P1
Every place that changes to the defer library implicitly changes to using native promises instead of Promise.jsm.  This means we would lose promise rejection tracking for all such places until it is implemented for native promises (bug 1242505).

If we want to make this switch without losing test coverage, maybe we should implement bug 1242505 first?
I think it should be a requirement, to avoid the problems we got into last time this stuff
disappeared.
Depends on: 1242505
Assignee: jlaster → nobody
Status: ASSIGNED → NEW
Iteration: 49.2 - May 23 → ---
Priority: P1 → P3
I discussed this on irc with :jryans and we came to the following conclusions:

1. Some spots in-tree explicitly import or require Promises.jsm or sdk/promises.js;
   these should be rewritten to use the "promise" provided by the devtools loader.
2. The defer module can just use "promise"
3. We can drop the 1242505 dependency.

We might want to resurrect 1242505 depending on our in-content test plan.
It's unclear so far.

I'll be filing a new bug because we will need to deal with the fact
that the devtools loader makes "promise" available.  Either the in-content
loader will need to do the same, or we'll need some pass over devtools
to change this.

Finally, we may be able to do some testing (not for checkin purposes) by changing
the devtools loader to use DOM promises as the "promise" implementation.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
No longer depends on: 1242505
Iteration: --- → 49.3 - Jun 6
Priority: P3 → P1
(In reply to Tom Tromey :tromey from comment #3)

> I'll be filing a new bug because we will need to deal with the fact
> that the devtools loader makes "promise" available.

Actually I think I will just update the existing loader bug to note
some requirements, like that it must provide a "promise" module somehow.
Depends on: 1277243
Flags: qe-verify? → qe-verify-
Iteration: 49.3 - Jun 6 → 50.1
Review commit: https://reviewboard.mozilla.org/r/58770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58770/
Attachment #8761680 - Flags: review?(jryans)
Attachment #8761681 - Flags: review?(jryans)
Attachment #8761682 - Flags: review?(jryans)
Attachment #8761683 - Flags: review?(jryans)
Attachment #8761684 - Flags: review?(jryans)
Attachment #8761685 - Flags: review?(jryans)
Attachment #8761686 - Flags: review?(jryans)
Attachment #8761687 - Flags: review?(jryans)
Comment on attachment 8761680 [details]
Bug 1273941 - replace uses of promise.defer in devtools/shared;

https://reviewboard.mozilla.org/r/58770/#review55886

Looks good.  Do we need some way to ensure people use this approach in new files?
Attachment #8761680 - Flags: review?(jryans) → review+
Comment on attachment 8761681 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/shared;

https://reviewboard.mozilla.org/r/58772/#review55888

::: devtools/client/shared/test/browser_treeWidget_keyboard_interaction.js:11
(Diff revision 1)
>  
>  const TEST_URI = "data:text/html;charset=utf-8,<head>" +
>    "<link rel='stylesheet' type='text/css' href='chrome://devtools/skin/widg" +
>    "ets.css'></head><body><div></div><span></span></body>";
>  const {TreeWidget} = require("devtools/client/shared/widgets/TreeWidget");
>  const Promise = require("promise");

Can this be removed?

::: devtools/client/shared/test/head.js
(Diff revision 1)
>  // shared-head.js handles imports, constants, and utility functions
>  Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js", this);
>  
>  const {DOMHelpers} = Cu.import("resource://devtools/client/shared/DOMHelpers.jsm", {});
>  const {Hosts} = require("devtools/client/framework/toolbox-hosts");
> -const {defer} = require("promise");

Some of the tests in this directory seem to be use `defer` without importing...  Is that right?
Comment on attachment 8761682 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/framework;

https://reviewboard.mozilla.org/r/58774/#review55896
Attachment #8761682 - Flags: review?(jryans) → review+
Comment on attachment 8761683 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/inspector;

https://reviewboard.mozilla.org/r/58776/#review55898
Attachment #8761683 - Flags: review?(jryans) → review+
Comment on attachment 8761684 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/styleeditor;

https://reviewboard.mozilla.org/r/58778/#review55900
Attachment #8761684 - Flags: review?(jryans) → review+
Comment on attachment 8761685 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/eyedropper;

https://reviewboard.mozilla.org/r/58780/#review55904
Attachment #8761685 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)

> Looks good.  Do we need some way to ensure people use this approach in new
> files?

Yes, good question.  I had considered it, but my thinking was that a new eslint rule
would be premature, considering that I have only converted the "plausibly inspector-related"
subset of devtools directories.

What do you think?

I was planning to send an email on the topic.

Another thought was to polyfill promise.defer at startup and just drop all these patches.
But I didn't do this as the explicit approach was what we'd agreed on.
(In reply to Tom Tromey :tromey from comment #19)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> 
> > Looks good.  Do we need some way to ensure people use this approach in new
> > files?
> 
> Yes, good question.  I had considered it, but my thinking was that a new
> eslint rule
> would be premature, considering that I have only converted the "plausibly
> inspector-related"
> subset of devtools directories.
> 
> What do you think?

Okay, I think I agree that we can wait until later when it's used more thoroughly.

> Another thought was to polyfill promise.defer at startup and just drop all
> these patches.
> But I didn't do this as the explicit approach was what we'd agreed on.

I still think explicit approach is best.  It feels wrong for a module to add things to a language object like Promise.  If Promise.defer was on track for a future JS version, then a polyfill approach would seem acceptable, but AFAIK it currently is not.
Comment on attachment 8761686 [details]
Bug 1273941 - do not Cu.import promises in devtools;

https://reviewboard.mozilla.org/r/58782/#review55918

You should also fix this file:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/doorhanger.js#11

(assuming it's not already in some other patch).

::: devtools/server/tests/mochitest/test_inspector-resize.html:16
(Diff revision 1)
>    <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
>    <script type="application/javascript;version=1.8" src="inspector-helpers.js"></script>
>    <script type="application/javascript;version=1.8">
>  window.onload = function() {
>    const Cu = Components.utils;
>    Cu.import("resource://devtools/shared/Loader.jsm");

Change this to:

```
const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
```

here and in the similar tests.  (I don't think I searched HTML files like this in my recent Cu.import efforts.)

::: devtools/server/tests/mochitest/test_inspector-resize.html:17
(Diff revision 1)
>    <script type="application/javascript;version=1.8" src="inspector-helpers.js"></script>
>    <script type="application/javascript;version=1.8">
>  window.onload = function() {
>    const Cu = Components.utils;
>    Cu.import("resource://devtools/shared/Loader.jsm");
> -  const {Promise: promise} =
> +  const Promise = devtools.require("promise");

Our convention is to use `promise` as the variable name when importing `Promise.jsm` (instead of `Promise` the language built-in).
Attachment #8761686 - Flags: review?(jryans) → review+
Comment on attachment 8761687 [details]
Bug 1273941 - remove last use of sdk/core/promise from devtools;

https://reviewboard.mozilla.org/r/58784/#review55926
Attachment #8761687 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)

> Some of the tests in this directory seem to be use `defer` without
> importing...  Is that right?

Yes, because defer is loaded by devtools/client/framework/test/shared-head.js.
It occurs to me now that I may need to either squash some patches or
change the ordering for this series to make sense.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> >  const Promise = require("promise");
> 
> Can this be removed?

Yes; I've removed it now.
(In reply to Tom Tromey :tromey from comment #23)

> It occurs to me now that I may need to either squash some patches or
> change the ordering for this series to make sense.

I handled this instead by moving the shared-head.js hunk that defines "defer"
into the client/shared patch.  This way the ordering works out ok.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #21)

> You should also fix this file:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/
> doorhanger.js#11
> (assuming it's not already in some other patch).

It is.

> ::: devtools/server/tests/mochitest/test_inspector-resize.html:16
[...]
> Change this to:
> 
> ```
> const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
> ```
> 
> here and in the similar tests.

Done.

> Our convention is to use `promise` as the variable name when importing
> `Promise.jsm` (instead of `Promise` the language built-in).

Done here, and I've tacked on an additional patch that makes this change
across devtools.
(In reply to Tom Tromey :tromey from comment #25)

> > Our convention is to use `promise` as the variable name when importing
> > `Promise.jsm` (instead of `Promise` the language built-in).
> 
> Done here, and I've tacked on an additional patch that makes this change
> across devtools.

... except in defer.js due to eslint:

  16:20  error  A constructor name should not start with a lowercase letter  new-cap
In devtools the convention is that the result of `require("promise")`
should be called "promise", not "Promise".  This patch fixes places in
devtools that were violating this convention.

Review commit: https://reviewboard.mozilla.org/r/59016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59016/
Attachment #8762146 - Flags: review?(jryans)
Comment on attachment 8761680 [details]
Bug 1273941 - replace uses of promise.defer in devtools/shared;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58770/diff/1-2/
Comment on attachment 8761681 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/shared;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58772/diff/1-2/
Comment on attachment 8761682 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/framework;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58774/diff/1-2/
Comment on attachment 8761683 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/inspector;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58776/diff/1-2/
Comment on attachment 8761684 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/styleeditor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58778/diff/1-2/
Comment on attachment 8761685 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/eyedropper;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58780/diff/1-2/
Comment on attachment 8761686 [details]
Bug 1273941 - do not Cu.import promises in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58782/diff/1-2/
Comment on attachment 8761687 [details]
Bug 1273941 - remove last use of sdk/core/promise from devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58784/diff/1-2/
Comment on attachment 8762146 [details]
Bug 1273941 - change devtools to follow require("promise") convention;

https://reviewboard.mozilla.org/r/59016/#review56026

Now that I see the patch, it seems too hard to balance ESLint's cap rule with the convention.  So, let's drop this patch and close our eyes until we're using real Promise everywhere.

Thanks for giving it a shot anyway!
Attachment #8762146 - Attachment is obsolete: true
Comment on attachment 8761680 [details]
Bug 1273941 - replace uses of promise.defer in devtools/shared;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58770/diff/2-3/
Comment on attachment 8761681 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/shared;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58772/diff/2-3/
Comment on attachment 8761682 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/framework;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58774/diff/2-3/
Comment on attachment 8761683 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/inspector;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58776/diff/2-3/
Comment on attachment 8761684 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/styleeditor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58778/diff/2-3/
Comment on attachment 8761685 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/eyedropper;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58780/diff/2-3/
Comment on attachment 8761686 [details]
Bug 1273941 - do not Cu.import promises in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58782/diff/2-3/
Comment on attachment 8761687 [details]
Bug 1273941 - remove last use of sdk/core/promise from devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58784/diff/2-3/
Iteration: 50.1 → 50.2
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Comment on attachment 8761680 [details]
Bug 1273941 - replace uses of promise.defer in devtools/shared;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58770/diff/3-4/
Comment on attachment 8761681 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/shared;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58772/diff/3-4/
Comment on attachment 8761682 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/framework;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58774/diff/3-4/
Comment on attachment 8761683 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/inspector;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58776/diff/3-4/
Comment on attachment 8761684 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/styleeditor;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58778/diff/3-4/
Comment on attachment 8761685 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/eyedropper;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58780/diff/3-4/
Comment on attachment 8761686 [details]
Bug 1273941 - do not Cu.import promises in devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58782/diff/3-4/
Comment on attachment 8761687 [details]
Bug 1273941 - remove last use of sdk/core/promise from devtools;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58784/diff/3-4/
Rebased.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/51bc72f4ddcc
replace uses of promise.defer in devtools/shared; r=jryans
https://hg.mozilla.org/integration/fx-team/rev/c93918e6463a
replace uses of promise.defer in devtools/client/shared; r=jryans
https://hg.mozilla.org/integration/fx-team/rev/65a02c905ec6
replace uses of promise.defer in devtools/client/framework; r=jryans
https://hg.mozilla.org/integration/fx-team/rev/be8fdffc3665
replace uses of promise.defer in devtools/client/inspector; r=jryans
https://hg.mozilla.org/integration/fx-team/rev/3489bbcc4886
replace uses of promise.defer in devtools/client/styleeditor; r=jryans
https://hg.mozilla.org/integration/fx-team/rev/bdea53872708
replace uses of promise.defer in devtools/client/eyedropper; r=jryans
https://hg.mozilla.org/integration/fx-team/rev/6a0db3b0afcf
do not Cu.import promises in devtools; r=jryans
https://hg.mozilla.org/integration/fx-team/rev/ed1226de3214
remove last use of sdk/core/promise from devtools; r=jryans
Keywords: checkin-needed
Product: Firefox → DevTools
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dce0158ed7c0
Followup: remove leftover comment in browser_styleeditor_sourcemap_watching.js. DONTBUILD
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: