Closed
Bug 1360237
Opened 4 years ago
Closed 4 years ago
[ESLint] Fix ESLint issues in devtools/client/framework/{devtools,devtools-browser}.js
Categories
(DevTools :: General, enhancement)
DevTools
General
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
Details
Attachments
(3 files)
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•4 years ago
|
||
mozreview-review |
Comment on attachment 8862476 [details] Bug 1360237 - Fix eslint issues for devtools/client/framework/devtools.js. https://reviewboard.mozilla.org/r/134392/#review137342
Attachment #8862476 -
Flags: review?(pbrosset) → review+
Comment 5•4 years ago
|
||
mozreview-review |
Comment on attachment 8862477 [details] Bug 1360237 - Make gDevToolsBrowser.inspectNode more readable by using async/await. https://reviewboard.mozilla.org/r/134394/#review137346 I have not read the dev-platform thread about using await/async, but this looks good to me. What was the outcome of the discussion? Can we use those more generally in our code now?
Attachment #8862477 -
Flags: review?(pbrosset) → review+
Comment 6•4 years ago
|
||
mozreview-review |
Comment on attachment 8862478 [details] Bug 1360237 - Fix eslint issues for devtools/client/framework/devtools-browser.js. https://reviewboard.mozilla.org/r/134396/#review137348 Seems good to me. Since the files you changed in this series of commits are not clean, you should also make sure future eslint errors are caught in CI, but removing exceptions for these files in .eslintignore. ::: devtools/client/framework/devtools-browser.js:257 (Diff revision 1) > - (toolId == "webconsole" && toolbox.splitConsole))) > + (toolId == "webconsole" && toolbox.splitConsole))) { > - { > toolbox.fireCustomKey(toolId); > > - if (toolDefinition.preventClosingOnKey || toolbox.hostType == Toolbox.HostType.WINDOW) { > + if (toolDefinition.preventClosingOnKey || > + toolbox.hostType == Toolbox.HostType.WINDOW) { nit: I think we tend to vertically align conditions like this: ``` if (toolDefinition.preventClosingOnKey || toolbox.hostType == Toolbox.HostType.WINDOW) { ```
Attachment #8862478 -
Flags: review?(pbrosset) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•4 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #5) > Comment on attachment 8862477 [details] > Bug 1360237 - Makes gDevToolsBrowser.inspectNode more readable by using > async/await. > > https://reviewboard.mozilla.org/r/134394/#review137346 > > I have not read the dev-platform thread about using await/async, but this > looks good to me. What was the outcome of the discussion? Can we use those > more generally in our code now? It isn't really clear to me, but as I started receiving pathes with await/async from jryans (who's watching everyhing), I supposed we had a green light. But it is very confusing if you see these two messages: https://mail.mozilla.org/pipermail/firefox-dev/2017-March/005235.html https://mail.mozilla.org/pipermail/firefox-dev/2017-March/005233.html https://groups.google.com/forum/#!topic/mozilla.dev.platform/4--twpqTrMY Posted at about the same time. And it seems to be used in very various places: http://searchfox.org/mozilla-central/search?q=async+function&path= But when I see that: http://searchfox.org/mozilla-central/source/js/src/frontend/Parser.h#828-836 bool asyncIterationSupported() { #ifdef RELEASE_OR_BETA return false; #else I tend to think all this code using async/await is going to start throwing on merge to beta?! So, I think I will revert back to Task.
As posted in #devtools, async iteration is not the same as async / await. As far as I know, async / await was enabled for all channels in 52 (bug 1185106). Also, Firefox team is planning to rewrite the tree to async / await (bug 1353542).
Comment hidden (mozreview-request) |
Comment 13•4 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ac07cdd492c3 Fix eslint issues for devtools/client/framework/devtools.js. r=pbro https://hg.mozilla.org/integration/autoland/rev/30ea0bb33e98 Make gDevToolsBrowser.inspectNode more readable by using async/await. r=pbro https://hg.mozilla.org/integration/autoland/rev/a55f523231a2 Fix eslint issues for devtools/client/framework/devtools-browser.js. r=pbro
Comment 14•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac07cdd492c3 https://hg.mozilla.org/mozilla-central/rev/30ea0bb33e98 https://hg.mozilla.org/mozilla-central/rev/a55f523231a2
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•