Closed Bug 1278337 Opened 4 years ago Closed 4 years ago

remove preprocessing from preferences .js files by converting to AppConstants.jsm

Categories

(Thunderbird :: Preferences, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 50.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 1 obsolete file)

24.05 KB, patch
aceman
: review+
Details | Diff | Splinter Review
Remove preprocessing from preferences .js files by converting to AppConstants.jsm
Blocks: 1251586
Attached patch patch (obsolete) — Splinter Review
Try run: https://hg.mozilla.org/try-comm-central/rev/b9d0418d43d8bedbe589ef36750aa697c4f9feb7
Attachment #8760464 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8760464 [details] [diff] [review]
patch

Review of attachment 8760464 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/preferences/advanced.js
@@ +77,5 @@
> +        checkElem.checked = false;
> +      }
> +      checkElem = document.getElementById("checkDefaultButton");
> +      if (checkElem)
> +        checkElem.disabled = true;

Good with the added comment, but document.getElementById is really fast enough that the old code is better as it's more readable

@@ +208,5 @@
>   */
>  updateReadPrefs: function ()
>  {
> +  if (!AppConstants.MOZ_UPDATER)
> +    return;

unneeded, no?

@@ +255,5 @@
>   */
>  updateWritePrefs: function ()
>  {
> +  if (!AppConstants.MOZ_UPDATER)
> +    return;

and here

@@ +278,5 @@
>  
>    showUpdates: function ()
>    {
> +    if (!AppConstants.MOZ_UPDATER)
> +      return;

and here
Attachment #8760464 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Magnus Melin from comment #2)
> @@ +208,5 @@
> >   */
> >  updateReadPrefs: function ()
> >  {
> > +  if (!AppConstants.MOZ_UPDATER)
> > +    return;
> 
> unneeded, no?
Safety and preserving semantics :)
With the preprocessing the functions wouldn't even exist. So now at least make them do nothing so that nobody calls them by mistake. And if he does, no harm happens.
Should I remove it?
Well they are available now, not just called. Anyway, I think it's unnecessary to have those checks
Attached patch patch v1.1Splinter Review
OK.
Attachment #8760464 - Attachment is obsolete: true
Attachment #8760890 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/c37795888b0e9e92bbb145403b72bf14a201285e
Bug 1278337 - remove preprocessing from preferences .js files by converting to AppConstants.jsm. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
Depends on: 1388882
You need to log in before you can comment on or make changes to this bug.