1461785 - Enable ESlint for dom/media/mediasource

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Expand eslint coverage to mediasource directory. This will provide linting on the tests in subdirectories. Linting more media mochitests helps provide us with quicker reviews, more consistent code, and help catch issues in code.

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

Add .eslintrc.js to configure globals from mediasource.js and to add extra
rules to encourage use of let and const over var.

Linting changes:
- Prefer const and let to var. This provides tighter scoping and avoids
  reassignment.
- Mozilla rules do not allow for shadowing of variables, so several tests have
  had promise lists renamed to avoid this.
- Numerous minor fixes to formatting including using double quotes, spacing,
  missing semicolons.
- Remove some unused variables.
- Fix naming of resourcePathSeen in test_MediaSource_memory_reporting.html.

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

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1461785 - Update eslint ignore file to no longer ignore dom/media/mediasource.

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

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8975949 [details]
Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

https://reviewboard.mozilla.org/r/244172/#review250208

Thanks for doing that.
I'm ambiguous on the indentation of the first then() following the declaration of a promise however, I see both style on MDN being used:
like https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises
here we have:
Promise()
.then()
.then()

while here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then
we have:
Promise()
  .then()
  .then()

I'm not sure I like the later, as it feels inconsistent to me. If .then() is indented, then all the ones following should be too as fundamentally, they all return a promise
Promise()
  .then()
    .then()

then it quickly becomes silly.

Could we get a signoff on that first?
maybe on mozilla-dev

::: dom/media/mediasource/test/.eslintrc.js:11
(Diff revision 1)
> +  // Globals from mediasource.js. We use flase to indicate they should not
> +  // be overwritten in scripts.
> +  "globals": {
> +    "addMSEPrefs": false,
> +    "fetchAndLoad": false,
> +    "fetchWithXHR": false,

I think we need to add the Async method added by bug 1280613

::: dom/media/mediasource/test/mediasource.js:5
(Diff revision 1)
>  // Helpers for Media Source Extensions tests
>  
> -var gMSETestPrefs = [
> +let gMSETestPrefs = [
>    [ "media.mediasource.enabled", true ],
> -  ['media.audio-max-decode-error', 0],
> +  ["media.audio-max-decode-error", 0],

could make the spacing consistent while at it...

::: dom/media/mediasource/test/mediasource.js:57
(Diff revision 1)
>  
>    return p;
> -};
> +}
>  
>  function range(start, end) {
> -  var rv = [];
> +  const rv = [];

what's the rationale between the use of const vs let?

here, for example we're adding elements to rv below, seems weird to make that variable "const", though I understand that JS const only means you can't assign another value to rv..

::: dom/media/mediasource/test/test_AVC3_mp4.html:23
(Diff revision 1)
>      ok(true, "Receive a sourceopen event");
> -    var videosb = ms.addSourceBuffer("video/mp4");
> +    const videosb = ms.addSourceBuffer("video/mp4");
>  
> -    fetchAndLoad(videosb, 'avc3/init', [''], '.mp4')
> +    fetchAndLoad(videosb, "avc3/init", [""], ".mp4")
> -    .then(function() {
> +      .then(function() {
> -      var promises = [];
> +        const promises = [];

another const vs let thing ... I find it confusing to declare it const.
maybe I've spent too much time in the c++ world

::: dom/media/mediasource/test/test_ChangeType.html:48
(Diff revision 1)
>      is(el.readyState, el.HAVE_NOTHING, "readyState is HAVE_NOTHING");
> -    var promises = [];
> -    promises.push(fetchAndLoad(videosb, 'bipbop/bipbop', ['init'], '.mp4'));
> -    promises.push(once(el, 'loadedmetadata'));
> -    Promise.all(promises)
> +    const loadedmetadataPromises = [];
> +    loadedmetadataPromises.push(fetchAndLoad(videosb, "bipbop/bipbop", ["init"], ".mp4"));
> +    loadedmetadataPromises.push(once(el, "loadedmetadata"));
> +    Promise.all(loadedmetadataPromises)
> -    .then(function() {
> +      .then(function() {

I find the indenting here confusing, mostly because it's not consistent with the rest.

Just below, none of the .then are indented further.

If .then is indented here, then it would make sense to indent all .then(), and then it starts to look weird...

::: dom/media/mediasource/test/test_ChangeWhileWaitingOnMissingData_mp4.html:20
(Diff revision 1)
>    el.controls = true;
> -  once(ms, 'sourceopen').then(function() {
> +  once(ms, "sourceopen").then(function() {
>      ok(true, "Receive a sourceopen event");
> -    var sb = ms.addSourceBuffer("video/mp4");
> -    fetchAndLoad(sb, 'bipbop/bipbop_480_624kbps-video', ['init'], '.mp4')
> -    .then(fetchAndLoad.bind(null, sb, 'bipbop/bipbop_480_624kbps-video', range(1, 3), '.m4s'))
> +    const sb = ms.addSourceBuffer("video/mp4");
> +    fetchAndLoad(sb, "bipbop/bipbop_480_624kbps-video", ["init"], ".mp4")
> +      .then(fetchAndLoad.bind(null, sb, "bipbop/bipbop_480_624kbps-video", range(1, 3), ".m4s"))

same here.. I find that indenting the first .then to be weird and inconsistent. Especially as .then runs in the same scope as the method above.

What's the styling rule here? do we have one?

Comment 4

[Jean-Yves Avenard [:jya]]

Mark, I added you for review in order to define what the Mozilla JS official style is to be in regards to indentation for things like:

Promise()
.then()
.then()

or is it to be:
Promise()
  .then()
  .then()

or if the above, wouldn't:
Promise()
  .then()
    .then()
be more consistent?

Also, for the use of const vs let

if we do:
const blah = [];
blah.push(...)

Seeing that blah is modified, shouldn't follow more a C++/rust approach and as the variable is modified, to use let instead of const?

Thank you

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975949 [details]
Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

https://reviewboard.mozilla.org/r/244172/#review250208

> I think we need to add the Async method added by bug 1280613

Good call. Will make sure globals are up to date before landing.

> could make the spacing consistent while at it...

We can. Based on this getting through our gloabl rules don't have something for this there, but we can enforce in media code using `array-bracket-spacing`. Do we prefer no spaces? We can also handle slightly more exotic spacings depending: https://eslint.org/docs/rules/array-bracket-spacing

> what's the rationale between the use of const vs let?
> 
> here, for example we're adding elements to rv below, seems weird to make that variable "const", though I understand that JS const only means you can't assign another value to rv..

I originally used let and let the linter enforce const where possible, so all instances of const a determined programatically. I agree that it's a strange usage compared to other languages. We can relax the prefer const rule and allow for let in these cases if that's preferrable, as our global rules don't enforce it.

> I find the indenting here confusing, mostly because it's not consistent with the rest.
> 
> Just below, none of the .then are indented further.
> 
> If .then is indented here, then it would make sense to indent all .then(), and then it starts to look weird...

Agreed. I think some earlier settings I had configured may have auto indented some of these files and some slipped the net. As it stands the rules in place should not enforce a particular indentation, though 2 spaces are mentioned here: https://wiki.mozilla.org/DevTools/CodingStandards

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8975949 [details]
Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

https://reviewboard.mozilla.org/r/244172/#review250430

I've only take a brief look at this patch from the ESLint perspective, but seeing as jya has reviewed it as well, that's fine. There's a few comments/responses below.

::: dom/media/mediasource/test/.eslintrc.js:6
(Diff revision 1)
> +"use strict";
> +
> +module.exports = {
> +  // Extend mochitest rules
> +  "extends": "plugin:mozilla/mochitest-test",
> +  // Globals from mediasource.js. We use flase to indicate they should not

Typo: flase

Also, just to note, I'm hoping at some stage to make handling scripts included in html files easier (bug 1456762), unfortunately we're not quite there yet, so this is a reasonable way of handling it in this case.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8975949 [details]
Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

https://reviewboard.mozilla.org/r/244172/#review250208

> I originally used let and let the linter enforce const where possible, so all instances of const a determined programatically. I agree that it's a strange usage compared to other languages. We can relax the prefer const rule and allow for let in these cases if that's preferrable, as our global rules don't enforce it.

For reference, devtools is also moving towards prefer-const (bug 1454696). Their discussion about it was here: https://github.com/devtools-html/rfcs/issues/44.

I haven't yet started the discussion on if that's something we'd want to do globally or not, though I believe I've seen the odd person do it. I too find it a little strange, but I can see some small advantages as well.

> same here.. I find that indenting the first .then to be weird and inconsistent. Especially as .then runs in the same scope as the method above.
> 
> What's the styling rule here? do we have one?

So we don't really have a style guide for this at the moment, and our ESLint rules don't enforce anything to do with indent. Generally I'm keeping new style rules out of ESLint at the moment, and waiting for a bit ... I'm hoping that when we get clang-format rolled out to c++ files, then we can have a similar effort for javascript, and use something like prettier or something else, and build up the full layout/style then.

Personally I'd go with:

Promise()
  .then()
  .then()
  
Although, these days I'd also be tempted to rewrite the promise chain with async/await which I find a lot cleaner & simpler.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8975950 [details]
Bug 1461785 - Update eslint ignore file to no longer ignore dom/media/mediasource.

https://reviewboard.mozilla.org/r/244174/#review250440

You could just wrap this into the previous changeset, but I'm fine either way.

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975949 [details]
Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

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

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975950 [details]
Bug 1461785 - Update eslint ignore file to no longer ignore dom/media/mediasource.

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

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975949 [details]
Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

https://reviewboard.mozilla.org/r/244172/#review250208

> Agreed. I think some earlier settings I had configured may have auto indented some of these files and some slipped the net. As it stands the rules in place should not enforce a particular indentation, though 2 spaces are mentioned here: https://wiki.mozilla.org/DevTools/CodingStandards

Reverted most of the indentation changes. We can enforce indentation, but I incorrectly though that we had some in our globals when I must have applied some from a temp config I had while working on this.

Comment 12

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975949 [details]
Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

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

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975950 [details]
Bug 1461785 - Update eslint ignore file to no longer ignore dom/media/mediasource.

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

Comment 14

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975949 [details]
Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

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

Comment 15

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975950 [details]
Bug 1461785 - Update eslint ignore file to no longer ignore dom/media/mediasource.

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

Comment 16

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975949 [details]
Bug 1461785 - Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244172/diff/4-5/

Comment 17

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8975950 [details]
Bug 1461785 - Update eslint ignore file to no longer ignore dom/media/mediasource.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244174/diff/4-5/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abc9351a9c15
Update dom/media/mediasource/test to abide eslint rules, add .eslintrc.js. r=jya,standard8
https://hg.mozilla.org/integration/autoland/rev/fdd6d3850e7e
Update eslint ignore file to no longer ignore dom/media/mediasource. r=standard8

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/abc9351a9c15
https://hg.mozilla.org/mozilla-central/rev/fdd6d3850e7e