1311043 - Use eslint more aggressively in browser/components/migration/

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1311043 - make eslint much more strict in browser/components/migration/, and fix issues,

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

Comment 2

[:Gijs (he/him)]

This depends on the fix from bug 1310596, after which we'll need to add an /* globals OS */ annotation next to the import of osfile.jsm . With that, there are 0 remaining eslint issues in browser/components/migration with the rules this enables (which have been mostly copied from toolkit/components/narrate/ ).

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8802142 [details]
Bug 1311043 - make eslint much more strict in browser/components/migration/, and fix issues,

https://reviewboard.mozilla.org/r/86650/#review85596

Big patch! I've put a few comments in there, but I'm only a bit scared about the one in MigrationUtils.jsm. It's up to your consideration which comments you decide to address in your final patch.

::: browser/components/migration/360seProfileMigrator.js:314
(Diff revision 1)
>    });
>    if (!bookmarksPaths.length) {
>      return Promise.resolve(new Date(0));
>    }
>    let datePromises = bookmarksPaths.map(path => {
> -    return OS.File.stat(path).catch(_ => null).then(info => {
> +    return OS.File.stat(path).catch(() => null).then(info => {

nit: for an empty lambda, I find `() => {}` more descriptive. But hmmkay.

::: browser/components/migration/ChromeProfileMigrator.js:219
(Diff revision 1)
>      prefsFile.append("Preferences");
>      if (prefsFile.exists()) {
>        // XXX reading and parsing JSON is synchronous.
>        let fstream = Cc[FILE_INPUT_STREAM_CID].
>                      createInstance(Ci.nsIFileInputStream);
> -      fstream.init(file, -1, 0, 0);
> +      fstream.init(prefsFile, -1, 0, 0);

Woah, nice catch.

::: browser/components/migration/ChromeProfileMigrator.js:254
(Diff revision 1)
>  
>      migrate: function(aCallback) {
>        return Task.spawn(function* () {
>          let gotErrors = false;
> -        let errorGatherer = () => gotErrors = true;
> -        let jsonStream = yield new Promise(resolve =>
> +        let errorGatherer = function() { gotErrors = true };
> +        let jsonStream = yield new Promise((resolve, reject) => {

Woa, nice find, again.

::: browser/components/migration/ChromeProfileMigrator.js:361
(Diff revision 1)
>                }
>              });
>            });
>          }
> -      }).then(() => { aCallback(true); },
> +      }).then(() => { aCallback(true) },
>                ex => {

Why not have a catch-clause anyway? This only calls the callback on a proper `reject()`, but not throws. But prolly out-of-scope for this bug.

::: browser/components/migration/IEProfileMigrator.js:453
(Diff revision 1)
>        return;
>  
>      if (aTransformFn)
>        value = aTransformFn(value);
>  
> -    switch (typeof(value)) {
> +    switch (typeof (value)) {

...or not use brackets at all?

::: browser/components/migration/MigrationUtils.jsm:679
(Diff revision 1)
>      };
>  
>      let browserDesc = "";
>      let key = "";
>      try {
> -      let browserDesc =
> +      browserDesc =

strict-mode violation! Did this ever work?

::: browser/components/migration/MigrationUtils.jsm:680
(Diff revision 1)
>  
>      let browserDesc = "";
>      let key = "";
>      try {
> -      let browserDesc =
> +      browserDesc =
>          Cc["@mozilla.org/uriloader/external-protocol-service;1"].

nit: while you're here, can you align this Cc-incantation properly?

::: browser/components/migration/tests/unit/test_Edge_db_migration.js:43
(Diff revision 1)
>    wrapper.column.coltyp = type;
>    let fallback = 0;
>    switch (type) {
>      case COLUMN_TYPES.JET_coltypText:
>        fallback = 255;
> +      // intentional fall through

Nit: `// Intentional fall-through.`

::: browser/components/migration/tests/unit/test_fx_telemetry.js:99
(Diff revision 1)
>                   .createInstance(Ci.nsISupports)
>                   .wrappedJSObject;
>    let migrators = migrator._getResourcesInternal(srcDir, targetDir);
>    for (let m of migrators) {
>      if (m.name == name) {
> -      return new Promise((resolve, reject) => {
> +      return new Promise(resolve => {

nit: might as well make this a one-liner.

Comment 4

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Comment on attachment 8802142 [details]
> Bug 1311043 - make eslint much more strict in browser/components/migration/,
> and fix issues,
> 
> https://reviewboard.mozilla.org/r/86650/#review85596
> 
> Big patch! I've put a few comments in there, but I'm only a bit scared about
> the one in MigrationUtils.jsm. It's up to your consideration which comments
> you decide to address in your final patch.
> 
> ::: browser/components/migration/360seProfileMigrator.js:314
> (Diff revision 1)
> >    });
> >    if (!bookmarksPaths.length) {
> >      return Promise.resolve(new Date(0));
> >    }
> >    let datePromises = bookmarksPaths.map(path => {
> > -    return OS.File.stat(path).catch(_ => null).then(info => {
> > +    return OS.File.stat(path).catch(() => null).then(info => {
> 
> nit: for an empty lambda, I find `() => {}` more descriptive. But hmmkay.
> 
> ::: browser/components/migration/ChromeProfileMigrator.js:219
> (Diff revision 1)
> >      prefsFile.append("Preferences");
> >      if (prefsFile.exists()) {
> >        // XXX reading and parsing JSON is synchronous.
> >        let fstream = Cc[FILE_INPUT_STREAM_CID].
> >                      createInstance(Ci.nsIFileInputStream);
> > -      fstream.init(file, -1, 0, 0);
> > +      fstream.init(prefsFile, -1, 0, 0);
> 
> Woah, nice catch.
> 
> ::: browser/components/migration/ChromeProfileMigrator.js:254
> (Diff revision 1)
> >  
> >      migrate: function(aCallback) {
> >        return Task.spawn(function* () {
> >          let gotErrors = false;
> > -        let errorGatherer = () => gotErrors = true;
> > -        let jsonStream = yield new Promise(resolve =>
> > +        let errorGatherer = function() { gotErrors = true };
> > +        let jsonStream = yield new Promise((resolve, reject) => {
> 
> Woa, nice find, again.
> 
> ::: browser/components/migration/ChromeProfileMigrator.js:361
> (Diff revision 1)
> >                }
> >              });
> >            });
> >          }
> > -      }).then(() => { aCallback(true); },
> > +      }).then(() => { aCallback(true) },
> >                ex => {
> 
> Why not have a catch-clause anyway? This only calls the callback on a proper
> `reject()`, but not throws. But prolly out-of-scope for this bug.

Not sure I understand. The second handler here is the "bar" of  a .then(foo, bar); on the:

      Task.spawn(function* () {

on line 315.

I believe that because it's a Task, it'll catch a throw, and invoke the reject handler for the task, so this will be invoked for throws. Unless you mean you want a catch clause for the invocation of `aCallback(true)` ? In that case, because it's a consumer-provided callback, I don't think that would be a correct 'division of labour', as it were. It's the consumer's responsibility to ensure the callback doesn't throw. Or did you mean something else still?

> ::: browser/components/migration/IEProfileMigrator.js:453
> (Diff revision 1)
> >        return;
> >  
> >      if (aTransformFn)
> >        value = aTransformFn(value);
> >  
> > -    switch (typeof(value)) {
> > +    switch (typeof (value)) {
> 
> ...or not use brackets at all?
> 
> ::: browser/components/migration/MigrationUtils.jsm:679
> (Diff revision 1)
> >      };
> >  
> >      let browserDesc = "";
> >      let key = "";
> >      try {
> > -      let browserDesc =
> > +      browserDesc =
> 
> strict-mode violation! Did this ever work?

I'm not sure what you mean. This was "only" shadowing the outer variable.

let foo = "bar";
try {
  let foo = "baz";
  doStuffWith(foo);
} catch (e) {}


is legal in both strict and non-strict mode, I think? There's just a useless `foo` in the parent. An alternative fix here would be to kill off the outer `foo`, actually, because it's not used anywhere else. Let me know if you would prefer that.

> ::: browser/components/migration/MigrationUtils.jsm:680
> (Diff revision 1)
> >  
> >      let browserDesc = "";
> >      let key = "";
> >      try {
> > -      let browserDesc =
> > +      browserDesc =
> >          Cc["@mozilla.org/uriloader/external-protocol-service;1"].
> 
> nit: while you're here, can you align this Cc-incantation properly?

Can you clarify exactly what you mean by 'properly' so I get it right? :-)



(I'm replying on here rather than on reviewboard because I can't set needinfo otherwise, and you've already r+'d so I also can't re-request review through reviewboard)

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #4)
> Not sure I understand. The second handler here is the "bar" of  a .then(foo,
> bar); on the:
> 
>       Task.spawn(function* () {
> 
> on line 315.
> 
> I believe that because it's a Task, it'll catch a throw, and invoke the
> reject handler for the task, so this will be invoked for throws. Unless you
> mean you want a catch clause for the invocation of `aCallback(true)` ? In
> that case, because it's a consumer-provided callback, I don't think that
> would be a correct 'division of labour', as it were. It's the consumer's
> responsibility to ensure the callback doesn't throw. Or did you mean
> something else still?

So, yes, `.catch()` would get throws from the `then(resolve => {...})` handler as well. But that doesn't help the situation more than it needs to here, so you can ignore my comment.

> I'm not sure what you mean. This was "only" shadowing the outer variable.
> 
> let foo = "bar";
> try {
>   let foo = "baz";
>   doStuffWith(foo);
> } catch (e) {}
> 
> 
> is legal in both strict and non-strict mode, I think? There's just a useless
> `foo` in the parent. An alternative fix here would be to kill off the outer
> `foo`, actually, because it's not used anywhere else. Let me know if you
> would prefer that.

Yeah, I'd prefer that! This was confusing me enough to see a strict-mode violation where there wasn't.

> Can you clarify exactly what you mean by 'properly' so I get it right? :-)

I think we usually do:
```js
Cc["@mozilla.org/uriloader/external-protocol-service;1"]
  .getService(Ci.nsIExternalProtocolService)
  .getApplicationDescription("http");
```

If you disagree, please leave it out - not important enough.

Comment 6

[Mark Banner (:standard8)]

Please can you also update this for the .eslintrc -> .eslintrc.js rename? (bug 1251003).

Comment 7

[:Gijs (he/him)]

Comment on attachment 8802142 [details]
Bug 1311043 - make eslint much more strict in browser/components/migration/, and fix issues,

https://reviewboard.mozilla.org/r/86650/#review85596

> nit: for an empty lambda, I find `() => {}` more descriptive. But hmmkay.

This actually returns a promise that resolves to `null` in case the stat failed. With `() => {}` I'd be confused about whether it returns an object (it does of course return undefined). Maybe that's just me. As you didn't mark this as an issue, leaving like this.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8802142 [details]
Bug 1311043 - make eslint much more strict in browser/components/migration/, and fix issues,

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05d46ac826ac
make eslint much more strict in browser/components/migration/, and fix issues, r=mikedeboer

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05d46ac826ac