Closed Bug 1386684 Opened 2 years ago Closed 2 years ago

Enable ESLint for toolkit/components/url-classifier

Categories

(Toolkit :: Safe Browsing, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: standard8, Assigned: dbugs, Mentored)

Details

Attachments

(2 files)

As part of rolling out ESLint across the tree, we should enable it for the toolkit/components/url-classifier directory.

I'm happy to mentor this bug. There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Here's some approximate steps:

- In .eslintignore, remove the `toolkit/components/url-classifier/**` line.
- Run eslint:

./mach eslint --fix toolkit/components/url-classifier

This should fix most/all of the issues.

- Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.

- Create a commit of the work so far.

- For the remaining issues, you'll need to fix them by hand, run

./mach eslint toolkit/components/url-classifier

to find/check them

- Create a second commit of the manual changes and push it to mozreview:

http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

If you're not sure of how to fix something, please look at the rules documentation http://eslint.org/docs/rules/ or just ask here.
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: waiting for lock on working directory of /repo/hg/mozilla/try held by process '30618' on host 'hgssh4.dmz.scl3.mozilla.com/effffffc'
remote: got lock after 206 seconds
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: abort: abandoned transaction found!
remote: (run 'hg recover' to clean up transaction)
abort: stream ended unexpectedly (got 0 bytes, expected 4)
Attachment #8893374 - Flags: review?(francois) → review?(hchang)
Attachment #8893375 - Flags: review?(francois) → review?(hchang)
Comment on attachment 8893374 [details]
Bug 1386684 - Enable ESLint for toolkit/components/url-classifier (automatic fixes).

https://reviewboard.mozilla.org/r/164478/#review170964

Looks good except for some alignment issue. I wonder if they can be fixed by the automation tool.

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:351
(Diff revision 2)
> -      let isTableNameV4 = aTableName.endsWith('-proto');
> +      let isTableNameV4 = aTableName.endsWith("-proto");
>        if (0 === this.tableNames.size) {
>          // Decide if this request is v4 by the first added partial hash.
>          this.isV4 = isTableNameV4;
>        } else if (this.isV4 !== isTableNameV4) {
>          log('ERROR: Cannot mix "proto" tables with other types within ' +

Not sure if this line should be 

"ERROR: Cannot mix \"proto\" tables with other types within " 

according to eslint rules.

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:566
(Diff revision 2)
>    handleResponseV4: function HCR_handleResponseV4() {
>      let callback = {
>        // onCompleteHashFound will be called for each fullhash found in
>        // FullHashResponse.
> -      onCompleteHashFound : (aCompleteHash,
> +      onCompleteHashFound: (aCompleteHash,
>                               aTableNames,

nit: alignment

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:595
(Diff revision 2)
>  
>        // onResponseParsed will be called no matter if there is match in
>        // FullHashResponse, the callback is mainly used to pass negative cache
>        // duration and minimum wait duration.
> -      onResponseParsed : (aMinWaitDuration,
> +      onResponseParsed: (aMinWaitDuration,
>                            aNegCacheDuration) => {

nit: alignment

::: toolkit/components/url-classifier/nsUrlClassifierListManager.js:104
(Diff revision 2)
>      this.needsUpdate_[updateUrl] = {};
>  
>      // Using the V4 backoff algorithm for both V2 and V4. See bug 1273398.
>      this.requestBackoffs_[updateUrl] = new RequestBackoffV4(
>                                              4 /* num requests */,
> -                                   60*60*1000 /* request time, 60 min */);
> +                                   60 * 60 * 1000 /* request time, 60 min */);

nit: alignment

::: toolkit/components/url-classifier/nsUrlClassifierListManager.js:216
(Diff revision 2)
>  
>  /**
>   *  Set timer to check update after delay
>   */
> -PROT_ListManager.prototype.setUpdateCheckTimer = function (updateUrl,
> -                                                           delay)
> +PROT_ListManager.prototype.setUpdateCheckTimer = function(updateUrl,
> +                                                           delay) {

nit: alignment

::: toolkit/components/url-classifier/nsUrlClassifierListManager.js:377
(Diff revision 2)
> -    let isCurTableProto = tableName.endsWith('-proto');
> +    let isCurTableProto = tableName.endsWith("-proto");
>      if (!onceThru) {
>        useProtobuf = isCurTableProto;
>        onceThru = true;
>      } else if (useProtobuf !== isCurTableProto) {
>        log('ERROR: Cannot mix "proto" tables with other types ' +

Not sure if we should use

"ERROR: Cannot mix \"proto\" tables with other types "

::: toolkit/components/url-classifier/tests/mochitest/classifiedAnnotatedFrame.html:160
(Diff revision 2)
>  newScript.id = "badscript2";
>  newScript.src = "http://tracking.example.com/tests/toolkit/components/url-classifier/tests/mochitest/evil.js";
> -newScript.addEventListener("load", function() {scriptItem2 = scriptItem;});
> +newScript.addEventListener("load", function() { scriptItem2 = scriptItem; });
>  document.body.appendChild(newScript);
>  
> -/// Try loading from a tracking image URI (2)
> +// / Try loading from a tracking image URI (2)

Should be just

// Try loading from a tracking image URI (2)

::: toolkit/components/url-classifier/tests/mochitest/test_allowlisted_annotations.html:37
(Diff revision 2)
>                                     { url: "https://allowlisted.example.com" }));
>  }
>  
>  SpecialPowers.pushPrefEnv(
> -  {"set" : [["urlclassifier.trackingTable", "test-track-simple"],
> +  {"set": [["urlclassifier.trackingTable", "test-track-simple"],
>              ["privacy.trackingprotection.enabled", true],

nit: alignment

::: toolkit/components/url-classifier/tests/mochitest/test_classifier.html:162
(Diff revision 2)
>    });
>  }
>  
>  SpecialPowers.pushPrefEnv(
> -  {"set" : [["urlclassifier.malwareTable", "test-malware-simple,test-unwanted-simple"],
> +  {"set": [["urlclassifier.malwareTable", "test-malware-simple,test-unwanted-simple"],
>              ["urlclassifier.phishTable", "test-phish-simple"],

nit: alignment

::: toolkit/components/url-classifier/tests/mochitest/test_classifier_changetablepref.html:98
(Diff revision 2)
>  }
>  
>  // Set nextupdatetime to 1 to trigger an update
>  SpecialPowers.pushPrefEnv(
> -  {"set" : [["privacy.trackingprotection.enabled", true],
> +  {"set": [["privacy.trackingprotection.enabled", true],
>              ["channelclassifier.allowlist_example", true],

nit: alignment

::: toolkit/components/url-classifier/tests/mochitest/test_classifier_worker.html:56
(Diff revision 2)
>    SimpleTest.finish();
> -};
> +}
>  
>  SpecialPowers.pushPrefEnv(
> -  {"set" : [["urlclassifier.malwareTable", "test-malware-simple,test-unwanted-simple"],
> +  {"set": [["urlclassifier.malwareTable", "test-malware-simple,test-unwanted-simple"],
>              ["urlclassifier.phishTable", "test-phish-simple"]]},

nit: alignment

::: toolkit/components/url-classifier/tests/mochitest/test_reporturl.html:201
(Diff revision 2)
>  
>  ];
>  
>  SpecialPowers.pushPrefEnv(
> -  {"set" : [["browser.safebrowsing.provider.google.reportPhishMistakeURL", BASE_URL + "action=reporturl&reporturl="],
> +  {"set": [["browser.safebrowsing.provider.google.reportPhishMistakeURL", BASE_URL + "action=reporturl&reporturl="],
>              ["browser.safebrowsing.provider.google4.reportPhishMistakeURL", BASE_URL + "action=reporturl&reporturl="],

nit: alignment

::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_bug1157081.html:74
(Diff revision 2)
>    is(win.document.getElementById("badscript").dataset.touched, aBlocked ? "no" : "yes", "Should not load tracking javascript");
>  }
>  
>  SpecialPowers.pushPrefEnv(
> -  {"set" : [["urlclassifier.trackingTable", "test-track-simple"],
> +  {"set": [["urlclassifier.trackingTable", "test-track-simple"],
>              ["privacy.trackingprotection.enabled", true],

nit: alignment

::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_bug1312515.html:107
(Diff revision 2)
>    SimpleTest.finish();
>  }
>  
>  SpecialPowers.pushPrefEnv(
> -  {"set" : [["urlclassifier.trackingTable", "test-track-simple"],
> +  {"set": [["urlclassifier.trackingTable", "test-track-simple"],
>              ["privacy.trackingprotection.annotate_channels", true],

nit: alignment

::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_whitelist.html:114
(Diff revision 2)
>    is(allNodeMatch, true, "All tracking nodes are expected to be annotated as such");
>  }
>  
>  SpecialPowers.pushPrefEnv(
> -  {"set" : [["privacy.trackingprotection.enabled", true],
> +  {"set": [["privacy.trackingprotection.enabled", true],
>              ["channelclassifier.allowlist_example", true]]},

nit: alignment
Attachment #8893374 - Flags: review?(hchang) → review+
There seems to be a permanent timeout:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=552c402a937e

test_classifier_changetablepref.html
Comment on attachment 8893375 [details]
Bug 1386684 - Enable ESLint for toolkit/components/url-classifier (manual fixes).

https://reviewboard.mozilla.org/r/164480/#review171096

A few potential bugs are fixed by this patch! Thanks :) 

Everything seems good except for some alignment nits.

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js:802
(Diff revision 2)
>    observe: function HCR_observe(aSubject, aTopic, aData) {
>      if (aTopic == "quit-application") {
>        this._shuttingDown = true;
>        if (this._channel) {
>          this._channel.cancel(Cr.NS_ERROR_ABORT);
> -        telemetryClockStart = 0;
> +        this.telemetryClockStart = 0;

Wow a potential bug is fixed!!

::: toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html:86
(Diff revision 2)
>  
>  // Reset function in helper try to simulate the behavior we restart firefox
>  function reset() {
>    return classifierHelper.resetDatabase()
>      .catch(err => {
> -      ok(false, "Couldn't update classifier. Error code: " + errorCode);
> +      ok(false, "Couldn't update classifier. Error code: " + err);

Wow another potential bug is fixed!

::: toolkit/components/url-classifier/tests/mochitest/test_bug1254766.html:125
(Diff revision 2)
>      { url: UNWANTED_HOST2, db: UNWANTED_LIST, len: length }
>    ];
>  
>    return classifierHelper.addUrlToDB(testData)
>      .catch(err => {
> -      ok(false, "Couldn't update classifier. Error code: " + errorCode);
> +      ok(false, "Couldn't update classifier. Error code: " + err);

Yet another bug is fixed :)

::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_bug1157081.html:38
(Diff revision 2)
> -
>  function testOnWindow(aCallback) {
>    var win = mainWindow.OpenBrowserWindow();
>    win.addEventListener("load", function() {
> -    whenDelayedStartupFinished(win, function() {
> +    TestUtils.topicObserved("browser-delayed-startup-finished",
> +                                  subject => subject == win).then(() => {

nit: alignment

::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_bug1312515.html:38
(Diff revision 2)
> -
>  function testOnWindow(aPrivate, aCallback) {
>    var win = mainWindow.OpenBrowserWindow({private: aPrivate});
>    win.addEventListener("load", function() {
> -    whenDelayedStartupFinished(win, function() {
> +    TestUtils.topicObserved("browser-delayed-startup-finished",
> +                                  subject => subject == win).then(() => {

nit: alignment

::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_whitelist.html:39
(Diff revision 2)
> -
>  function testOnWindow(contentPage, aCallback) {
>    var win = mainWindow.OpenBrowserWindow();
>    win.addEventListener("load", function() {
> -    whenDelayedStartupFinished(win, function() {
> +    TestUtils.topicObserved("browser-delayed-startup-finished",
> +                                  subject => subject == win).then(() => {

nit: alignment

::: toolkit/components/url-classifier/tests/unit/test_hashcompleter.js:194
(Diff revision 2)
>      let hash;
>      let prefix;
>      do {
>        hash = "";
>        let length = 1 + rand.nextNum(5);
> -      for (let i = 0; i < length; i++)
> +      for (let j = 0; j < length; j++)

Fixed a potential bug again!
Attachment #8893375 - Flags: review?(hchang) → review+
Comment on attachment 8893374 [details]
Bug 1386684 - Enable ESLint for toolkit/components/url-classifier (automatic fixes).

https://reviewboard.mozilla.org/r/164478/#review170964

> Not sure if this line should be 
> 
> "ERROR: Cannot mix \"proto\" tables with other types within " 
> 
> according to eslint rules.

The eslint rules allow the use of single quotes to avoid escaping
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4148c51d43fa
Enable ESLint for toolkit/components/url-classifier (automatic fixes). r=hchang
https://hg.mozilla.org/integration/autoland/rev/ff12e5530767
Enable ESLint for toolkit/components/url-classifier (manual fixes). r=hchang
https://hg.mozilla.org/mozilla-central/rev/4148c51d43fa
https://hg.mozilla.org/mozilla-central/rev/ff12e5530767
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.