Closed
Bug 1376357
Opened 6 years ago
Closed 6 years ago
Enable ESLint for dom/media/*.js
Categories
(Core :: WebRTC, enhancement, P2)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
Details
Attachments
(3 files, 1 obsolete file)
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) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
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.
Updated•6 years ago
|
Rank: 25
Priority: -- → P2
Comment 5•6 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•6 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•6 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•6 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) |
Assignee | ||
Updated•6 years ago
|
Attachment #8881323 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•6 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•6 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•6 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•6 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
Closed: 6 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•