Enable ESlint for dom/media/mediasource

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bryce, Assigned: bryce)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

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.
Assignee

Updated

a year ago
Assignee: nobody → bvandyk
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
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?
Attachment #8975949 - Flags: review?(jyavenard) → review+
Attachment #8975949 - Flags: review?(standard8)
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
Flags: needinfo?(standard8)
Assignee

Comment 5

a year ago
mozreview-review-reply
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 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.
Attachment #8975949 - Flags: review?(standard8) → review+

Comment 7

a year ago
mozreview-review-reply
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 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.
Attachment #8975950 - Flags: review?(standard8) → review+
Flags: needinfo?(standard8)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: P1 → P3

Comment 18

a year ago
Pushed by bvandyk@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/abc9351a9c15
https://hg.mozilla.org/mozilla-central/rev/fdd6d3850e7e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.