Enable ESLint for dom/presentation

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
9 months ago
a month ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks 1 bug)

Trunk
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

9 months ago
As part of rolling out ESLint across the tree, I've worked on enabling it for dom/presentation, especially as there's some production code here.

The first patch is the automatic fixes, the second patch are the additional manual fixes.
Comment hidden (mozreview-request)

Updated

9 months ago
Priority: -- → P2
(Assignee)

Updated

9 months ago
Attachment #8991403 - Flags: review?(peterv) → review?(continuation)
Attachment #8991404 - Flags: review?(peterv) → review?(continuation)

Comment 3

9 months ago
mozreview-review
Comment on attachment 8991404 [details]
Bug 1475004 - Enable ESLint for dom/presentation - manual fixes.

https://reviewboard.mozilla.org/r/256294/#review267824

::: dom/presentation/tests/mochitest/PresentationSessionFrameScript.js:63
(Diff revision 1)
>  
>    const Cm = Components.manager;
>  
>    const mockedChannelDescription = {
> -    QueryInterface: ChromeUtils.generateQI(["nsIPresentationChannelDescription"]),
> +    /* eslint-disable-next-line mozilla/use-chromeutils-generateqi */
> +    QueryInterface(iid) {

This change should be reverted. It looks like maybe a rebase issue.

::: dom/presentation/tests/mochitest/PresentationSessionFrameScript.js:94
(Diff revision 1)
>    }
>  
>    const mockedSessionTransport = {
> -    QueryInterface: ChromeUtils.generateQI(["nsIPresentationSessionTransport", "nsIPresentationDataChannelSessionTransportBuilder", "nsIFactory"]),
> +    /* eslint-disable-next-line mozilla/use-chromeutils-generateqi */
> +    QueryInterface(iid) {
> +      const interfaces = [Ci.nsIPresentationSessionTransport,

Likewise.

::: dom/presentation/tests/mochitest/file_presentation_reconnect.html
(Diff revision 1)
>            aResolve();
>          };
>        },
>        function(aError) {
>          ok(false, "Error occurred when establishing a connection: " + aError);
> -        teardown();

Is this removal correct? This file is run from a script tag, along with another script tag that does define teardown(). Would that work? I could certainly believe that this doesn't work given that it is an error case.
Attachment #8991404 - Flags: review?(continuation) → review+

Comment 4

9 months ago
mozreview-review
Comment on attachment 8991403 [details]
Bug 1475004 - Enable ESLint for dom/presentation - automatic fixes.

https://reviewboard.mozilla.org/r/256292/#review267770

::: dom/presentation/provider/PresentationControlService.js:362
(Diff revision 1)
>      }
>    },
>  
>    classID: Components.ID("{f4079b8b-ede5-4b90-a112-5b415a931deb}"),
> -  QueryInterface : ChromeUtils.generateQI([Ci.nsIServerSocketListener,
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsIServerSocketListener,
>                                             Ci.nsIPresentationControlService,

indentation got broken here

::: dom/presentation/tests/xpcshell/test_presentation_device_manager.js
(Diff revision 1)
>  add_test(terminateRequest);
>  add_test(reconnectRequest);
>  add_test(removeDevice);
>  add_test(removeProvider);
> -
> -function run_test() {

This removal looks wrong to me. Isn't this how we actually run the tests? I don't know why this was removed for two of these files and left alone for 3 others.

::: dom/presentation/tests/xpcshell/test_presentation_state_machine.js
(Diff revision 1)
>  add_test(disconnect);
>  add_test(receiverDisconnect);
>  add_test(abnormalDisconnect);
> -
> -function run_test() { // jshint ignore:line
> -  run_next_test();

I think this removal is also wrong. See above.
Attachment #8991403 - Flags: review?(continuation) → review+
(Assignee)

Comment 5

9 months ago
mozreview-review-reply
Comment on attachment 8991403 [details]
Bug 1475004 - Enable ESLint for dom/presentation - automatic fixes.

https://reviewboard.mozilla.org/r/256292/#review267770

> This removal looks wrong to me. Isn't this how we actually run the tests? I don't know why this was removed for two of these files and left alone for 3 others.

The main xpcshell-test infrastructure automatically detects a missing run_test and calls run_next_test automatically, so in these cases the function is superfluous:

https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/testing/xpcshell/head.js#523-530

Generally people are moving towards add_test or add_task, and will run any setup work in an add_task(function setup() {...}), so these are just unnecessary (and tended to have been left behind, which is why ESLint is cleaning them up).
(Assignee)

Comment 6

9 months ago
mozreview-review-reply
Comment on attachment 8991404 [details]
Bug 1475004 - Enable ESLint for dom/presentation - manual fixes.

https://reviewboard.mozilla.org/r/256294/#review267824

> This change should be reverted. It looks like maybe a rebase issue.

I just realised what happened here. The automatic ESLint fixes rewrote this to use ChromeUtils, however running this test locally fails with ChromeUtils being undefined (I guess as this is loaded in a special scope). So in the manual patch, I reverted it.

I'll drop the change from the automatic patch, and just add the eslint comment here.
(Assignee)

Comment 7

9 months ago
(In reply to Andrew McCreight [:mccr8] (away Aug 6 - 10) from comment #3)
> ::: dom/presentation/tests/mochitest/file_presentation_reconnect.html
> (Diff revision 1)
> >            aResolve();
> >          };
> >        },
> >        function(aError) {
> >          ok(false, "Error occurred when establishing a connection: " + aError);
> > -        teardown();
> 
> Is this removal correct? This file is run from a script tag, along with
> another script tag that does define teardown(). Would that work? I could
> certainly believe that this doesn't work given that it is an error case.

Isn't this a iframe or am I reading something wrong? https://searchfox.org/mozilla-central/search?q=file_presentation_reconnect.html&case=false&regexp=false&path=

I've just tried putting teardown() at various places in file_presentation_reconnect.html and it definitely appears to be undefined.

So I think the removal is correct, though it may also be a bug in the test... I can file a follow-up if you wish?
Flags: needinfo?(continuation)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Mark Banner (:standard8) from comment #7)
> So I think the removal is correct, though it may also be a bug in the
> test... I can file a follow-up if you wish?

Sounds good, thanks.
Flags: needinfo?(continuation)
(Assignee)

Comment 11

9 months ago
Filed bug 1480470.

Comment 12

9 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0639e328afde
Enable ESLint for dom/presentation - automatic fixes. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/ee957e55c91e
Enable ESLint for dom/presentation - manual fixes. r=mccr8

Comment 13

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0639e328afde
https://hg.mozilla.org/mozilla-central/rev/ee957e55c91e
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.