Enable eslint for browser/components/preferences

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

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

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

No description provided.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

3 years ago
mozreview-review
Comment on attachment 8780864 [details]
Bug 1294989 - Enable eslint for browser/components/preferences.

https://reviewboard.mozilla.org/r/71444/#review68960

Thanks for doing this. Some of this code is dying to be tidied a bit (like making the manual for (i++) array walking more idiomatic, and the duplication between cookies and permissions' treeviews, and...) but that can be done another day, this patch was enough work to write and review as it was. :-)

::: browser/components/preferences/fonts.js:80
(Diff revision 2)
>      // Only care in in-content prefs
>      if (!window.frameElement) {
>        return true;
>      }

Rip this out while we're here, please.

::: browser/components/preferences/fonts.js:93
(Diff revision 2)
>      if (!preferences.length) {
> -      return;
> +      return false;
>      }

AIUI this should be return true. Just like with "normal" events, returning undefined is not the same as returning false, and the docs say false prevents closing / saving the prefs.

::: browser/components/preferences/in-content/advanced.js:95
(Diff revision 2)
>      setEventListener("viewSecurityDevicesButton", "command",
>                       gAdvancedPane.showSecurityDevices);
>      setEventListener("cacheSize", "change",
>                       gAdvancedPane.updateCacheSizePref);
>  
> -#ifdef MOZ_WIDGET_GTK
> +    if (AppConstants.MOZ_WIDGET_GTK) {

This doesn't exist. You want

```
if (AppConstants.MOZ_WIDGET_TOOLKIT.startsWith("gtk"))
```

I expect (dxr shows that on dxr the value is "gtk3" and I guess "gtk2" is also possible and should also pass this -- that or define MOZ_WIDGET_GTK on AppConstants...).

::: browser/components/preferences/in-content/main.js:5
(Diff revision 2)
>  Components.utils.import("resource://gre/modules/Downloads.jsm");
>  Components.utils.import("resource://gre/modules/FileUtils.jsm");
>  Components.utils.import("resource://gre/modules/Task.jsm");
>  Components.utils.import("resource:///modules/ShellService.jsm");
>  Components.utils.import("resource:///modules/TransientPrefs.jsm");

So AppConstants.jsm is not requested here, but it's requested by a number of other files included in preferences.js. Can you file a (good first bug?) followup to make this system more sane and put the common lazy module getters / imports in preferences.js and dedup them?

::: browser/components/preferences/in-content/main.js:338
(Diff revision 2)
>        useCurrent.label = useCurrent.getAttribute("label2");
>      else
>        useCurrent.label = useCurrent.getAttribute("label1");
>  
>      // In this case, the button's disabled state is set by preferences.xml.
> -    if (document.getElementById
> +    if (document.getElementById("pref.browser.homepage.disable_button.current_page").locked)

Nit: please put the pref name in a temp var.

::: browser/components/preferences/in-content/sync.js:557
(Diff revision 2)
>          (event.charCode == KeyEvent.DOM_VK_SPACE || event.keyCode == KeyEvent.DOM_VK_RETURN))) {
>        return true;
> -    } else {
> -      return false;
>      }
> +    return false;

if (foo) return true; else return false; is almost as bad a pet peeve of mine as else-after-return...

Could just:

```
return <insert if condition here>
```

but I guess this keeps more blame and might be more readable according to some? Up to you.
Attachment #8780864 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)
Assignee

Comment 5

3 years ago
mozreview-review-reply
Comment on attachment 8780864 [details]
Bug 1294989 - Enable eslint for browser/components/preferences.

https://reviewboard.mozilla.org/r/71444/#review68960

Filed bug 1295000 for this cleanup.

> So AppConstants.jsm is not requested here, but it's requested by a number of other files included in preferences.js. Can you file a (good first bug?) followup to make this system more sane and put the common lazy module getters / imports in preferences.js and dedup them?

Filed bug 1294999.

Comment 6

3 years ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90324e0a64e7
Enable eslint for browser/components/preferences. r=Gijs

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/90324e0a64e7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

3 years ago
Depends on: 1307132
Depends on: 1342427
Depends on: 1342472
No longer depends on: 1365957
You need to log in before you can comment on or make changes to this bug.