1376357 - Enable ESLint for dom/media/*.js

Status: RESOLVED FIXED

(Core :: WebRTC, enhancement, P2)

Description

[Mark Banner (:standard8)]

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 1

[Mark Banner (:standard8)]

Attachment: Bug 1376357 - Drop use of non-standard catch-if in dom/media/*.js.

Review commit: https://reviewboard.mozilla.org/r/152510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152510/

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1376357 - Automatically fix ESLint issues in dom/media/*.js*.

Review commit: https://reviewboard.mozilla.org/r/152512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152512/

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1376357 - Fix remaining ESLint issues in dom/media/*.js*.

Review commit: https://reviewboard.mozilla.org/r/152514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152514/

Comment 4

[Mark Banner (:standard8)]

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.

Comment 5

[Byron Campen [:bwc]]

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.

Comment 6

[Byron Campen [:bwc]]

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

https://reviewboard.mozilla.org/r/152512/#review158050

Comment 7

[Byron Campen [:bwc]]

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.

Comment 8

[Mark Banner (:standard8)]

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 9

[Mark Banner (:standard8)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152510/diff/1-2/

Comment 10

[Mark Banner (:standard8)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152514/diff/1-2/

Comment 11

[Mark Banner (:standard8)]

Attachment: Bug 1376357 - Automatically fix ESLint issues in dom/media/*.js*.

Review commit: https://reviewboard.mozilla.org/r/152970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152970/

Comment 12

[Mark Banner (:standard8)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152514/diff/2-3/

Comment 13

[Byron Campen [:bwc]]

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

https://reviewboard.mozilla.org/r/152970/#review158262

Comment 14

[Byron Campen [:bwc]]

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

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27aa42ed3235
https://hg.mozilla.org/mozilla-central/rev/61a73c8436a7
https://hg.mozilla.org/mozilla-central/rev/5206e8f59b66