1316870 - Enable no-shadow rule for eslint for browser/

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

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.

Comment 1

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

Attachment: Bug 1316870 - Enable no-shadow eslint rule for browser/.

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

Comment 2

[Dave Townsend [:mossop]]

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

Comment 3

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

Comment on attachment 8809969 [details]
Bug 1316870 - Enable no-shadow eslint rule for browser/.

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

Comment 4

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

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

[Dave Townsend [:mossop]]

Comment on attachment 8809969 [details]
Bug 1316870 - Enable no-shadow eslint rule for browser/.

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

Comment 6

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

Comment on attachment 8809969 [details]
Bug 1316870 - Enable no-shadow eslint rule for browser/.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21b69215ab80
Enable no-shadow eslint rule for browser/. r=mossop

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/5f5e46d71cbf98eb61db0e5e934fd5ed7efc6109 so jaws can reland it rebased on inbound.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/602c0cfe0c921e8b0015ec4ae5fceec4f9d3c7ae
Bug 1316870 - Enable no-shadow eslint rule for browser/. r=mossop

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/602c0cfe0c92