Closed
Bug 440616
Opened 16 years ago
Closed 3 years ago
Clean up Thunderbird's global scope a bit more
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(thunderbird_esr78 unaffected)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
thunderbird_esr78 | --- | unaffected |
People
(Reporter: jminta, Assigned: jminta)
References
Details
(Keywords: student-project, Whiteboard: [patchlove])
Attachments
(9 files, 4 obsolete files)
18.42 KB,
patch
|
Details | Diff | Splinter Review | |
16.32 KB,
patch
|
philor
:
review+
|
Details | Diff | Splinter Review |
16.49 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
6.68 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
26.01 KB,
patch
|
philor
:
review+
|
Details | Diff | Splinter Review |
21.67 KB,
patch
|
philor
:
review+
|
Details | Diff | Splinter Review |
21.91 KB,
patch
|
Details | Diff | Splinter Review | |
10.71 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
9.68 KB,
patch
|
philor
:
review-
|
Details | Diff | Splinter Review |
Thunderbird's global scope still needs a bunch more cleaning up... Going to dump some patches here in the coming weeks. This first one takes out globals related to nsIMsgSession, as well as a bunch of 1-line functions and functions with only 1 caller. I know philor loves reviewing these. :-)
Attachment #325902 -
Flags: review?(philringnalda)
Assignee | ||
Comment 1•16 years ago
|
||
This one focuses on mailWindow.js, getting rid of the datasource global variables, gBrandBundle, and msgComposeService. Also includes the removal of a few more 1-line/1-call functions.
Attachment #326095 -
Flags: review?(philringnalda)
Assignee | ||
Comment 2•16 years ago
|
||
This one focuses on msgHdrViewOverlay, mostly getting rid of cached prefs. As a bonus, a lot of pref changes shouldn't require a restart anymore.
Attachment #326105 -
Flags: review?(philringnalda)
Assignee | ||
Comment 3•16 years ago
|
||
This one takes care of threadPane.js, because that's shared we need bienvenu's review.
Attachment #326107 -
Flags: superreview?(bienvenu)
Attachment #326107 -
Flags: review?(bienvenu)
Assignee | ||
Comment 4•16 years ago
|
||
This patch addresses junkCommands.js and shareglue.js
Attachment #326109 -
Flags: superreview?(bienvenu)
Attachment #326109 -
Flags: review?(bienvenu)
Comment 5•16 years ago
|
||
With part 1 depending on bug 441077, and part 2 depending on part 1, looks like I'll be waiting on 441077.
Depends on: 441077
Comment 6•16 years ago
|
||
Comment on attachment 326107 [details] [diff] [review] part 4 v1 nice
Attachment #326107 -
Flags: superreview?(bienvenu)
Attachment #326107 -
Flags: superreview+
Attachment #326107 -
Flags: review?(bienvenu)
Attachment #326107 -
Flags: review+
Assignee | ||
Comment 7•16 years ago
|
||
Part 4 has been checked in. Checking in mail/base/content/mailWindowOverlay.xul; /cvsroot/mozilla/mail/base/content/mailWindowOverlay.xul,v <-- mailWindowOverlay.xul new revision: 1.250; previous revision: 1.249 done Checking in mailnews/base/resources/content/mailWindowOverlay.xul; /cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.xul,v <-- mailWindowOverlay.xul new revision: 1.354; previous revision: 1.353 done Checking in mailnews/base/resources/content/threadPane.js; /cvsroot/mozilla/mailnews/base/resources/content/threadPane.js,v <-- threadPane.js new revision: 1.94; previous revision: 1.93 done
Updated•16 years ago
|
Attachment #326109 -
Flags: superreview?(bienvenu)
Attachment #326109 -
Flags: superreview+
Attachment #326109 -
Flags: review?(bienvenu)
Attachment #326109 -
Flags: review+
Assignee | ||
Comment 8•16 years ago
|
||
Part 5 has been checked in. Checking in mailnews/base/resources/content/junkCommands.js; /cvsroot/mozilla/mailnews/base/resources/content/junkCommands.js,v <-- junkCommands.js new revision: 1.2; previous revision: 1.1 done Checking in mailnews/base/resources/content/shareglue.js; /cvsroot/mozilla/mailnews/base/resources/content/shareglue.js,v <-- shareglue.js new revision: 1.31; previous revision: 1.30 done
Updated•16 years ago
|
Comment 9•16 years ago
|
||
Comment on attachment 326105 [details] [diff] [review] part 3 v1 Sorry, I forgot this one wasn't blocked until I realized I been running with it by almost including it in another patch. >-function CheckNotify() Please remove the "if ("FinishEmailProcessing" in this) FinishEmailProcessing(), too - it's the same NS6 AIM integration stuff.
Attachment #326105 -
Flags: review?(philringnalda) → review+
Assignee | ||
Comment 10•16 years ago
|
||
Part 3 checked in. Checking in mail/base/content/msgHdrViewOverlay.js; /cvsroot/mozilla/mail/base/content/msgHdrViewOverlay.js,v <-- msgHdrViewOverlay.js new revision: 1.106; previous revision: 1.105 done
Comment 11•16 years ago
|
||
Comment on attachment 325902 [details] [diff] [review] part 1 v1 Bound to be bit-rotted by now, and it looks like people were much more interested in stopping you than in doing something else to fix the blocker. Let me know when we're clear to go again.
Attachment #325902 -
Flags: review?(philringnalda)
Updated•16 years ago
|
Attachment #326095 -
Flags: review?(philringnalda)
Assignee | ||
Comment 12•16 years ago
|
||
This is part 1, unbitrotted and not depending on the search-dialog patch (I don't have near the energy to touch that bug again yet.).
Attachment #325902 -
Attachment is obsolete: true
Attachment #336850 -
Flags: review?(philringnalda)
Assignee | ||
Comment 13•16 years ago
|
||
This takes care of a bunch of nsMsgFolderFlags constants, now that they're in an idl.
Attachment #336864 -
Flags: review?(philringnalda)
Comment 14•16 years ago
|
||
Comment on attachment 336850 [details] [diff] [review] part 1 v2 Looks good at first glance, but as usual I'm too slow and it's too bitrotted to apply (at least, I think it's rot, and not depending on something else in your queue).
Comment 15•16 years ago
|
||
Comment on attachment 336864 [details] [diff] [review] part 6 v1 Would it be possible to pick the one true style (even if that's two styles, "use C.i.nsMsgFolderFlags.Foo if you're using one, const FLAGS and FLAGS.Foo if you're using more than one"), and use it everywhere? At least seeing them all in one place like this, the "that, plus sometimes a const for a particular flag, whether it's used twice or just once" style is a little confusing.
Updated•16 years ago
|
Attachment #336850 -
Flags: review?(philringnalda) → review+
Updated•16 years ago
|
Attachment #336864 -
Flags: review?(philringnalda) → review+
Assignee | ||
Comment 16•16 years ago
|
||
Comment on attachment 336864 [details] [diff] [review] part 6 v1 This is too rotten for me to feel confident about checking in right now. Will post an updated patch when I get some time.
Attachment #336864 -
Attachment is obsolete: true
Assignee | ||
Comment 17•16 years ago
|
||
This is mostly a cleanup of mailWindowOverlay.js, eliminating functions that are only called once (or never MsgHome, getMarkupDocumentViewer), or only pass a simple call on to another function.
Attachment #345870 -
Flags: review?(philringnalda)
Comment 18•16 years ago
|
||
Comment on attachment 345870 [details] [diff] [review] command cleanup v1 r=me, minus the trailing whitespace: http://beaufour.dk/jst-review/?patch=345870
Attachment #345870 -
Flags: review?(philringnalda) → review+
Assignee | ||
Comment 19•16 years ago
|
||
Same idea as the last one, but for mail3PaneWindowCommands.js
Attachment #345943 -
Flags: review?(philringnalda)
Assignee | ||
Comment 20•16 years ago
|
||
command cleanup v1 was landed as rev da8475453ed5.
Assignee | ||
Comment 21•16 years ago
|
||
Same as before, but for msgMail3PaneWindow.js
Attachment #345973 -
Flags: review?(philringnalda)
Assignee | ||
Comment 22•16 years ago
|
||
Attachment #347164 -
Flags: review?(mkmelin+mozilla)
Comment 23•16 years ago
|
||
Comment on attachment 345943 [details] [diff] [review] command cleanup2 v1 (Waiting on one that will apply.)
Attachment #345943 -
Flags: review?(philringnalda)
Updated•16 years ago
|
Attachment #345973 -
Flags: review?(philringnalda)
Comment 24•16 years ago
|
||
Comment on attachment 347164 [details] [diff] [review] searchBar.js Looks good! A few sentences could do with a period, and please spell javascript JavaScript. r=mkmelin with that. gStatusBar seems like a fine candidate to get rid of too... While you're touching searchBar.js, would you mind fixing the dump of "Search Exception" to include the exception, might help someone fix bug 464059 i found testing this.
Attachment #347164 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 25•16 years ago
|
||
This one should apply to tip.
Attachment #345943 -
Attachment is obsolete: true
Attachment #348196 -
Flags: review?(philringnalda)
Comment 26•15 years ago
|
||
Someone seems to have let this rot a bit...
Attachment #348196 -
Attachment is obsolete: true
Attachment #361114 -
Flags: review?(philringnalda)
Attachment #348196 -
Flags: review?(philringnalda)
Comment 27•15 years ago
|
||
Comment on attachment 361114 [details] [diff] [review] command cleanup2 v3 > case "cmd_find": > case "cmd_findAgain": > case "cmd_findPrevious": >- return IsMessageDisplayedInMessagePane(); >+ return (!IsMessagePaneCollapsed() && (GetNumSelectedMessages() > 0)); > break; I select one message by clicking, then shift+click a second, the message pane is blank, but I can cmd+f and fail to find anything to my heart's content. I suspect this means to be GetNumSelectedMessages() == 1, doesn't it? > function SetFocusThreadPaneIfNotOnMessagePane() > { > var focusedElement = WhichPaneHasFocus(); > >- if((focusedElement != GetThreadTree()) && >- (focusedElement != GetMessagePane())) >- SetFocusThreadPane(); >+ if ((focusedElement != GetThreadTree()) && >+ (focusedElement != GetMessagePane())) >+ document.getElementById('threadTree').focus(); > } I can live with it, but it feels a little weird to call GetThreadTree(), which does exactly document.getElementById("threadTree"), and then immediately after do document.getElementById('threadTree'). Either stick it in a local var, or at least use double-quotes (for all four times you get it with single quotes).
Attachment #361114 -
Flags: review?(philringnalda) → review-
Updated•14 years ago
|
Whiteboard: [patchlove]
Comment 28•13 years ago
|
||
Joey is probably no longer working on these? Smells like easy student-project fodder.
Keywords: student-project
Comment 29•11 years ago
|
||
This could use marking patches that were already checked in.
Comment 30•3 years ago
|
||
(In reply to :aceman from comment #29)
This could use marking patches that were already checked in.
Should a new bug be filed, after someone sorts out the above and sets TM?
Flags: needinfo?(mkmelin+mozilla)
Comment 31•3 years ago
|
||
After 12 years, not worth investigating what went where.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(mkmelin+mozilla)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•