Closed Bug 416666 Opened 15 years ago Closed 15 years ago

Clean up Thunderbird's global scope a bit


(Thunderbird :: General, defect)

Not set


(Not tracked)



(Reporter: jminta, Assigned: jminta)




(2 files, 1 obsolete file)

Attached patch patch v1Splinter Review
The attached patch removes several functions that have no callers, and several global variables that are unused.
Attachment #302401 - Flags: superreview?(dmose)
Attachment #302401 - Flags: review?(dmose)
Attached patch more cleanup v1 (obsolete) — Splinter Review
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
Attachment #302420 - Flags: superreview?(dmose)
Attachment #302420 - Flags: review?(dmose)
Attachment #302401 - Flags: review?(dmose) → review?(philringnalda)
Attachment #302420 - Flags: review?(dmose) → review?(philringnalda)
Comment on attachment 302401 [details] [diff] [review]
patch v1

Code in mail/ doesn't require sr.
Attachment #302401 - Flags: superreview?(dmose)
Attachment #302420 - Flags: superreview?(dmose)
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.
Attachment #302401 - Flags: review?(philringnalda) → review+
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 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[";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        = ";1";
>-var statusFeedbackContractID   = ";1";
> var mailSessionContractID      = ";1";
>-var msgWindowContractID      = ";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[";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.)
(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.
"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
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
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
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
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
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.
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 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/"/>
>   </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 fixed, and we'll be ready to be a lot less global.
Attachment #302420 - Flags: review?(philringnalda) → review-
(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.
Attached patch more cleanup v2Splinter Review
This should address the review comments, as well as removing a couple more contract-id declarations.
Attachment #302420 - Attachment is obsolete: true
Attachment #303943 - Flags: review?(philringnalda)
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 :)
Attachment #303943 - Flags: review?(philringnalda) → review+
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
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
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
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
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
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
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
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
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
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
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
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: TB2SM
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.
Thanks Joe!  Just checked in a fix for that.
Depends on: 418610
Depends on: 419184
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
Depends on: 419555
Depends on: 425825
Depends on: 425756
Depends on: 424764
Depends on: 452661
Blocks: 770842
No longer blocks: TB2SM
You need to log in before you can comment on or make changes to this bug.