458548 - Get rid of string-bundle globals

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: patch v1

There are a bunch of files that assume that various string-bundle global variables will have been set prior to various functions being called. Because this isn't well centralized, we end up having tons of people setting these variables just to be sure [1], which is silly. Moreover, my standard argument that getElementById is *very* fast applies, making these variables silly in the first place.

This patch removes gMessengerBundle and gBrandBundle. It also includes a bit of code to clean up how we open windows, but that part should be very straightforward.

Have to ask for sr since this global variable mess also breaks the "don't let common code depend on forked code" rule. :-(

Comment 1

[Phil Ringnalda (:philor)]

Comment on attachment 341784 [details] [diff] [review]
patch v1

r=me as long as you clean up the (yeah, you just moved it, but that means you own it) trailing whitespace and the easy long lines from http://beaufour.dk/jst-review/?patch=341784

Comment 2

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

bienvenu, sr ping?

Comment 3

[David :Bienvenu]

ah, an other bug that I wasn't cc'ed on, so I didn't see the r+ go by. Does this still apply?

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 341784 [details] [diff] [review]
patch v1

This patch is definitely rotted.  Regrettably, refactoring patches tend to rot quickly.  Even if they still apply, new code may have been introduced that the patch needs to cover...

Comment 5

[Ian Neal]

Attachment: Remove gMessengerBundle and gBrandBundle patch v2.0 [Backed out: comment 7 & 8]

Unbitrotted version of the patch

Comment 6

[Phil Ringnalda (:philor)]

Comment on attachment 536193 [details] [diff] [review]
Remove gMessengerBundle and gBrandBundle patch v2.0 [Backed out: comment 7 & 8]

Still seems like a good idea to me.

Comment 7

[Ian Neal]

Comment on attachment 536193 [details] [diff] [review]
Remove gMessengerBundle and gBrandBundle patch v2.0 [Backed out: comment 7 & 8]

http://hg.mozilla.org/comm-central/rev/4f13eb9c9536

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 536193 [details] [diff] [review]
Remove gMessengerBundle and gBrandBundle patch v2.0 [Backed out: comment 7 & 8]

I had to back this out because it was causing our Mac and Windows MozMill tests to be orange, but I couldn't identify the cause of the error:

http://hg.mozilla.org/comm-central/rev/f438ee543856

Comment 9

[Jim Porter (:squib)]

One issue is with this (I think):

+      item.label = document.getElementById("bundle_messenger")
+                           .getFormattedString("attachmentDisplayNameFormat",
+                                               [attachmentIndex, displayName]);

The label property won't work here, since item is an XBL element that hasn't been added to the DOM yet, so the XBL stuff hasn't loaded yet. You need to use setAttribute.

Comment 10

[Ian Neal]

Attachment: Fixed patch for labels not added to DOM

Changes since last version:
* Made sure setAttribute is used where the menuitem hasn't been added to the DOM yet.

Comment 11

[Jim Porter (:squib)]

Fair warning: I probably bitrotted this in bug 656045 (this bug bitrotted me when it had been checked in).

Comment 12

[Ian Neal]

Attachment: Unbitrotted non-DOM attached labels patch [Checked in: Comment 13 & Comment 15]

Changes since previous version:
* Unbitrotted following landing of patch from bug 656045.

Comment 13

[Ian Neal]

Comment on attachment 537225 [details] [diff] [review]
Unbitrotted non-DOM attached labels patch [Checked in: Comment 13 & Comment 15]

http://hg.mozilla.org/comm-central/rev/ae64e58a6575

Comment 14

[Jim Porter (:squib)]

Comment on attachment 537225 [details] [diff] [review]
Unbitrotted non-DOM attached labels patch [Checked in: Comment 13 & Comment 15]

Review of attachment 537225 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgHdrViewOverlay.js
@@ +1187,5 @@
>    var card = aCard || getCardForEmail(aEmailAddress).card;
>  
>    // If this address is one of the user's identities...
>    if (aEmailAddress == identity.email) {
> +    var brand = document.getElementById("bundle_messenger");

I think you mean "var bundle" here. This is currently breaking the "You" shorthand in message headers.

Comment 15

[Ian Neal]

(In reply to comment #14)
> Comment on attachment 537225 [details] [diff] [review] [review]
> Unbitrotted non-DOM attached labels patch [Checked in: Comment 13]
> 
> Review of attachment 537225 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/msgHdrViewOverlay.js
> @@ +1187,5 @@
> >    var card = aCard || getCardForEmail(aEmailAddress).card;
> >  
> >    // If this address is one of the user's identities...
> >    if (aEmailAddress == identity.email) {
> > +    var brand = document.getElementById("bundle_messenger");
> 
> I think you mean "var bundle" here. This is currently breaking the "You"
> shorthand in message headers.

Thanks for catching that, bustage fix pushed as http://hg.mozilla.org/comm-central/rev/228f42df2090