1273941 - Convert client promise.jsm defer uses to promise_defer

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P1)

Description

[Jason Laster [:jlast]]

There are several places in the client where promise.jsm is used for defer and we should start using promise_defer

Comment 1

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

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?

Comment 2

[Tom Tromey :tromey]

I think it should be a requirement, to avoid the problems we got into last time this stuff
disappeared.

Comment 3

[Tom Tromey :tromey]

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.

Comment 4

[Tom Tromey :tromey]

(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.

Comment 5

[Tom Tromey :tromey]

Attachment: Bug 1273941 - replace uses of promise.defer in devtools/shared;

Review commit: https://reviewboard.mozilla.org/r/58770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58770/

Comment 6

[Tom Tromey :tromey]

Attachment: Bug 1273941 - replace uses of promise.defer in devtools/client/shared;

Review commit: https://reviewboard.mozilla.org/r/58772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58772/

Comment 7

[Tom Tromey :tromey]

Attachment: Bug 1273941 - replace uses of promise.defer in devtools/client/framework;

Review commit: https://reviewboard.mozilla.org/r/58774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58774/

Comment 8

[Tom Tromey :tromey]

Attachment: Bug 1273941 - replace uses of promise.defer in devtools/client/inspector;

Review commit: https://reviewboard.mozilla.org/r/58776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58776/

Comment 9

[Tom Tromey :tromey]

Attachment: Bug 1273941 - replace uses of promise.defer in devtools/client/styleeditor;

Review commit: https://reviewboard.mozilla.org/r/58778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58778/

Comment 10

[Tom Tromey :tromey]

Attachment: Bug 1273941 - replace uses of promise.defer in devtools/client/eyedropper;

Review commit: https://reviewboard.mozilla.org/r/58780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58780/

Comment 11

[Tom Tromey :tromey]

Attachment: Bug 1273941 - do not Cu.import promises in devtools;

Review commit: https://reviewboard.mozilla.org/r/58782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58782/

Comment 12

[Tom Tromey :tromey]

Attachment: Bug 1273941 - remove last use of sdk/core/promise from devtools;

Review commit: https://reviewboard.mozilla.org/r/58784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58784/

Comment 13

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

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?

Comment 14

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

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 15

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

Comment on attachment 8761682 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/framework;

https://reviewboard.mozilla.org/r/58774/#review55896

Comment 16

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

Comment on attachment 8761683 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/inspector;

https://reviewboard.mozilla.org/r/58776/#review55898

Comment 17

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

Comment on attachment 8761684 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/styleeditor;

https://reviewboard.mozilla.org/r/58778/#review55900

Comment 18

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

Comment on attachment 8761685 [details]
Bug 1273941 - replace uses of promise.defer in devtools/client/eyedropper;

https://reviewboard.mozilla.org/r/58780/#review55904

Comment 19

[Tom Tromey :tromey]

(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.

Comment 20

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

(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 21

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

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).

Comment 22

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

Comment on attachment 8761687 [details]
Bug 1273941 - remove last use of sdk/core/promise from devtools;

https://reviewboard.mozilla.org/r/58784/#review55926

Comment 23

[Tom Tromey :tromey]

(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.

Comment 24

[Tom Tromey :tromey]

(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.

Comment 25

[Tom Tromey :tromey]

(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.

Comment 26

[Tom Tromey :tromey]

(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

Comment 27

[Tom Tromey :tromey]

Attachment: Bug 1273941 - change devtools to follow require("promise") convention;

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/

Comment 28

[Tom Tromey :tromey]

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 29

[Tom Tromey :tromey]

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 30

[Tom Tromey :tromey]

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 31

[Tom Tromey :tromey]

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 32

[Tom Tromey :tromey]

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 33

[Tom Tromey :tromey]

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 34

[Tom Tromey :tromey]

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 35

[Tom Tromey :tromey]

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 36

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

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!

Comment 37

[Tom Tromey :tromey]

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

Comment 38

[Tom Tromey :tromey]

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 39

[Tom Tromey :tromey]

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 40

[Tom Tromey :tromey]

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 41

[Tom Tromey :tromey]

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 42

[Tom Tromey :tromey]

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 43

[Tom Tromey :tromey]

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 44

[Tom Tromey :tromey]

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 45

[Tom Tromey :tromey]

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/

Comment 46

[Tom Tromey :tromey]

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

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 48

[Tom Tromey :tromey]

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 49

[Tom Tromey :tromey]

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 50

[Tom Tromey :tromey]

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 51

[Tom Tromey :tromey]

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 52

[Tom Tromey :tromey]

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 53

[Tom Tromey :tromey]

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 54

[Tom Tromey :tromey]

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 55

[Tom Tromey :tromey]

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/

Comment 56

[Tom Tromey :tromey]

Rebased.

Comment 57

[Pulsebot]

Pushed by [e-mail]:
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

Comment 58

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51bc72f4ddcc
https://hg.mozilla.org/mozilla-central/rev/c93918e6463a
https://hg.mozilla.org/mozilla-central/rev/65a02c905ec6
https://hg.mozilla.org/mozilla-central/rev/be8fdffc3665
https://hg.mozilla.org/mozilla-central/rev/3489bbcc4886
https://hg.mozilla.org/mozilla-central/rev/bdea53872708
https://hg.mozilla.org/mozilla-central/rev/6a0db3b0afcf
https://hg.mozilla.org/mozilla-central/rev/ed1226de3214

Comment 59

[Tim Nguyen :ntim]

Attachment: Bug 1273941 - Followup: remove leftover comment in browser_styleeditor_sourcemap_watching.js.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dce0158ed7c0
Followup: remove leftover comment in browser_styleeditor_sourcemap_watching.js. DONTBUILD

Comment 61

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/dce0158ed7c0