Closed Bug 1376357 Opened 6 years ago Closed 6 years ago

Enable ESLint for dom/media/*.js

Categories

(Core :: WebRTC, enhancement, P2)

Product:
Core
Component:
WebRTC
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: standard8, Assigned: standard8)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1376357 - Drop use of non-standard catch-if in dom/media/*.js.
59 bytes, text/x-review-board-request
bwc
: review+
Details
Bug 1376357 - Fix remaining ESLint issues in dom/media/*.js*.
59 bytes, text/x-review-board-request
bwc
: review+
Details
Bug 1376357 - Automatically fix ESLint issues in dom/media/*.js*.
59 bytes, text/x-review-board-request
bwc
: review+
Details 
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.
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 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 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 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)
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.
Attachment #8881323 - Attachment is obsolete: true
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 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+
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
You need to log in before you can comment on or make changes to this bug.