Open Bug 440616 Opened 12 years ago Updated 7 years ago

Clean up Thunderbird's global scope a bit more

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: jminta, Assigned: jminta)

References

Details

(Keywords: student-project, Whiteboard: [patchlove])

Attachments

(9 files, 4 obsolete files)

Attached patch part 1 v1 (obsolete) — 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)
Attached patch part 2 v1Splinter Review
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)
Attached patch part 3 v1Splinter Review
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)
Attached patch part 4 v1Splinter Review
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)
Attached patch part 5 v1Splinter Review
This patch addresses junkCommands.js and shareglue.js
Attachment #326109 - Flags: superreview?(bienvenu)
Attachment #326109 - Flags: review?(bienvenu)
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 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+
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
Attachment #326109 - Flags: superreview?(bienvenu)
Attachment #326109 - Flags: superreview+
Attachment #326109 - Flags: review?(bienvenu)
Attachment #326109 - Flags: review+
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
Blocks: 441750
No longer blocks: 441750
Depends on: 441750
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+
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
Depends on: 441987
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)
Attachment #326095 - Flags: review?(philringnalda)
Attached patch part 1 v2Splinter Review
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)
Attached patch part 6 v1 (obsolete) — Splinter Review
This takes care of a bunch of nsMsgFolderFlags constants, now that they're in an idl.
Attachment #336864 - Flags: review?(philringnalda)
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 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.
Attachment #336850 - Flags: review?(philringnalda) → review+
Attachment #336864 - Flags: review?(philringnalda) → review+
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
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 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+
Attached patch command cleanup2 v1 (obsolete) — Splinter Review
Same idea as the last one, but for mail3PaneWindowCommands.js
Attachment #345943 - Flags: review?(philringnalda)
command cleanup v1 was landed as rev da8475453ed5.
Same as before, but for msgMail3PaneWindow.js
Attachment #345973 - Flags: review?(philringnalda)
Depends on: 463786
Attached patch searchBar.jsSplinter Review
Attachment #347164 - Flags: review?(mkmelin+mozilla)
Comment on attachment 345943 [details] [diff] [review]
command cleanup2 v1

(Waiting on one that will apply.)
Attachment #345943 - Flags: review?(philringnalda)
Attachment #345973 - Flags: review?(philringnalda)
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+
Attached patch command cleanup2 v2 (obsolete) — Splinter Review
This one should apply to tip.
Attachment #345943 - Attachment is obsolete: true
Attachment #348196 - Flags: review?(philringnalda)
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 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-
Whiteboard: [patchlove]
Joey is probably no longer working on these?

Smells like easy student-project fodder.
Keywords: student-project
This could use marking patches that were already checked in.
You need to log in before you can comment on or make changes to this bug.