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)

Product:
DevTools
Component:
Framework
Type:
defect

Tracking

(firefox50 fixed)

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

People

(Reporter: jlast, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(9 files, 1 obsolete file)

Bug 1273941 - replace uses of promise.defer in devtools/shared;
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1273941 - replace uses of promise.defer in devtools/client/shared;
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1273941 - replace uses of promise.defer in devtools/client/framework;
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1273941 - replace uses of promise.defer in devtools/client/inspector;
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1273941 - replace uses of promise.defer in devtools/client/styleeditor;
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1273941 - replace uses of promise.defer in devtools/client/eyedropper;
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1273941 - do not Cu.import promises in devtools;
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1273941 - remove last use of sdk/core/promise from devtools;
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1273941 - Followup: remove leftover comment in browser_styleeditor_sourcemap_watching.js.
(deleted), text/x-phabricator-request
Details
There are several places in the client where promise.jsm is used for defer and we should start using promise_defer
Whiteboard: [devtools-html]
Blocks: devtools-html-3
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

Creator:
Created:
Updated:
Size: