Convert client promise.jsm defer uses to promise_defer

RESOLVED FIXED in Firefox 50

Status

P1
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: jlast, Assigned: tromey)

Tracking

unspecified
Firefox 50
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(8 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
There are several places in the client where promise.jsm is used for defer and we should start using promise_defer
(Reporter)

Updated

2 years ago
Whiteboard: [devtools-html]
(Reporter)

Updated

2 years ago
Blocks: 1263287

Updated

2 years ago
Flags: qe-verify?

Updated

2 years ago
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?
(Assignee)

Comment 2

2 years ago
I think it should be a requirement, to avoid the problems we got into last time this stuff
disappeared.
Depends on: 1242505

Updated

2 years ago
Assignee: jlaster → nobody
Status: ASSIGNED → NEW
Iteration: 49.2 - May 23 → ---
Priority: P1 → P3
(Assignee)

Comment 3

2 years ago
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

Updated

2 years ago
Iteration: --- → 49.3 - Jun 6
Priority: P3 → P1
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Updated

2 years ago
Depends on: 1277243

Updated

2 years ago
Flags: qe-verify? → qe-verify-

Updated

2 years ago
Iteration: 49.3 - Jun 6 → 50.1
(Assignee)

Comment 5

2 years ago
Created attachment 8761680 [details]
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/
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)
(Assignee)

Comment 6

2 years ago
Created attachment 8761681 [details]
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/
(Assignee)

Comment 7

2 years ago
Created attachment 8761682 [details]
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/
(Assignee)

Comment 8

2 years ago
Created attachment 8761683 [details]
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/
(Assignee)

Comment 9

2 years ago
Created attachment 8761684 [details]
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/
(Assignee)

Comment 10

2 years ago
Created attachment 8761685 [details]
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/
(Assignee)

Comment 11

2 years ago
Created attachment 8761686 [details]
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/
(Assignee)

Comment 12

2 years ago
Created attachment 8761687 [details]
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 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+
Attachment #8761681 - 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+
(Assignee)

Comment 19

2 years ago
(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+
(Assignee)

Comment 23

2 years ago
(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.
(Assignee)

Comment 24

2 years ago
(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.
(Assignee)

Comment 25

2 years ago
(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.
(Assignee)

Comment 26

2 years ago
(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
(Assignee)

Comment 27

2 years ago
Created attachment 8762146 [details]
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/
Attachment #8762146 - Flags: review?(jryans)
(Assignee)

Comment 28

2 years ago
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/
(Assignee)

Comment 29

2 years ago
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/
(Assignee)

Comment 30

2 years ago
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/
(Assignee)

Comment 31

2 years ago
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/
(Assignee)

Comment 32

2 years ago
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/
(Assignee)

Comment 33

2 years ago
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/
(Assignee)

Comment 34

2 years ago
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/
(Assignee)

Comment 35

2 years ago
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/
Attachment #8762146 - Flags: review?(jryans)
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!
(Assignee)

Updated

2 years ago
Attachment #8762146 - Attachment is obsolete: true
(Assignee)

Comment 38

2 years ago
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/
(Assignee)

Comment 39

2 years ago
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/
(Assignee)

Comment 40

2 years ago
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/
(Assignee)

Comment 41

2 years ago
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/
(Assignee)

Comment 42

2 years ago
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/
(Assignee)

Comment 43

2 years ago
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/
(Assignee)

Comment 44

2 years ago
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/
(Assignee)

Comment 45

2 years ago
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/

Updated

2 years ago
Iteration: 50.1 → 50.2
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
(Assignee)

Comment 48

2 years ago
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/
(Assignee)

Comment 49

2 years ago
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/
(Assignee)

Comment 50

2 years ago
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/
(Assignee)

Comment 51

2 years ago
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/
(Assignee)

Comment 52

2 years ago
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/
(Assignee)

Comment 53

2 years ago
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/
(Assignee)

Comment 54

2 years ago
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/
(Assignee)

Comment 55

2 years ago
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/
(Assignee)

Comment 56

2 years ago
Rebased.
Keywords: checkin-needed

Comment 57

2 years ago
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

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.