Closed Bug 1333044 Opened 7 years ago Closed 7 years ago

Enable eslint no-undef for services/

Categories

(Cloud Services :: General, defect)

Product:
Cloud Services
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1333044 - Prepare services/ for enabling no-undef eslint rule.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1333044 - Fix no-shadow issues in services/.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1333044 - Enable no-undef eslint rule for services/.
59 bytes, text/x-review-board-request
markh
: review+
Details 
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.
Assignee: nobody → standard8
Comment on attachment 8829997 [details]
Bug 1333044 - Prepare services/ for enabling no-undef eslint rule.

https://reviewboard.mozilla.org/r/106940/#review108060
Attachment #8829997 - Flags: review?(jaws) → review+
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?
Attachment #8829998 - Flags: review?(markh) → review+
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
Attachment #8829999 - Flags: review?(markh) → review+
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).
Pushed by mbanner@mozilla.com:
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
Attached patch Fix eslint failures (obsolete) — — Splinter Review 
Follow-up to fix the eslint failures.
Attachment #8830276 - Attachment is patch: true
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1f3d30546a1
"Enable eslint no-undef for services/" a=tomcat
sorry had to backout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=71891487&repo=autoland
Flags: needinfo?(standard8)
Backout by cbook@mozilla.com:
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
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?
Flags: needinfo?(standard8) → needinfo?(markh)
Attachment #8830276 - Attachment is obsolete: true
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.
Attachment #8829999 - Flags: review+ → review?(markh)
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!
Attachment #8829999 - Flags: review?(markh) → review+
(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!
Flags: needinfo?(markh)
Pushed by mbanner@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: