If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[ESLint] Fix ESLint issues in devtools/client/framework/{devtools,devtools-browser}.js

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

5 months 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

5 months 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

5 months 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

5 months 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

5 months 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

5 months 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
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.