Closed
Bug 416666
Opened 17 years ago
Closed 17 years ago
Clean up Thunderbird's global scope a bit
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jminta, Assigned: jminta)
References
Details
Attachments
(2 files, 1 obsolete file)
5.79 KB,
patch
|
philor
:
review+
|
Details | Diff | Splinter Review |
51.12 KB,
patch
|
philor
:
review+
|
Details | Diff | Splinter 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)
Assignee | ||
Comment 1•17 years ago
|
||
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•17 years ago
|
Attachment #302401 -
Flags: review?(dmose) → review?(philringnalda)
Assignee | ||
Updated•17 years ago
|
Attachment #302420 -
Flags: review?(dmose) → review?(philringnalda)
Comment 2•17 years ago
|
||
Comment on attachment 302401 [details] [diff] [review] patch v1 Code in mail/ doesn't require sr.
Attachment #302401 -
Flags: superreview?(dmose)
Updated•17 years ago
|
Attachment #302420 -
Flags: superreview?(dmose)
Comment 3•17 years ago
|
||
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 4•17 years ago
|
||
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•17 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•17 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
Comment 7•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 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•17 years ago
|
||
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 12•17 years ago
|
||
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•17 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
Closed: 17 years ago
Resolution: --- → FIXED
Comment 14•17 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•17 years ago
|
||
Thanks Joe! Just checked in a fix for that.
Comment 16•17 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 --------
You need to log in
before you can comment on or make changes to this bug.
Description
•