416666 - Clean up Thunderbird's global scope a bit

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Joey Minta]

Attachment: patch v1

The attached patch removes several functions that have no callers, and several global variables that are unused.

Comment 1

[Joey Minta]

Attachment: more cleanup v1

This one is a bit trickier, but still the same basic ideas.  Some key points:
  -removing global declarations of contract ids
  -I consolidated a lot of compose functions into one helper function
  -removed global declarations of composeType and composeFormat interfaces
  -removed one global pref object (still 2 others out there)
  -shareglue.js is worthless

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 302401 [details] [diff] [review]
patch v1

Code in mail/ doesn't require sr.

Comment 3

[Phil Ringnalda (:philor)]

Comment on attachment 302401 [details] [diff] [review]
patch v1

r++ - would review again!

My favorites are the ones that mxr.landfill says have been around completely unused since M10.

Comment 4

[Phil Ringnalda (:philor)]

Comment on attachment 302420 [details] [diff] [review]
more cleanup v1

>diff -r b98371ab2d8c mail/base/content/commandglue.js
>-    else if (gFakeAccountPageLoaded)
>-      UpdateMailToolbar("gFakeAccountPageLoaded");

It pains me to say it, but we need to leave the two bits of FakeAccount junk, until we can try to rip the whole FakeAccount thing out in a separate bug where we find out whether anyone has asked mscott or sspitzer how they are supposed to actually use http://www.mozilla.org/mailnews/arch/fakeaccounts.html in recent memory. (Though I suppose it's possible someone figured it out on their own, by tearing apart a copy of NS7.)

>diff -r b98371ab2d8c mail/base/content/contentAreaClick.js
>-  var pref = Components.classes["@mozilla.org/preferences-service;1"]
>-                       .getService(Components.interfaces.nsIPrefBranch);

Note half to myself: initMoveToFolderAgainMenu in mailWindowOverlay.js depends on "pref", but since hiddenWindow isn't calling CreateMailWindowGlobals, opening the Message menu without any open windows on OS X throws.

>diff -r b98371ab2d8c mail/base/content/mailWindow.js
>-var messengerContractID        = "@mozilla.org/messenger;1";
>-var statusFeedbackContractID   = "@mozilla.org/messenger/statusfeedback;1";
> var mailSessionContractID      = "@mozilla.org/messenger/services/session;1";
>-var msgWindowContractID      = "@mozilla.org/messenger/msgwindow;1";

If we really have to keep that one (and I'm not convinced, particularly since we don't use that var-const the one time in this same file that we use the contract ID), fewer spaces would be better.

>-var gFakeAccountPageLoaded = false;

Sigh, keepme please.

>-  messenger = Components.classes[messengerContractID].createInstance(Components.interfaces.nsIMessenger);
>+  messenger = Components.classes["@mozilla.org/messenger;1"].createInstance(Components.interfaces.nsIMessenger);

I don't insist on 80 char lines when it takes extreme gyrations, but when it's easy and you're touching it anyway like this, please shorten 'em.

>+  var acctCentralPage = pref.getComplexValue("mailnews.account_central_page.url",
>                                                Components.interfaces.nsIPrefLocalizedString).data;

Couple spaces past your allotted limit :)

>diff -r b98371ab2d8c mail/base/content/mailWindowOverlay.js
(Sadly, as far as I got, curse you, fakeaccount distractions.)

Comment 5

[Joey Minta]

(In reply to comment #4)
> It pains me to say it, but we need to leave the two bits of FakeAccount junk,
I left most of the fake-account stuff.  The reason I yanked this was that mxr seemed to say that no one ever changed that variable to true, thus, that branch never got called.
http://mxr.mozilla.org/seamonkey/search?string=gFakeAccountPageLoaded&find=&findi=&filter=&tree=seamonkey

Comment 6

[Joey Minta]

"patch v1" checked in.

Checking in mail/base/content/commandglue.js;
/cvsroot/mozilla/mail/base/content/commandglue.js,v  <--  commandglue.js
new revision: 1.87; previous revision: 1.86
done
Checking in mail/base/content/mail3PaneWindowCommands.js;
/cvsroot/mozilla/mail/base/content/mail3PaneWindowCommands.js,v  <--  mail3PaneWindowCommands.js
new revision: 1.42; previous revision: 1.41
done
Checking in mail/base/content/mailWindowOverlay.js;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.184; previous revision: 1.183
done
Checking in mail/base/content/msgMail3PaneWindow.js;
/cvsroot/mozilla/mail/base/content/msgMail3PaneWindow.js,v  <--  msgMail3PaneWindow.js
new revision: 1.142; previous revision: 1.141
done
Checking in mail/base/content/widgetglue.js;
/cvsroot/mozilla/mail/base/content/widgetglue.js,v  <--  widgetglue.js
new revision: 1.22; previous revision: 1.21
done

Comment 7

[Phil Ringnalda (:philor)]

Yep, it's not set in-tree, but then none of the fake account stuff is. My concern is that the invisible Step 38 in the imaginary documentation is to have myFakeAccountCentral.xul have an onload handler that sets gFakeAccountPageLoaded so that whatever mumble mumble with the command update and the observer notification.

I'm 100% behind the (Core) bug to remove the whole thing, the only thing that would stop me is someone adding a fully-functional fake account to the tree so it would be possible for us to tell whether we've broken it (personally, my guess would be it's been broken dozens of times since whenever NS used it, if they ever did), I just don't want to kill it with a thousand cuts.

Comment 8

[Phil Ringnalda (:philor)]

Oh, nevermind, it's already taken at least the first 400 cuts, since only qute packages the CSS, and we don't actually apply it anywhere. It's already an ex-fake-account.

Comment 9

[Phil Ringnalda (:philor)]

Comment on attachment 302420 [details] [diff] [review]
more cleanup v1

>+++ b/mail/base/content/messageWindow.xul	Sun Feb 10 11:11:40 2008 -0500
>@@ -74,7 +74,6 @@
>     <stringbundle id="bundle_offlinePrompts" src="chrome://messenger/locale/offline.properties"/>
>   </stringbundleset>
> 
>-  <script type="application/x-javascript" src="chrome://messenger/content/shareglue.js"/>
>   <script type="application/x-javascript" src="chrome://messenger/content/commandglue.js"/>
>   <script type="application/x-javascript" src="chrome://messenger/content/mailWindow.js"/>
>   <script type="application/x-javascript" src="chrome://messenger/content/messageWindow.js"/>
>diff -r b98371ab2d8c mail/base/content/messenger.xul
>--- a/mail/base/content/messenger.xul	Sat Feb 09 18:05:42 2008 -0500
>+++ b/mail/base/content/messenger.xul	Sun Feb 10 11:11:40 2008 -0500
>@@ -75,7 +75,6 @@
> 
> <script type="application/x-javascript" src="chrome://messenger/content/widgetglue.js"/>
> <script type="application/x-javascript" src="chrome://messenger/content/commandglue.js"/>
>-<script type="application/x-javascript" src="chrome://messenger/content/shareglue.js"/>
> <script type="application/x-javascript" src="chrome://messenger/content/msgViewNavigation.js"/>
> <script type="application/x-javascript" src="chrome://messenger/content/mailWindow.js"/>
> <script type="application/x-javascript" src="chrome://messenger/content/msgMail3PaneWindow.js"/>

Where were you planning on defining MessengerSetForcedCharacterSet() instead, that didn't make it into the patch? Right now, you've broken all of View - Character Encoding.

>+++ b/mail/base/content/utilityOverlay.js	Sun Feb 10 11:11:40 2008 -0500
>@@ -1,3 +1,5 @@
>+
>+
> /* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Seems like a bad choice of place for a couple of blank lines :)

>-var gShowBiDi = false;
>-

>-  if (gShowBiDi)
>-    goUpdateCommand('cmd_switchTextDirection');

Please leave these two, senseless though they seem - I'm pretty sure we'll want them when we get around to actually adding bidi UI (the odd-seeming global-false apparently makes sure that it's absolutely false in any window which doesn't explicitly set it true), and meantime I rather not risk messing with a dependency in the bidi-mailui extension.

Once more, without the stray spaces and blank lines, *with* the fake account stuff removed, but with working View - Character Encoding and at least the easy-to-break long lines in http://beaufour.dk/jst-review/?patch=302420 fixed, and we'll be ready to be a lot less global.

Comment 10

[Joey Minta]

(In reply to comment #9)
> Where were you planning on defining MessengerSetForcedCharacterSet() instead,
> that didn't make it into the patch? Right now, you've broken all of View -
> Character Encoding.
Oh, geez.  Why didn't I suspect that mail specific functions would have callers in toolkit?  How naive of me.  New patch coming soon.

Comment 11

[Joey Minta]

Attachment: more cleanup v2

This should address the review comments, as well as removing a couple more contract-id declarations.

Comment 12

[Phil Ringnalda (:philor)]

Comment on attachment 303943 [details] [diff] [review]
more cleanup v2

r=philringnalda, since...

>+  var acctCentralPage = pref.getComplexValue(prefName,
>                                                Components.interfaces.nsIPrefLocalizedString).data;

...I'm sure you'll remember to take my mortal enemy, those two extra spaces in the second line, out before you check in :)

Comment 13

[Joey Minta]

Patch "more cleanup v2" checked in with the nit picked.  I'm going to close this bug and open a new one if we find more easy global-scope wins.

Checking in mail/base/content/commandglue.js;
/cvsroot/mozilla/mail/base/content/commandglue.js,v  <--  commandglue.js
new revision: 1.88; previous revision: 1.87
done
Checking in mail/base/content/contentAreaClick.js;
/cvsroot/mozilla/mail/base/content/contentAreaClick.js,v  <--  contentAreaClick.js
new revision: 1.19; previous revision: 1.18
done
Checking in mail/base/content/mailCommands.js;
/cvsroot/mozilla/mail/base/content/mailCommands.js,v  <--  mailCommands.js
new revision: 1.38; previous revision: 1.37
done
Checking in mail/base/content/mailWindow.js;
/cvsroot/mozilla/mail/base/content/mailWindow.js,v  <--  mailWindow.js
new revision: 1.61; previous revision: 1.60
done
Checking in mail/base/content/mailWindowOverlay.js;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.185; previous revision: 1.184
done
Checking in mail/base/content/msgHdrViewOverlay.js;
/cvsroot/mozilla/mail/base/content/msgHdrViewOverlay.js,v  <--  msgHdrViewOverlay.js
new revision: 1.101; previous revision: 1.100
done
Checking in mail/base/content/msgMail3PaneWindow.js;
/cvsroot/mozilla/mail/base/content/msgMail3PaneWindow.js,v  <--  msgMail3PaneWindow.js
new revision: 1.143; previous revision: 1.142
done
Checking in mail/base/content/searchBar.js;
/cvsroot/mozilla/mail/base/content/searchBar.js,v  <--  searchBar.js
new revision: 1.46; previous revision: 1.45
done
Checking in mail/base/content/subscribe.js;
/cvsroot/mozilla/mail/base/content/subscribe.js,v  <--  subscribe.js
new revision: 1.12; previous revision: 1.11
done
Checking in mail/base/content/utilityOverlay.js;
/cvsroot/mozilla/mail/base/content/utilityOverlay.js,v  <--  utilityOverlay.js
new revision: 1.4; previous revision: 1.3
done
Checking in mail/extensions/newsblog/content/toolbar-icon.xul;
/cvsroot/mozilla/mail/extensions/newsblog/content/toolbar-icon.xul,v  <--  toolbar-icon.xul
new revision: 1.10; previous revision: 1.9
done

Comment 14

[Joe Sabash [:JoeS1]]

Houry build:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008021816 Thunderbird/3.0a1pre ID:2008021816

"Edit as new" fails to open a compose window.

Error Console:
Error: event is not defined
Source File: chrome://messenger/content/mailWindowOverlay.js
Line: 1113

The prior Nightly was fine.

Comment 15

[Joey Minta]

Thanks Joe!  Just checked in a fix for that.

Comment 16

[Henrik Gemal]

caused bug 419184

----
when I drag a mail to Calendar item I get:

Error: msgHeaderParser is not defined
Source File: chrome://calendar/content/calendar-dnd-listener.js
Line: 62

using trunk tbird and trunk lightning
--------