Closed Bug 1316870 Opened 3 years ago Closed 3 years ago

Enable no-shadow rule for eslint for browser/

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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 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)
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 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+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/21b69215ab80
Enable no-shadow eslint rule for browser/. r=mossop
https://hg.mozilla.org/mozilla-central/rev/602c0cfe0c92
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Blocks: 1330063
You need to log in before you can comment on or make changes to this bug.