Closed Bug 1360237 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: General, enhancement)

Product:
DevTools
Component:
General
Type:
enhancement
Priority:
Not set

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

Details

Attachments

(3 files)

Bug 1360237 - Fix eslint issues for devtools/client/framework/devtools.js.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1360237 - Make gDevToolsBrowser.inspectNode more readable by using async/await.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1360237 - Fix eslint issues for devtools/client/framework/devtools-browser.js.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
No description provided.
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 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 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+
(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).
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.