Enable ESLint for dom/media/*.js

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Chatting to Nils about ESLint, we discussed about enabling ESLint on dom/media. It turns out the tests will need a little work, but we can fairly easily enable ESLint on the main "production" files, especially PeerConnection.js.

Byron: if this is going to conflict with your existing work too much, let me know, I'm happy to put it off for a while.
Comment hidden (mozreview-request)
Oh I forgot to mention, the default set of rules getting enabled here are the ones defined in:

https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js

This is the configuration that we're spreading throughout the tree, we'll be adding more to it as we go along.
Rank: 25
Priority: -- → P2

Comment 5

2 years ago
mozreview-review
Comment on attachment 8881322 [details]
Bug 1376357 - Drop use of non-standard catch-if in dom/media/*.js.

https://reviewboard.mozilla.org/r/152510/#review158044

::: dom/media/IdpSandbox.jsm:176
(Diff revision 1)
> -  } catch (e if (typeof e.result !== 'undefined' &&
> -                 e.result === Cr.NS_ERROR_MALFORMED_URI)) {
> +  } catch (e) {
> +    if (typeof e.result !== 'undefined' &&
> +                   e.result === Cr.NS_ERROR_MALFORMED_URI) {
> -    throw new Error(message + 'must produce a valid URI');
> +      throw new Error(message + 'must produce a valid URI');
> -  }
> +    }
> +  }

Don't we want to throw e here, to keep the behavior the same?

::: dom/media/PeerConnection.js:630
(Diff revision 1)
> -      } catch (e if (e.result == Cr.NS_ERROR_MALFORMED_URI)) {
> +      } catch (e) {
> +        if (e.result == Cr.NS_ERROR_MALFORMED_URI) {
> -        throw new this._win.DOMException(msg + " - malformed URI: " + uriStr,
> +          throw new this._win.DOMException(msg + " - malformed URI: " + uriStr,
> -                                         "SyntaxError");
> +                                           "SyntaxError");
> -      }
> +        }
> +      }

Same question here.
Attachment #8881322 - Flags: review?(docfaraday) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8881323 [details]
Bug 1376357 - Automatically fix ESLint issues in dom/media/*.js*.

https://reviewboard.mozilla.org/r/152512/#review158050
Attachment #8881323 - Flags: review?(docfaraday) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8881324 [details]
Bug 1376357 - Fix remaining ESLint issues in dom/media/*.js*.

https://reviewboard.mozilla.org/r/152514/#review158054

I had some questions about an earlier patch that will impact this.
Attachment #8881324 - Flags: review?(docfaraday)
Assignee

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8881322 [details]
Bug 1376357 - Drop use of non-standard catch-if in dom/media/*.js.

https://reviewboard.mozilla.org/r/152510/#review158044

> Don't we want to throw e here, to keep the behavior the same?

Yes, nice catch. I missed that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8881323 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8881911 [details]
Bug 1376357 - Automatically fix ESLint issues in dom/media/*.js*.

https://reviewboard.mozilla.org/r/152970/#review158262
Attachment #8881911 - Flags: review?(docfaraday) → review+

Comment 14

2 years ago
mozreview-review
Comment on attachment 8881324 [details]
Bug 1376357 - Fix remaining ESLint issues in dom/media/*.js*.

https://reviewboard.mozilla.org/r/152514/#review158264
Attachment #8881324 - Flags: review?(docfaraday) → review+

Comment 15

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27aa42ed3235
Drop use of non-standard catch-if in dom/media/*.js. r=bwc
https://hg.mozilla.org/integration/autoland/rev/61a73c8436a7
Automatically fix ESLint issues in dom/media/*.js*. r=bwc
https://hg.mozilla.org/integration/autoland/rev/5206e8f59b66
Fix remaining ESLint issues in dom/media/*.js*. r=bwc

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/27aa42ed3235
https://hg.mozilla.org/mozilla-central/rev/61a73c8436a7
https://hg.mozilla.org/mozilla-central/rev/5206e8f59b66
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.