421846 - Clean up Thunderbird's compose global scope

Status: RESOLVED WORKSFORME

(Thunderbird :: Message Compose Window, defect)

Description

[Joey Minta]

Attachment: patch v1

This started out as a mission to trim out some RDF and morphed into a clean-up global scope patch.  This patch also removes some bizarre null-checks (and results in a lot of whitespace changes).  I know philor loves this kind of bug. :-)

Comment 1

[Joey Minta]

Attachment: patch v1 -w

Same as above but trying -w

Comment 2

[Mark Banner (:standard8)]

-const nsIMsgCompDeliverMode = Components.interfaces.nsIMsgCompDeliverMode;
-const nsIMsgCompSendFormat = Components.interfaces.nsIMsgCompSendFormat;
...

So now we have to type it long fashion (ok, I can cope with that), but this appears to have taken lines over the 80-char "limit" that we typcially soft-impose.

-var gAccountManager

This doesn't seem to be a saving. From what I can tell we're going to need account manager whenever we compose a message. So with this patch we'll have 5 instances in different functions where we get the account manager service each time, rather than just caching that in memory. I really don't think that's an advantage.

Comment 3

[Joey Minta]

> So now we have to type it long fashion (ok, I can cope with that), but this
> appears to have taken lines over the 80-char "limit" that we typcially
> soft-impose.
We can have the Cc/Ci debate if you want, or I can create a bunch of shorter temporary variables to store the longer interfaces/contracts just prior to calling createInstance.

> This doesn't seem to be a saving. From what I can tell we're going to need
> account manager whenever we compose a message. So with this patch we'll have 5
> instances in different functions where we get the account manager service each
> time, rather than just caching that in memory. I really don't think that's an
> advantage.
> 
The point isn't about saving lines. It's about (1) allowing GC to collect things we're not using and (2) making it clear what's being used where, instead of functions reaching up 2000 lines (or into other files) for variables.  An extension author trying to lxr for "gaccountmanager" to figure out what this thing is will be in a much tougher position if it's a global variable, rather than being defined right where it's used.  (See http://mxr.mozilla.org/seamonkey/search?string=gaccountmanager It took me a few days at first to figure out what "messenger" actually was.)

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> > So now we have to type it long fashion (ok, I can cope with that), but this
> > appears to have taken lines over the 80-char "limit" that we typcially
> > soft-impose.
> We can have the Cc/Ci debate if you want, or I can create a bunch of shorter
> temporary variables to store the longer interfaces/contracts just prior to
> calling createInstance.

I'm not too fussed, as long as we keep line lengths to a resonable limit. I'm sure I saw one or two earlier where you had replaced them with the extended version which made the lines very long, unfortunately, I can't find them now.

> > This doesn't seem to be a saving. From what I can tell we're going to need
> > account manager whenever we compose a message. So with this patch we'll have 5
> > instances in different functions where we get the account manager service each
> > time, rather than just caching that in memory. I really don't think that's an
> > advantage.
> > 
> The point isn't about saving lines. It's about (1) allowing GC to collect
> things we're not using and (2) making it clear what's being used where, instead
> of functions reaching up 2000 lines (or into other files) for variables.  An
> extension author trying to lxr for "gaccountmanager" to figure out what this
> thing is will be in a much tougher position if it's a global variable, rather
> than being defined right where it's used.  (See
> http://mxr.mozilla.org/seamonkey/search?string=gaccountmanager It took me a few
> days at first to figure out what "messenger" actually was.)

Well AFAIK GC won't collect services - they will remain alive for the length of the time we've got the app running, so doing that replacement here I believe will just slow things down and increase code size as opposed to improving memory usage. From what I have heard FF, caching services definitely helps improve performance. If I'm wrong please correct me.

Yes it can be unclear, in MsgComposeCommands.js, gAccountManager is actually badly defined anyway. it should be:

var gAccountManager = Components.classes....

and not being initialised to null and then set up in InitializeGlobalVariables. That would be one step to making it a lot clearer.

The second step would be the naming, being consistent in our naming, e.g. using "gIOService" everywhere not only allows us to save getting the global services all the time, but provides consistency for developers (and extension developers) - I can look at that and know that in all probability its got the nsIIOService in it, if it hasn't then it shouldn't have been called that.

Additionally, gAccountManager IMHO is a bad name because a) it screws up mxr searches (as you rightly pointed out, though more due the interface name), b) it doesn't reflect the interface name correctly - the better option would be gMsgAccountManager. This better option would clearly identify it for the Mailnews/Messenger account manager and not an account manager for internet sites (I know we don't have one of those, its just an example).

I agree though that we shouldn't have big functions just for variables, but those are obviously designed wrong, and maybe a class-based approach would have been better used.

So I think we should cache services, but we need to review our designs to ensure that we're not making things unnecessarily complex by grouping everything into one file (e.g. the LDAP stuff shouldn't even be in that file).

Comment 5

[Joey Minta]

(In reply to comment #4)
> Well AFAIK GC won't collect services - they will remain alive for the length of
> the time we've got the app running, so doing that replacement here I believe
> will just slow things down and increase code size as opposed to improving
> memory usage. From what I have heard FF, caching services definitely helps
> improve performance. If I'm wrong please correct me.

OK, I'll grant the memory bit.  I should also say that I'm not opposed to caching services generally, but rather to caching them in the global scope.  The services I've seen cached in Firefox have always been done within large variables/prototypes that contain all the functionality for a given area.  I fully support that idea.  See e.g. [1].  The required "this" reference in that case avoids the problem I described of being unable to identify what the object is.  I *think* this was your 'class-based' approach?

It seems at this point, we both agree that the ideal solution would be to have the compose commands be in such a self-contained object.  I'm certainly in favor of requiring this for new functionality we add.  The question remains, which is a better second-best alternative, since I don't think a full restructuring of the file is worthwhile at this point.  Global services or repetitive calls?

[1] http://mxr.mozilla.org/mozilla/source/browser/components/places/content/utils.js#100

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

If there's a sufficiently large amount of repetition, caching them globally might be ok, but in general, keeping them close to where they're used is a worthwhile endeavor.  It makes them more likely to be removed when they become unnecessary.

Comment 7

[Joey Minta]

Attachment: just the s-vars

Since the last patch was a bit ridiculous, this patch just takes care of the s* variables.  This is the trickiest of the sets (g* vars are easier, and interfaces are the easiest), but it's the one which involves RDF, so I care about it the most.

Comment 8

[Phil Ringnalda (:philor)]

Comment on attachment 310408 [details] [diff] [review]
just the s-vars

Looks fine, and seems to work fine near as I can tell, just a bunch of nits (mostly not your original fault, I know, but you touch 'em, you own 'em):

>+                serverURL.spec = getPref(autocompleteDirectory +".uri", true);

Space after +

>+                ldapFormatter.addressFormat = getPref(autocompleteDirectory + 
>+                                                       ".autoComplete.addressFormat",
>+                                                      true);

Extra space on the middle line

>+                    ldapFormatter.commentFormat = getPref(
>+                                autocompleteDirectory + ".description", true);

The rest of the things that were using this grotesque wrapping used a 4 space indent on the wrapped line, rather than... some random amount. Blech. There's no good solution, but this an awful one.

>+    document.getElementById("returnReceiptMenu").setAttribute('checked', 
>+                                         gMsgCompose.compFields.returnReceipt);
>+    document.getElementById("dsnMenu").setAttribute('checked', gMsgCompose.compFields.DSN);
>+    document.getElementById("cmd_attachVCard").setAttribute('checked', 
>+                                         gMsgCompose.compFields.attachVCard);
>+    document.getElementById("menu_inlineSpellCheck")
>+            .setAttribute('checked', getPref("mail.spellcheck.inline"));

Oy! The last of those wrapping (or not wrapping) methods both works for all four lines, and is something that could actually be described; let's go with that one.

>+      // setEditorType MUST be call before setContentWindow

be called

>+        //Remove HTML toolbar, format and insert menus as we are editing in plain text mode

Space after //, s/Remove/Hide/, should be two lines (yeah, I know, just moving it, but you're going to have blame)

>+      // Add an observer to be called when document is done loading,
>+      //   which creates the editor

Knock out those extra spaces in the second line, please

>+        // Load empty page to create the editor
>+        editorElement.webNavigation.loadURI("about:blank", // uri string
>+                             0,                            // load flags
>+                             null,                         // referrer
>+                             null,                         // post-data stream
>+                             null);

Did that spacing make some sort of sense before someone changed spaces to tabs? This seems like a marvelous chance to de-ass it.

Those, plus please de-nit yourself for the trailing whitespace and any of the long lines that will wrap without severe ugliness in http://beaufour.dk/jst-review/?patch=310408 and I swear the next time I'll be quicker to review.

Comment 9

[Joey Minta]

Attachment: just the s-vars v2

The s-vars, version 2.  This one should have the review comments addressed, as well as jst's whitespace nits picked.

Comment 10

[Phil Ringnalda (:philor)]

Comment on attachment 312840 [details] [diff] [review]
just the s-vars v2

looks good, likely to save us all some tooth enamel, r=me

Comment 11

[Joey Minta]

s-vars checked in. g-vars better be on the lookout because I'm coming for them next.

Checking in mail/components/compose/content/MsgComposeCommands.js;
/cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.128; previous revision: 1.127
done

Comment 12

[Joey Minta]

Attachment: the gvars

This takes care of the easier global variables.  Getting rid of some of the others would require more refactoring, that should probably be done in other bugs.  This patch also gets rid of some functions that only had one caller, as well as a bunch of dump()s.

Comment 13

[Phil Ringnalda (:philor)]

Comment on attachment 312899 [details] [diff] [review]
the gvars

>-var gAccountManager;

Breaks SMIME in http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mail/extensions/smime/content/msgCompSMIMEOverlay.js&rev=1.4&mark=154#152

>-var gPromptService;

Breaks (well, bends into window.alert()) http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mail/components/compose/content/addressingWidgetOverlay.js&rev=1.14&mark=874#874 doesn't it?

>-var gIsOffline;

Please terminate http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mail/components/addrbook/content/abCommon.js&rev=1.29&mark=1130,1164#1130 with extreme prejudice. Err, I mean, fix it.

>-          if (gIOService && gIOService.offline)
>+          var ioSvc = Components.classes["@mozilla.org/network/io-service;1"]
>+                                .getService(Components.interfaces.nsIIOService);
>+          if (ioService.offline)

Error: ioService is not defined

>-function SetupCommandUpdateHandlers()
>-{
>-  top.controllers.insertControllerAt(0, defaultController);
>-}
>-

We have this same strange thing in the messagewindow and the 3pane (where it's extra-evil, calling into a different file), and in SM, too - please leave it, and I'll kill all six at once in another bug.

>+    var promptSvc = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                              .getService(Components.interfaces.nsIPromptService);
>+    promptSvce.alert(window, errorTitle, errorMsg);

Error: promptSvce has no properties. (Well, probably a "is not a function", but it would make things clearer if it was no properties.)

>-function GetMsgSubjectElement()

Breaks http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mail/extensions/smime/content/msgCompSMIMEOverlay.js&rev=1.4&mark=315#310

I'm still not absolutely convinced that getting the account manager service eight times, and the prompt service seven, is really the right thing to do, so if you've got any more persuasion on that, bring it on.

Comment 14

[Joey Minta]

Attachment: better use of objects

This is a lot of the same gvar fixes from before (but minus the global-services), and instead includes a bunch of moves of 1-caller functions, usually inside an observer object.

Comment 15

[Joey Minta]

Comment on attachment 347159 [details] [diff] [review]
better use of objects

This needs more work actually...

Comment 16

[Joey Minta]

Attachment: better use of objects v2

This one will actually send attachments.

Comment 17

[Phil Ringnalda (:philor)]

Comment on attachment 347161 [details] [diff] [review]
better use of objects v2

I kept thinking that keeping this review request around would make me feel guilty enough to unrot the patch, but it turns out it's just reminding me of how much I suck without actually making me do anything.

Comment 18

[Wayne Mery (:wsmwk)]

Another one

Comment 19

[Magnus Melin [:mkmelin]]

Not worth keeping after 12 yrs.