440616 - Clean up Thunderbird's global scope a bit more

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: 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. :-)

Comment 1

[Joey Minta]

Attachment: 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.

Comment 2

[Joey Minta]

Attachment: 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.

Comment 3

[Joey Minta]

Attachment: part 4 v1

This one takes care of threadPane.js, because that's shared we need bienvenu's review.

Comment 4

[Joey Minta]

Attachment: part 5 v1

This patch addresses junkCommands.js and shareglue.js

Comment 5

[Phil Ringnalda (:philor)]

With part 1 depending on bug 441077, and part 2 depending on part 1, looks like I'll be waiting on 441077.

Comment 6

[David :Bienvenu]

Comment on attachment 326107 [details] [diff] [review]
part 4 v1

nice

Comment 7

[Joey Minta]

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

Comment 8

[Joey Minta]

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

Comment 9

[Phil Ringnalda (:philor)]

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.

Comment 10

[Joey Minta]

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

[Phil Ringnalda (:philor)]

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.

Comment 12

[Joey Minta]

Attachment: 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.).

Comment 13

[Joey Minta]

Attachment: part 6 v1

This takes care of a bunch of nsMsgFolderFlags constants, now that they're in an idl.

Comment 14

[Phil Ringnalda (:philor)]

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

[Phil Ringnalda (:philor)]

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.

Comment 16

[Joey Minta]

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.

Comment 17

[Joey Minta]

Attachment: 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.

Comment 18

[Phil Ringnalda (:philor)]

Comment on attachment 345870 [details] [diff] [review]
command cleanup v1

r=me, minus the trailing whitespace: http://beaufour.dk/jst-review/?patch=345870

Comment 19

[Joey Minta]

Attachment: command cleanup2 v1

Same idea as the last one, but for mail3PaneWindowCommands.js

Comment 20

[Joey Minta]

command cleanup v1 was landed as rev da8475453ed5.

Comment 21

[Joey Minta]

Attachment: msgMail3Pane cleanup v1

Same as before, but for msgMail3PaneWindow.js

Comment 22

[Joey Minta]

Attachment: searchBar.js

Comment 23

[Phil Ringnalda (:philor)]

Comment on attachment 345943 [details] [diff] [review]
command cleanup2 v1

(Waiting on one that will apply.)

Comment 24

[Magnus Melin [:mkmelin]]

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.

Comment 25

[Joey Minta]

Attachment: command cleanup2 v2

This one should apply to tip.

Comment 26

[Phil Ringnalda (:philor)]

Attachment: command cleanup2 v3

Someone seems to have let this rot a bit...

Comment 27

[Phil Ringnalda (:philor)]

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).

Comment 28

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Joey is probably no longer working on these?

Smells like easy student-project fodder.

Comment 29

[:aceman]

This could use marking patches that were already checked in.

Comment 30

[Wayne Mery (:wsmwk)]

(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?

Comment 31

[Magnus Melin [:mkmelin]]

After 12 years, not worth investigating what went where.