Enable no-shadow rule for eslint for browser/

RESOLVED FIXED in Firefox 53

Status

()

Firefox
General
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Enabling the no-shadow rule for all of m-c has thousands of errors, so this approach is to fix the no-shadow rule piecemeal.

The patch here should fix the no-shadow rule for /browser.
Blocks: 1316872
Comment hidden (mozreview-request)

Comment 2

7 months ago
mozreview-review
Comment on attachment 8809969 [details]
Bug 1316870 - Enable no-shadow eslint rule for browser/.

https://reviewboard.mozilla.org/r/92444/#review92500

I'm seeing a few things that I'd expect to show up on try so I'll wait to review this until it has a clean try run.

::: browser/base/content/browser-addons.js:136
(Diff revision 1)
>              name.setAttribute("class", "addon-install-confirmation-name");
>              container.appendChild(name);
>  
>              if (someUnsigned && install.addon.signedState <= AddonManager.SIGNEDSTATE_MISSING) {
> -              let unsigned = document.createElement("label");
> -              unsigned.setAttribute("value", gNavigatorBundle.getString("addonInstall.unsigned"));
> +              let unsignedLabel = document.createElement("label");
> +              unsignedLabela.setAttribute("value",

This looks like a typo.

::: browser/base/content/browser.js:6008
(Diff revision 1)
>          callback: function() {
> -          for (let [browser, docId, uri] of notification.options.controlledItems) {
> -            OfflineApps.allowSite(browser, docId, uri);
> +          for (let controlledItem of notification.options.controlledItems) {
> +            OfflineApps.allowSite(
> +              controlledItem.browser,
> +              controlledItem.docId,
> +              controlledItem.uri

You're trying to expand an array as if it is an object.

::: browser/base/content/browser.js:6018
(Diff revision 1)
>        let secondaryActions = [{
>          label: gNavigatorBundle.getString("offlineApps.never"),
>          accessKey: gNavigatorBundle.getString("offlineApps.neverAccessKey"),
>          callback: function() {
> -          for (let [, , uri] of notification.options.controlledItems) {
> -            OfflineApps.disallowSite(uri);
> +          for (let controlledItem of notification.options.controlledItems) {
> +            OfflineApps.disallowSite(controlledItem.uri);

Same here
Attachment #8809969 - Flags: review?(dtownsend)
Comment hidden (mozreview-request)
I pushed this to tryserver again.

The first time I pushed this to tryserver there were some failures that showed up:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f898b30cd3aaedf92685434ab7237cb901767f2

I can reproduce these failures locally, but these tests continue to fail locally even when the patch is unapplied. I believe this is because I'm using artifact builds, and the trypush above is using an artifact build.

The following trypush is not using an artifact build,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8241b4b0a4cfc6721dfe9a7050f9c105c8d136a

Comment 5

6 months ago
mozreview-review
Comment on attachment 8809969 [details]
Bug 1316870 - Enable no-shadow eslint rule for browser/.

https://reviewboard.mozilla.org/r/92444/#review93296
Attachment #8809969 - Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request)

Comment 7

6 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21b69215ab80
Enable no-shadow eslint rule for browser/. r=mossop
Backed out in https://hg.mozilla.org/integration/autoland/rev/5f5e46d71cbf98eb61db0e5e934fd5ed7efc6109 so jaws can reland it rebased on inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/602c0cfe0c921e8b0015ec4ae5fceec4f9d3c7ae
Bug 1316870 - Enable no-shadow eslint rule for browser/. r=mossop
https://hg.mozilla.org/mozilla-central/rev/602c0cfe0c92
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

4 months ago
Blocks: 1330063
status-firefox52: affected → ---
You need to log in before you can comment on or make changes to this bug.