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

RESOLVED FIXED in seamonkey2.9

Status

enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

Trunk
seamonkey2.9
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Posted patch Patch v1.0 Code Cleanup. (obsolete) — Splinter Review
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.
Attachment #575627 - Flags: review?(iann_bugzilla)
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
Thanks! I was going cross-eyed after a while.
Posted patch Patch v1.1 More Cleanup. (obsolete) — Splinter Review
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.
Attachment #575627 - Attachment is obsolete: true
Attachment #575627 - Flags: review?(iann_bugzilla)
Attachment #576772 - Flags: review?(iann_bugzilla)
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).
Attachment #576772 - Flags: review?(iann_bugzilla) → review-
Posted patch Patch v1.2Splinter Review
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().
Attachment #576772 - Attachment is obsolete: true
Attachment #578261 - Flags: review?(iann_bugzilla)
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.
Attachment #578261 - Flags: review?(iann_bugzilla) → review-
>>       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 on attachment 578261 [details] [diff] [review]
Patch v1.2

Ok, r+ then :)
Attachment #578261 - Flags: review- → review+
Comment on attachment 578261 [details] [diff] [review]
Patch v1.2

asking sr/moa? from Karsten.
Attachment #578261 - Flags: superreview?(mnyromyr)
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. ;-)
Attachment #578261 - Flags: superreview?(mnyromyr) → superreview+
>> 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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.9
Depends on: 714595
You need to log in before you can comment on or make changes to this bug.