If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Clean up Thunderbird's global scope a bit

RESOLVED FIXED

Status

Thunderbird
General
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: Joey Minta, Assigned: Joey Minta)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 302401 [details] [diff] [review]
patch v1

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)
(Assignee)

Comment 1

10 years ago
Created attachment 302420 [details] [diff] [review]
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
Attachment #302420 - Flags: superreview?(dmose)
Attachment #302420 - Flags: review?(dmose)
(Assignee)

Updated

10 years ago
Attachment #302401 - Flags: review?(dmose) → review?(philringnalda)
(Assignee)

Updated

10 years ago
Attachment #302420 - Flags: review?(dmose) → review?(philringnalda)

Comment 2

10 years ago
Comment on attachment 302401 [details] [diff] [review]
patch v1

Code in mail/ doesn't require sr.
Attachment #302401 - Flags: superreview?(dmose)

Updated

10 years ago
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 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.)
(Assignee)

Comment 5

10 years ago
(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
(Assignee)

Comment 6

10 years ago
"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
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/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.
Attachment #302420 - Flags: review?(philringnalda) → review-
(Assignee)

Comment 10

10 years ago
(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.
(Assignee)

Comment 11

10 years ago
Created attachment 303943 [details] [diff] [review]
more cleanup v2

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+
(Assignee)

Comment 13

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Blocks: 360488

Comment 14

10 years ago
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.
(Assignee)

Comment 15

10 years ago
Thanks Joe!  Just checked in a fix for that.

Updated

10 years ago
Depends on: 418610

Updated

10 years ago
Depends on: 419184

Comment 16

10 years ago
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

Updated

10 years ago
Depends on: 425756

Updated

10 years ago
Depends on: 424764

Updated

9 years ago
Depends on: 452661

Updated

5 years ago
Blocks: 770842

Updated

5 years ago
No longer blocks: 360488
You need to log in before you can comment on or make changes to this bug.