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

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Serge Gautherie (:sgautherie)]

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"

Comment 1

[Serge Gautherie (:sgautherie)]

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?

Comment 2

[:aceman]

Yes, it looks like this is the single file today using this helper.

Comment 3

[:aceman]

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.

Comment 4

[:aceman]

Attachment: 734547.patch

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a51c78d74a8602d24e2dac340d0e22a0badd869a

Comment 5

[:aceman]

Attachment: 734547-SM.patch

Comment 6

[Jorg K (CEST = GMT+2)]

Your try run is pretty busted due to bug 1509194 :-(

Comment 7

[:aceman]

Yes, but the failures seem to be all in calendar and I see no composition tests failed.

Comment 8

[:aceman]

Try run now OK:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8d3690d65a70894b9934e0c009ef0dc376b6e44e

Comment 9

[Jorg K (CEST = GMT+2)]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dfe843fc864c
Use Services.prefs instead of getPref() in TB MsgComposeCommands.js. r=jorgk

Comment 11

[Frank-Rainer Grahl (:frg)]

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

Comment 12

[Jorg K (CEST = GMT+2)]

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.

Comment 13

[Frank-Rainer Grahl (:frg)]

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.

Comment 14

[:aceman]

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.

Comment 15

[Frank-Rainer Grahl (:frg)]

> 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.

Comment 16

[:aceman]

(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:(

Comment 17

[:aceman]

(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.

Comment 18

[Frank-Rainer Grahl (:frg)]

> 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.

Comment 19

[:aceman]

Attachment: 734547-SM.patch v1.1

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.

Comment 20

[Frank-Rainer Grahl (:frg)]

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.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6066121e0a2a
Use Services.prefs instead of getPref(), in SM MsgComposeCommands.js. r=frg

Comment 22

[Frank-Rainer Grahl (:frg)]

Done here? Happy to put it into esr60 too.

Comment 23

[:aceman]

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.