Clean up Thunderbird's global scope a bit more

ASSIGNED
Assigned to

Status

Thunderbird
Mail Window Front End
ASSIGNED
10 years ago
5 years ago

People

(Reporter: Joey Minta, Assigned: Joey Minta)

Tracking

({student-project})

Trunk
student-project
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove])

Attachments

(9 attachments, 4 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 325902 [details] [diff] [review]
part 1 v1

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

10 years ago
Created attachment 326095 [details] [diff] [review]
part 2 v1

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

10 years ago
Created attachment 326105 [details] [diff] [review]
part 3 v1

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

10 years ago
Created attachment 326107 [details] [diff] [review]
part 4 v1

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

10 years ago
Created attachment 326109 [details] [diff] [review]
part 5 v1

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 6

10 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

10 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

10 years ago
Attachment #326109 - Flags: superreview?(bienvenu)
Attachment #326109 - Flags: superreview+
Attachment #326109 - Flags: review?(bienvenu)
Attachment #326109 - Flags: review+
(Assignee)

Comment 8

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

Comment 10

10 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

Updated

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

Comment 12

10 years ago
Created attachment 336850 [details] [diff] [review]
part 1 v2

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

10 years ago
Created attachment 336864 [details] [diff] [review]
part 6 v1

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

Comment 16

9 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

9 years ago
Created attachment 345870 [details] [diff] [review]
command cleanup v1

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

Comment 19

9 years ago
Created attachment 345943 [details] [diff] [review]
command cleanup2 v1

Same idea as the last one, but for mail3PaneWindowCommands.js
Attachment #345943 - Flags: review?(philringnalda)
(Assignee)

Comment 20

9 years ago
command cleanup v1 was landed as rev da8475453ed5.
(Assignee)

Comment 21

9 years ago
Created attachment 345973 [details] [diff] [review]
msgMail3Pane cleanup v1

Same as before, but for msgMail3PaneWindow.js
Attachment #345973 - Flags: review?(philringnalda)
Depends on: 463786
(Assignee)

Comment 22

9 years ago
Created attachment 347164 [details] [diff] [review]
searchBar.js
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 24

9 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

9 years ago
Created attachment 348196 [details] [diff] [review]
command cleanup2 v2

This one should apply to tip.
Attachment #345943 - Attachment is obsolete: true
Attachment #348196 - Flags: review?(philringnalda)
Created attachment 361114 [details] [diff] [review]
command cleanup2 v3

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-

Updated

7 years ago
Whiteboard: [patchlove]
Joey is probably no longer working on these?

Smells like easy student-project fodder.
Keywords: student-project

Comment 29

5 years ago
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.