703836 - MsgComposeCommands.js Code cleanup: Use Services more, reuse code, and remove commented dumps.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, enhancement)

Description

[Philip Chee]

Relevant Thunderbird bugs.
  [Bug 472621] Thunderbird fails to use some fonts for the default font of HTML messages, if the font has non-ASCII name.
  [Bug 482181] Stop dumping expected conditions in MsgComposeCommands.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Code Cleanup.

The indentation style in this file is fustrating especially if you try to keep line lengths within 80 cols. So this patch is rather inconsistent. Suggestions for better formatting/indentation welcome.

> +  fontFace = GetStringPref("msgcompose.font_face");
> +  if (fontFace)
> +    doStatefulCommand('cmd_fontFace', fontFace);
> +
>    try {
> -    fontFace = pref.getCharPref("msgcompose.font_face");
> -    doStatefulCommand('cmd_fontFace', fontFace);
> -  } catch (e) {}
> -
> -  try {
> -    fontSize = pref.getCharPref("msgcompose.font_size");
> +    fontSize = Services.prefs.getCharPref("msgcompose.font_size");
>      EditorSetFontSize(fontSize);
>    } catch (e) {}

From [Bug 472621] Thunderbird fails to use some fonts for the default font of HTML messages, if the font has non-ASCII name.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 575627 [details] [diff] [review]
Patch v1.0 Code Cleanup.

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

Disclaimer: This is just an indent improvement suggestion (as requested on the newsgroups), no actual review. Furthermore I only checked the target side.

::: suite/mailnews/compose/MsgComposeCommands.js
@@ +733,5 @@
>    if (flag) {
> +    Services.prefs.addObserver("ldap_2.autoComplete.useDirectory",
> +                                directoryServerObserver, false);
> +    Services.prefs.addObserver("ldap_2.autoComplete.directoryServer",
> +                                directoryServerObserver, false);

Indent off by one (2x)

@@ +823,5 @@
>  
>          // fill in the session params if there is a session
>          //
>          if (LDAPSession) {
> +            let url = GetStringPref(autocompleteDirectory +".uri");

space before ".uri"

@@ +877,1 @@
>                    autocompleteDirectory + ".autoComplete.cjkMinStringLength");

indent two more spaces

@@ +925,5 @@
>                  case 1:
>                      // use the name of this directory
>                      //
> +                    ldapFormatter.commentFormat = GetStringPref(
> +                                autocompleteDirectory + ".description");

indent four spaces only

@@ +1800,5 @@
>          {
>            var disableFallback = false;
>            try
>            {
> +            disableFallback = Services.prefs.getBoolPref("mailnews.disable_fallback_to_utf8." + originalCharset);

could be wrapped

@@ +2458,5 @@
>    try {
>      var file = attachedLocalFile.QueryInterface(Components.interfaces.nsIFile);
>      var parent = file.parent.QueryInterface(Components.interfaces.nsILocalFile);
>  
> +    Services.prefs.setComplexValue(kComposeAttachDirPrefName, Components.interfaces.nsILocalFile, parent);

could be wrapped

Comment 3

[Philip Chee]

Thanks! I was going cross-eyed after a while.

Comment 4

[Philip Chee]

Attachment: Patch v1.1 More Cleanup.

Code Cleanup:

1. Make more use of Services.jsm.
2. Remove commented out dumps.
3. Make more use of GetMsgIdentityElement(), GetMsgSubjectElement(), GetMsgAttachmentElement(), GetMsgHeadersToolbarElement().
4. Port GetPref() helper function from Thunderbird's MsgComposeCommands.js and make use of it to shorten line lengths.
5. Remove a few globals that are now unused due to (4).

We don't have any unit tests so I just tested briefly by composing and successfully sending off a mail message.

> Indent off by one (2x)
> space before ".uri"
> indent two more spaces
> indent four spaces only
> could be wrapped
> could be wrapped
Fixed. More or less.

> +  // This version of GetStringPref() comes from editorUtilities.js instead
> +  // of utilitiesOverlay.js
> +  fontFace = GetStringPref("msgcompose.font_face");

Fortunately they both behave the same way.

Comment 5

[Ian Neal]

Comment on attachment 576772 [details] [diff] [review]
Patch v1.1 More Cleanup.

See https://bugzilla.mozilla.org/show_bug.cgi?id=665565#c2 I already tried to do some of this. If Mark is happy for this to go ahead, then fine, I will review it (some of the try/catches could be removed too in setupLdapAutocompleteSession function).

Comment 6

[Philip Chee]

Attachment: Patch v1.2

Changes since the last patch:

1. Reverted the LDAP related changes.
2. Remove DoCommandPrint() and call PrintUtils.print() directly since ::print()
   is already wrapped in a try/catch block.
3. Remove try/catch block around nsIRDFService. Nothing else in suite bothers
   to do this.
4. Move sAccountManagerDataSource into compareAccountSortOrder().
5. Make more use of GetMsgSubjectElement().

Comment 7

[Ian Neal]

Comment on attachment 578261 [details] [diff] [review]
Patch v1.2

>       try {
>         args[argname] = decodeURIComponent(argvalue);
>       } catch (e) {args[argname] = argvalue;}
>-    dump("[" + argname + "=" + args[argname] + "]\n");
>+    // dump("[" + argname + "=" + args[argname] + "]\n");
>   }
I know this is going from an uncommented to a commented out dump command but why not remove it completely?

>+  // XXX: Get the preferences service. Remove this once LDAP autocomplete has
>+  // been moved to toolkit interfaces.
>+  sPrefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                     .getService(Components.interfaces.nsIPrefService)
>+                     .QueryInterface(Components.interfaces.nsIPrefBranch2);
Why not set sPrefs to Services.prefs?

> function compareAccountSortOrder(account1, account2)
> {
>+  var ds = Components.classes["@mozilla.org/rdf/datasource;1?name=msgaccountmanager"]
>+                     .getService(Components.interfaces.nsIRDFDataSource);
How expensive is it to move this into the sort function? I guess most people do not have that many accounts but worth checking.

>+  // This version of GetStringPref() comes from editorUtilities.js instead of
>+  // utilitiesOverlay.js
>+  fontFace = GetStringPref("msgcompose.font_face");
I know you referenced a TB bug but I could not see this change in their code.
Also what is throwing and needs to be caught in this and the following code? Is there now a way of not waiting for the throw?

r- until some answers are know.

Comment 8

[Philip Chee]

>>       try {
>>         args[argname] = decodeURIComponent(argvalue);
>>       } catch (e) {args[argname] = argvalue;}
>>-    dump("[" + argname + "=" + args[argname] + "]\n");
>>+    // dump("[" + argname + "=" + args[argname] + "]\n");
>>   }
> I know this is going from an uncommented to a commented out dump command but why not remove it completely?

In Thunderbird Bug 482181 philor left this in commented. Nothing in the bug comments indicates why. And I doubt philor can remember why after more than two years.
http://hg.mozilla.org/comm-central/rev/8f212879cf31#l1.12
If Thunderbird eventually removes this commented out dump we can follow in a new bug/patch.

>>+  // XXX: Get the preferences service. Remove this once LDAP autocomplete has
>>+  // been moved to toolkit interfaces.
>>+  sPrefs = Components.classes["@mozilla.org/preferences-service;1"]
>>+                     .getService(Components.interfaces.nsIPrefService)
>>+                     .QueryInterface(Components.interfaces.nsIPrefBranch2);
> Why not set sPrefs to Services.prefs?

I thought that Services.prefs is a lazy service getter but we are forcing it to instantiate here, so we might as well go directly. I don't think there is much difference anyway since this code is slated for deletion. Also IIRC Neil doesn't like these multiple levels of indirection.

>> function compareAccountSortOrder(account1, account2)
>> {
>>+  var ds = Components.classes["@mozilla.org/rdf/datasource;1?name=msgaccountmanager"]
>>+                     .getService(Components.interfaces.nsIRDFDataSource);
> How expensive is it to move this into the sort function? I guess most people do not have that many accounts but worth checking.
I want to avoid mission creep. I prefer to do the refactoring if any in another port bug. IIRC Thunderbird does as you suggested.

>>+  // This version of GetStringPref() comes from editorUtilities.js instead of
>>+  // utilitiesOverlay.js
>>+  fontFace = GetStringPref("msgcompose.font_face");
> I know you referenced a TB bug but I could not see this change in their code.

http://hg.mozilla.org/comm-central/rev/c9502995cd11#l1.19
Thunderbird does s/pref.getCharPref(...)/getPref("msgcompose.font_face", true)/
Notice that if the second parameter to getPref() is true, getPref returns a unichar. Here I've called GetStringPref() directly since it is available to us.

> Also what is throwing and needs to be caught in this and the following code? Is there now a way of not waiting for the throw?
I want to avoid mission creep. Can we audit the try/catch blocks in another bug?

Comment 9

[Ian Neal]

Comment on attachment 578261 [details] [diff] [review]
Patch v1.2

Ok, r+ then :)

Comment 10

[Philip Chee]

Comment on attachment 578261 [details] [diff] [review]
Patch v1.2

asking sr/moa? from Karsten.

Comment 11

[Karsten Düsterloh]

Comment on attachment 578261 [details] [diff] [review]
Patch v1.2

Not a full review, just an architectural moa+.

> function loadHTMLMsgPrefs()
> {
>-  var pref = GetPrefs();
>   var fontFace;
>   var fontSize;
>   var textColor;
>   var bgColor;
> 
>+  // This version of GetStringPref() comes from editorUtilities.js instead of
>+  // utilitiesOverlay.js
>+  fontFace = GetStringPref("msgcompose.font_face");

You could move the var here.

>+// Thunderbird Compatibility function.
>+function getPref(aPrefName, aIsComplex)

Sigh. 
But even if it helps compatibility, it doesn't deserve an uppercase C. ;-)

Comment 12

[Philip Chee]

>> function loadHTMLMsgPrefs()
>> {
>>-  var pref = GetPrefs();
>>   var fontFace;
>>   var fontSize;
>>   var textColor;
>>   var bgColor;
>> 
>>+  // This version of GetStringPref() comes from editorUtilities.js instead of
>>+  // utilitiesOverlay.js
>>+  fontFace = GetStringPref("msgcompose.font_face");
> 
> You could move the var here.
Fixed.

>>+// Thunderbird Compatibility function.
>>+function getPref(aPrefName, aIsComplex)
> 
> Sigh. 
> But even if it helps compatibility, it doesn't deserve an uppercase C. ;-)
Fixed.

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/525cf03902bc