Use Services.prefs or Application.prefs, instead of getPref(), in (SM and TB) MsgComposeCommands.js

RESOLVED FIXED in Thunderbird 65.0

Status

defect
--
minor
RESOLVED FIXED
8 years ago
8 months ago

People

(Reporter: sgautherie, Assigned: aceman)

Tracking

Trunk
Thunderbird 65.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

At least, most uses should be able to be replaced by direct Services.prefs.get*Pref() calls.

Could we even get rid of the function itself or is supposed to be an API for other consumers?

"Found 54 matching lines in 2 files"
Flags: in-testsuite-
Depends on: 734549
Ah, this getPref() returns 'null' as a default value for non-complex types.
Do we actually need to support that case?
If yes, would it be "better" to use Application.prefs with explicit (typed) defaults?
Summary: Use Services.prefs, instead of getPref(), in (SM and TB) MsgComposeCommands.js → Use Services.prefs or Application.prefs, instead of getPref(), in (SM and TB) MsgComposeCommands.js
Yes, it looks like this is the single file today using this helper.
Assignee: nobody → acelists
Also these days the Services.prefs.get*Pref() function do accept a default value if the pref doesn't exist, so the conversion should be easier.
Status: NEW → ASSIGNED
Posted patch 734547-SM.patch (obsolete) — Splinter Review
Attachment #9026846 - Flags: review?(frgrahl)
Your try run is pretty busted due to bug 1509194 :-(
Yes, but the failures seem to be all in calendar and I see no composition tests failed.
Comment on attachment 9026845 [details] [diff] [review]
734547.patch

Looking good, thanks for the clean-up after six years. I'll land the TB part now.
Attachment #9026845 - Flags: review?(jorgk) → review+
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dfe843fc864c
Use Services.prefs instead of getPref() in TB MsgComposeCommands.js. r=jorgk
Target Milestone: --- → Thunderbird 65.0
Comment on attachment 9026846 [details] [diff] [review]
734547-SM.patch

Thanks for cleaning up. Still a few questions remain.

>+          let disableFallback = Services.prefs
>+            .getBoolPref("mailnews.disable_fallback_to_utf8." + originalCharset, "");
Shouldn't this be:
>+            .getBoolPref("mailnews.disable_fallback_to_utf8." + originalCharset, false);


>+  if (Services.prefs.getBoolPref("mail.autoComplete.highlightNonMatches"))
Same here:
>+  if (Services.prefs.getBoolPref("mail.autoComplete.highlightNonMatches", false))


>+  if (Services.prefs.getIntPref("mail.autoComplete.commentColumn", 0))
int is not bool so maybe
>+  if (Services.prefs.getIntPref("mail.autoComplete.commentColumn", 0) != 0)


>+  var fontSize = Services.prefs.getCharPref("msgcompose.font_size", "");
>+  if (fontSize)

Will return the default so should imho never be null. Maybe:
>+    if (!fontSize.isEmpty())

Unless the default should be set. Then you can imho remove the if completely.

>+  textColor = Services.prefs.getCharPref("msgcompose.text_color", "");
>+  if (!bodyElement.hasAttribute("text") && textColor)
>+  {

var textcolor and comparision as with fontSize

Only used if the body element has the text attribute so maybe skip the call otherwise e.g.

>+  textColor = Services.prefs.getCharPref("msgcompose.text_color", "");
>+  if (!bodyElement.hasAttribute("text")) {
>+    var textColor = Services.prefs.getCharPref("msgcompose.text_color", "");
>+     if (!textColor.isEmpty())

Same for bgColor
Attachment #9026846 - Flags: review?(frgrahl) → feedback+
Looking at https://hg.mozilla.org/comm-central/rev/dfe843fc864c, it still looks OK to me. getBoolPref() doesn't have a default value except here
  getBoolPref("mailnews.disable_fallback_to_utf8." + originalCharset, false);
where the "dynamic" pref doesn't need to exist.
I was/am under the impression that it still throws when the pref does not exist and you are not providing the default. If not then I stand corrected.
Yes, it still throws if the pref does not exist and no default is provided in the get*Pref() call.
But I didn't add a default where the original code didn't have one. E.g. if the original code had a try{}catch{} block to handle the non-existent pref, I added the default so it no longer throws. If the original code would throw, I let the new core throw.
Yes, if the getPref() returned null on non-existent prefs (did it take the nsIPrefBranch.PREF_INVALID branch?) then it would never throw, but I didn't want the return random nulls in the new code. I rather let it throw so that we can find the missing prefs. Also, I have checked which prefs we define in mailnews.js so none of them should trow.

So if you want I can add the default values for SM, but I do not deem them needed.
But I agree with the bug of Services.prefs.getIntPref() being handled as bool. I'll fix that, thanks.
> But I didn't add a default where the original code didn't have one.

That is what I mean. try catch got removed but no default e.g. here:

> -  try {
> -      if (getPref("mail.autoComplete.highlightNonMatches"))
> -        autoCompleteWidget.highlightNonMatches = true;
> -
> -      if (getPref("mail.autoComplete.commentColumn"))
> -        autoCompleteWidget.showCommentColumn = true;
> -  } catch (ex) {
> -      // if we can't get this pref, then don't show the columns (which is
> -      // what the XUL defaults to)
> -  }
> +  if (Services.prefs.getBoolPref("mail.autoComplete.highlightNonMatches"))
> +    autoCompleteWidget.highlightNonMatches = true;

> +          let disableFallback = Services.prefs
> +            .getBoolPref("mailnews.disable_fallback_to_utf8." + originalCharset, "");

And a default "" for a bool pref which I think is not right. Should be true or false.
(In reply to Frank-Rainer Grahl (:frg) from comment #15)
> > But I didn't add a default where the original code didn't have one.
> 
> That is what I mean. try catch got removed but no default e.g. here:

Because mail.autoComplete.highlightNonMatches is defined in mailnews.js so it will not throw.
 
> > +          let disableFallback = Services.prefs
> > +            .getBoolPref("mailnews.disable_fallback_to_utf8." + originalCharset, "");
> 
> And a default "" for a bool pref which I think is not right. Should be true
> or false.

Yes, this is a bug similar to the getIntPref() as bool, I'll fix this. Thanks for noticing. Somehow I didn't do the same mistake in the TB version:(
(In reply to Frank-Rainer Grahl (:frg) from comment #11)
> >+  var fontSize = Services.prefs.getCharPref("msgcompose.font_size", "");
> >+  if (fontSize)

> Will return the default so should imho never be null. Maybe:
> >+    if (!fontSize.isEmpty())

Ok, yes, msgcompose.font_size is in mailnews.js. I just went by the original code, which expected a trow (pref being undefined) so I added the pref and kept the check.

> Unless the default should be set. Then you can imho remove the if completely.

So if somebody sets the value to "", do you want to set EditorSetFontSize()? if("") skips the block.

> >+  textColor = Services.prefs.getCharPref("msgcompose.text_color", "");
> >+  if (!bodyElement.hasAttribute("text") && textColor)
> >+  {
> 
> var textcolor and comparision as with fontSize

I added check of textColor (not being empty or "") according to TB, maybe we do not want to set the "text" attribute to "", which may have a different meaning than the attribute missing completely.
> So if somebody sets the value to "", do you want to set EditorSetFontSize()? if("") skips the block.

I see msgcompose.font_size is set in mailnews.js so I would probably remove the check. If someone changes it "" is just one of many wrong values. To make it failsafe it would need to be validated too. I would say the orginal "if (fontSize)" even with the throw was always evaluted to true because inside the catch block.

> I added check of textColor (not being empty or "") according to TB, maybe we do not want to set 
> the "text" attribute to "", which may have a different meaning than the attribute missing 
> completely.

msgcompose.text_color and msgcompose.background_color are also defined in mailnews.js so the checks could probably be dropped to. If you set it to blank or an invalid value interesting things will probably happen anyway :)

In any case aside from the mentioned two errors I am fine with the check if it matches TB. Was just concerned that it would throw without a default value. Should have checked the default prefs.
Thanks, I updated the patch.
You can compare with the TB version which is also attached.
For the text colors, TB also does not set any if msgcompose.default_colors is true, which the SM versions doesn't have.
Attachment #9026846 - Attachment is obsolete: true
Attachment #9027355 - Flags: review?(frgrahl)
Comment on attachment 9027355 [details] [diff] [review]
734547-SM.patch v1.1

Thanks looks good. Backported to 2.53 and see no errors there either.
Attachment #9027355 - Flags: review?(frgrahl) → review+
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/6066121e0a2a
Use Services.prefs instead of getPref(), in SM MsgComposeCommands.js. r=frg
Done here? Happy to put it into esr60 too.
It does not need to go into 60, it does not fix any urgent bugs.
This just allows us to finally close long standing bug 720356.

But yes, this one is done, no further patches planned.
Thanks.
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.