Closed
Bug 1273941
Opened 9 years ago
Closed 8 years ago
Convert client promise.jsm defer uses to promise_defer
Categories
(DevTools :: Framework, defect, P1)
DevTools
Framework
Tracking
(firefox50 fixed)
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
Reporter | ||
Updated•9 years ago
|
Whiteboard: [devtools-html]
Reporter | ||
Updated•9 years ago
|
Blocks: devtools-html-3
Updated•9 years ago
|
Flags: qe-verify?
Updated•9 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•9 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•9 years ago
|
Assignee: jlaster → nobody
Status: ASSIGNED → NEW
Iteration: 49.2 - May 23 → ---
Priority: P1 → P3
Assignee | ||
Comment 3•9 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.
Updated•9 years ago
|
Iteration: --- → 49.3 - Jun 6
Priority: P3 → P1
Assignee | ||
Comment 4•9 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.
Updated•9 years ago
|
Flags: qe-verify? → qe-verify-
Updated•8 years ago
|
Iteration: 49.3 - Jun 6 → 50.1
Assignee | ||
Comment 5•8 years ago
|
||
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•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58772/
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58774/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58776/
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58778/
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58780/
Assignee | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/58782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58782/
Assignee | ||
Comment 12•8 years ago
|
||
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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
Attachment #8762146 -
Attachment is obsolete: true
Assignee | ||
Comment 37•8 years ago
|
||
Assignee | ||
Comment 38•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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/
Assignee | ||
Comment 46•8 years ago
|
||
Updated•8 years ago
|
Iteration: 50.1 → 50.2
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 48•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 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/
Comment 57•8 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
Comment 58•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 59•4 years ago
|
||
Comment 60•4 years ago
|
||
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
Comment 61•4 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•