1333044 - Enable eslint no-undef for services/

Status: RESOLVED FIXED

(Cloud Services :: General, defect)

Description

[Mark Banner (:standard8)]

As part of enabling no-undef in bug 1311312, it is fairly simple to enable it for services/, so I'd like to do that as a first step towards getting no-undef enabled in the tree.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1333044 - Prepare services/ for enabling no-undef eslint rule.

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

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1333044 - Fix no-shadow issues in services/.

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

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1333044 - Enable no-undef eslint rule for services/.

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

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8829997 [details]
Bug 1333044 - Prepare services/ for enabling no-undef eslint rule.

https://reviewboard.mozilla.org/r/106940/#review108060

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8829998 [details]
Bug 1333044 - Fix no-shadow issues in services/.

https://reviewboard.mozilla.org/r/106942/#review108088

::: services/fxaccounts/tests/xpcshell/test_accounts.js:528
(Diff revision 1)
>    user.verified = true;
>  
>    fxa.setSignedInUser(user).then(() => {
> -    fxa.getSignedInUser().then((user) => {
> +    fxa.getSignedInUser().then((user2) => {
>        // Before getKeys, we have no keys
>        do_check_eq(!!user.kA, false);

hrm - this code looks odd, but I think all |user|s in this block should be |user2|?

::: services/fxaccounts/tests/xpcshell/test_profile_client.js:24
(Diff revision 1)
>      // Store the request uri so tests can inspect it
>      Request._requestUri = requestUri;
> +    Request.ifNoneMatchSet = false;
>      return {
> -      setHeader() {},
> +      setHeader(header, value) {
> +        if (header == "If-None-Match" && value == "bogusETag") {

thanks!

::: services/sync/tests/unit/test_bookmark_tracker.js:960
(Diff revision 1)
>      PlacesUtils.favicons.replaceFaviconDataFromDataURL(iconURI, iconURL, 0,
>        Services.scriptSecurityManager.getSystemPrincipal());
>  
>      await new Promise(resolve => {
>        PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, iconURI, true,
> -        PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, (iconURI, dataLen, data, mimeType) => {
> +        PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, (uri, dataLen, data, mimeType) => {

I don't understand this change or where |uri| comes from?

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8829999 [details]
Bug 1333044 - Enable no-undef eslint rule for services/.

https://reviewboard.mozilla.org/r/106944/#review108084

awesome, thanks Mark!

::: services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm:937
(Diff revision 1)
>        return -1;
>      }
>      let expected_pos = -1;
>      if (this.props.before) {
> -      other_id = this.GetPlacesNodeId(this.props.folder_id,
> +      let other_id = this.GetPlacesNodeId(this.props.folder_id,
>                                        null,

we should probably indent these lines to match

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8829998 [details]
Bug 1333044 - Fix no-shadow issues in services/.

https://reviewboard.mozilla.org/r/106942/#review108088

> hrm - this code looks odd, but I think all |user|s in this block should be |user2|?

Yes, I missed that. Thanks for catching it.

> I don't understand this change or where |uri| comes from?

The last argument is a callback. iconURI is already declared in the upper scope, so this is just changing the argument name within the callback (no-shadow rule for eslint).

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8829997 [details]
Bug 1333044 - Prepare services/ for enabling no-undef eslint rule.

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

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8829998 [details]
Bug 1333044 - Fix no-shadow issues in services/.

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

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8829999 [details]
Bug 1333044 - Enable no-undef eslint rule for services/.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8b29e24280b
Prepare services/ for enabling no-undef eslint rule. r=jaws
https://hg.mozilla.org/integration/autoland/rev/06a658a93aab
Fix no-shadow issues in services/. r=markh
https://hg.mozilla.org/integration/autoland/rev/9c6b98edac72
Enable no-undef eslint rule for services/. r=markh

Comment 12

[Mark Banner (:standard8)]

Attachment: Fix eslint failures

Follow-up to fix the eslint failures.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1f3d30546a1
"Enable eslint no-undef for services/" a=tomcat

Comment 14

[Carsten Book [:Tomcat]]

sorry had to backout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=71891487&repo=autoland

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/788447f4f174
Backed out changeset d1f3d30546a1 
https://hg.mozilla.org/integration/autoland/rev/b01c14ac6688
Backed out changeset 9c6b98edac72 
https://hg.mozilla.org/integration/autoland/rev/a86db26c9700
Backed out changeset 06a658a93aab 
https://hg.mozilla.org/integration/autoland/rev/4346f9e1d390
Backed out changeset f8b29e24280b for perma failures in test_load_modules.js

Comment 16

[Mark Banner (:standard8)]

The tests fail on android as I was trying to include "resource://services-sync/constants.js" into services/crypto/modules/utils.js.

On Android, it appears that the sync part isn't shipped, and as a result, we then fail.

The reason we should use the constants.js file is that utils.js uses SYNC_KEY_DECODED_LENGTH.

https://dxr.mozilla.org/mozilla-central/rev/fbdfcecf0c774d2221f11aed5a504d5591c774e0/services/crypto/modules/utils.js#188

I think there's two options:

1) Move services/sync/modules/constants.js into common.
2) Change the check in utils.js to throw if dkLen isn't defined - as far as I can see, all the callers are defining dkLen.

Mark, any opinions on what we should do here?

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8829999 [details]
Bug 1333044 - Enable no-undef eslint rule for services/.

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

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8829999 [details]
Bug 1333044 - Enable no-undef eslint rule for services/.

Mark: this implements my option 2, as I think that's probably the best option.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8829999 [details]
Bug 1333044 - Enable no-undef eslint rule for services/.

https://reviewboard.mozilla.org/r/106944/#review109162

This looks great, thanks!

Comment 20

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Banner (:standard8) from comment #16)
> I think there's two options:
> 
> 1) Move services/sync/modules/constants.js into common.
> 2) Change the check in utils.js to throw if dkLen isn't defined - as far as
> I can see, all the callers are defining dkLen.
> 
> Mark, any opinions on what we should do here?

SGTM!

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 8829997 [details]
Bug 1333044 - Prepare services/ for enabling no-undef eslint rule.

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

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 8829998 [details]
Bug 1333044 - Fix no-shadow issues in services/.

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

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 8829999 [details]
Bug 1333044 - Enable no-undef eslint rule for services/.

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/964e4881f2fe
Prepare services/ for enabling no-undef eslint rule. r=jaws
https://hg.mozilla.org/integration/autoland/rev/d0ba3b285285
Fix no-shadow issues in services/. r=markh
https://hg.mozilla.org/integration/autoland/rev/f3741ccc800b
Enable no-undef eslint rule for services/. r=markh

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/964e4881f2fe
https://hg.mozilla.org/mozilla-central/rev/d0ba3b285285
https://hg.mozilla.org/mozilla-central/rev/f3741ccc800b