461112 - Remove obsolete/unused entities in Thunderbird string files

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Simon Paquet [:sipaq]]

Using an optimized version (from Philipp Kewisch) of the script mentioned in http://www.chrisfinke.com/2008/04/22/finding-unused-entities-in-your-firefox-extensions/ I was able to identify lots of unused strings in Thunderbird.

Those should be removed to lessen the localization burden for new localizers and to reduce the build size.

Comment 1

[Simon Paquet [:sipaq]]

Attachment: Removal patch for .dtd files

This is the script I used for finding these entities:

#!/bin/bash

nonlocales=
locales=

for i in $*
do
    nonlocales="`find $i -maxdepth 1 -type d -not -name locales -not -name . -not -name $i` $nonlocales"
    if [ -d "$i/locales" ]
    then 
        locales="$locales $i/locales"
    fi
done


echo "Searching $locales in $nonlocales"

echo "Unused entities:"

for dtdfile in `find $locales -name '*.dtd'`
do
    awk '/<!ENTITY/ {print $2}' < $dtdfile | while read line
    do
        search=`grep -r "${line}" $nonlocales`
        
        if [ "$search" == "" ]
        then
            if [ "$hasdisplayedname" == "" ]
            then
               echo "+++ $dtdfile"
               hasdisplayedname=1
            fi
            echo "${line}";
        fi
    done;
done;

echo ""
echo "Unused properties:"

for propfile in `find $locales -name '*.properties'`
do
    awk -F "=" '{if (!($2 == "")) { print $1 }}' < $propfile | while read line
    do
        search=`grep -r "${line}" $nonlocales`
        if [ "$search" == "" ]
        then
            if [ "$hasdisplayedname" == "" ]
            then
               echo "+++ $propfile"
               hasdisplayedname=1
            fi
            echo "${line}"
        fi
    done
done

Comment 2

[Phil Ringnalda (:philor)]

Whee, unused entities! (That l10n note for a string which has never been in the tree is lovely beyond belief.)

But, what command line did you use, and where? windowMenu.label is very much not unused, it's used in http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/macWindowMenu.inc#17 and not having it pretty much destroys the app on OS X.

Comment 3

[Phil Ringnalda (:philor)]

Before I forget in the excitement of tearing things out: please keep credit.memory - morbid though it is, if I die between string freeze and ship, I damn well expect the credits to say "In Fond Memory Of Phil Ringnalda" with no excuses like "well, we'd like to, of course we would, but after string freeze..."

davida: are any of those other credits strings things you plan on using, before we take them out and then have to put them right back in?

Comment 4

[Simon Paquet [:sipaq]]

(In reply to comment #2)
> But, what command line did you use, and where? windowMenu.label is very much
> not unused, it's used in
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/macWindowMenu.inc#17
> and not having it pretty much destroys the app on OS X.

I ran the following command "findentities.sh mail mailnews" (Beware! It takes a while). I must say I wasn't aware that we define strings in mail/locales, which are used in toolkit files.

BTW expect a second patch for obsolete strings in .properties files. However I still need to wade through the script output and remove some property names mentioned there, as the script is not able to detect property names, that are complied at runtime, , i.e

gSubscribeBundle.getString("subscribeLabel-" + serverType)

where susbscribeLabel-smtp, subscribeLabel-pop3 or subscribeLabel-IMAP are valid property strings.

Comment 5

[Simon Paquet [:sipaq]]

Attachment: Removal patch for .dtd files - v2

Updated patch with credits.memory and the Mac strings brought back.

Comment 6

[Phil Ringnalda (:philor)]

Comment on attachment 344522 [details] [diff] [review]
Removal patch for .dtd files - v2

Those all look like lovely things to kill, except...

>--- a/mail/locales/en-US/chrome/communicator/wallet/SignonViewer.dtd

That's actually either a false negative, or a false positive, depending on how you look at it - we build mozilla/extensions/wallet/signonviewer/, though we don't use it and it's actually been broken (for us) since 2005, but you're getting false results from the same strings used in viewpasswords, making you think half is used and half isn't, when either all or none are. Let's just leave it as it is, and remove it entirely when we switch over from wallet to toolkit's passwordmanager instead.

Comment 7

[Simon Paquet [:sipaq]]

(In reply to comment #6)
> >--- a/mail/locales/en-US/chrome/communicator/wallet/SignonViewer.dtd
> 
> That's actually either a false negative, or a false positive, depending 
> on how you look at it - we build mozilla/extensions/wallet/signonviewer/, 
> though we don't use it and it's actually been broken (for us) since 2005, 
> but you're getting false results from the same strings used in 
> viewpasswords, making you think half is used and half isn't, when either 
> all or none are. Let's just leave it as it is, and remove it entirely 
> when we switch over from wallet to toolkit's passwordmanager instead.

Well, I can of course do that, but if we don't use those strings at all, 
then it doesn't hurt us, if we remove some of them right now, correct?

It doesn't even hurt us, if we remove all of the strings in that file and IMO 
we should do that, since it not only saves us codesize, but it makes it less 
hard for new localizers to localize Thunderbird, as less strings means less 
work for them.

Comment 8

[Phil Ringnalda (:philor)]

Removing some absolutely hurts: you force every locale (that's keeping up) to remove half the lines in a file and commit to get green, when we know it's unused because it's unusable and they'll be doing an hg rm before long.

Removing all is fine by me, but doing that before we make the switch involves not just removing the file and the jar.mn lines, but also looking into whether or not we want to build anything in signonviewer, and #ifdeffing us out of some or all of it, and since we probably don't actually want the .xpt we're shipping, unpackaging and removed-filesing it. New bug for that if you decide to, please.

Comment 9

[Mark Banner (:standard8)]

Re removing the wallet strings - you'll actually be removing them from cvs (as that is where mozilla/extensions/wallet is) and you may therefore break someone depending on the 1.9 wallet code (for whatever reason). Unlikely but possible.

Comment 10

[Simon Paquet [:sipaq]]

(In reply to comment #9)
> Re removing the wallet strings - you'll actually be removing them from cvs (as
> that is where mozilla/extensions/wallet is) and you may therefore break someone
> depending on the 1.9 wallet code (for whatever reason). Unlikely but possible.

Mark, we're not talking about removing strings in extensions/wallet, just on our side in mail/locales/en-US/chrome/communicator/wallet

Comment 11

[Mark Banner (:standard8)]

Opps I forgot that. Well the password manager work is in progress now, although I'm not sure how far away it is realistically, but I'd expect it to be done before beta 2 at the very latest, so why not just hold off on the wallet strings for a bit, rather than giving the localizers two tasks for updating?

Comment 12

[Simon Paquet [:sipaq]]

Phil, could you or someone else please do the checkin for me?

Comment 13

[Phil Ringnalda (:philor)]

Sure, but please don't put checkin-needed on a bug that has r+-if-you-change-this unless you attach a patch for checkin with this-changed, since otherwise someone's going to just check in the patch that *is* attached.

Comment 14

[Phil Ringnalda (:philor)]

Attachment: Removal patch for .dtd files (as checked in)

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

Comment 15

[Simon Paquet [:sipaq]]

Attachment: Removal patch for .properties files

This is the patch covering the entities in .properties strings.

This patch only removes roughly 40%-50% of the strings mentioned when running the script from comment 1, because as pointed out in comment 4, the script does not catch strings that are constructed at runtime. I also refrained from removing cruft from wallet string files.

I also ran the script over editor/ui, but no unused strings were identified there.

Comment 16

[Joey Minta]

Comment on attachment 345005 [details] [diff] [review]
Removal patch for .properties files

-# mailCommands.js
-emptyJunkTitle=Confirm
-emptyJunkMessage=Are you sure you want to permanently delete all messages and subfolders in the Junk folder?
-emptyJunkDontAsk=Don't ask me again.
-emptyTrashTitle=Confirm
-emptyTrashMessage=Are you sure you want to permanently delete all messages and subfolders in the Trash folder?
-emptyTrashDontAsk=Don't ask me again.

My understanding is that these entities are constructed here: http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#450
(emptyJunk and emptyTrash are the two commands that will be passed in)

Comment 17

[Simon Paquet [:sipaq]]

(In reply to comment #16)
> My understanding is that these entities are constructed here:
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#450

You're right. Phil, please ignore these removals. Thanks for finding this, Joey.

Comment 18

[Phil Ringnalda (:philor)]

Comment on attachment 345005 [details] [diff] [review]
Removal patch for .properties files

>     <stringbundle id="bundlePreferences" src="chrome://messenger/locale/preferences/preferences.properties"/>
>
> 
>
> #ifdef HAVE_SHELL_SERVICE
>
>-    <stringbundle id="bundleShell" src="chrome://messenger/locale/shellservice.properties"/>
>
>+    <stringbundle id="bundleShell" src="chrome://messenger/locale/preferences/preferences.properties"/>

Ick. Rather than loading the same stringbundle twice with different names, let's fix general.js to just use bundlePreferences to get the strings it wants for checkDefaultNow().

Comment 19

[Simon Paquet [:sipaq]]

Ugh, I totally missed the first stringbundle. Expect a new patch with the issues raised by you and Joey.

Comment 20

[Vlado Valastiak [:wladow]]

Attachment: remove forgotten comments for dtds [checked in]

these 2 comments were not removed with corresponding entities

Comment 21

[Phil Ringnalda (:philor)]

Comment on attachment 345076 [details] [diff] [review]
remove forgotten comments for dtds [checked in]

Thanks for doing a better review than I did!

Comment 22

[Simon Paquet [:sipaq]]

Attachment: Removal patch for .properties files - v2

New patch with review comments from philor and jminta incorporated.

Comment 23

[Phil Ringnalda (:philor)]

Comment on attachment 345076 [details] [diff] [review]
remove forgotten comments for dtds [checked in]

http://hg.mozilla.org/comm-central/rev/6be903922c66

Comment 24

[David Ascher (:davida)]

phil: can you give me a compact view of what credit strings you're worried about? We need to do a Tb3 credits bug somewhere, but i don't think this bug is it.

Comment 25

[Phil Ringnalda (:philor)]

davida: too late, I already decided the right thing for you. It was:

-<!ENTITY credit.core      "Core Development Posse">
-<!ENTITY credit.gecko     "Gecko Layout Engine">
-<!ENTITY credit.visuals     "Visual Design Coordinator">
-<!ENTITY credit.brand       "Brand Identity">
-<!ENTITY credit.web         "Web Design">
-<!ENTITY credit.update      "Mozilla Update">
-<!ENTITY credit.qalead      "Quality Assurance Lead">
-<!ENTITY credit.infra       "Infrastructure Support">
-<!ENTITY credit.support     "Support Resources">
-<!ENTITY credit.manage      "Project Management">
-<!ENTITY credit.marketleads "Marketing Leads">
-<!ENTITY credit.market      "Marketing">
-<!ENTITY credit.creators    "Created By">

which were unused but intended to be used like the existing "Engineering Leads" and "Quality Assurance" and "Build and Release" headings. But because they've been translated unused for so many years, even if you do decide to use some of those as headings the right thing to do is to do them new, rather than suddenly using something with no way for localizers to know that they now need to be sure their translation of something that they did in 2004 was correct and fits in the width of the dialog and all. (Though I'd vote for just going with the Fx-style one big list without headings.)

(Your credit updating bug is bug 442222, which I thought I'd assigned to you, but at least Magnus did get it blocking+.)

Comment 26

[Phil Ringnalda (:philor)]

Comment on attachment 345132 [details] [diff] [review]
Removal patch for .properties files - v2

> # LOCALIZATION NOTE: Do not translate anything in this file (except for "MsgMdnWishToSend")
> #                    see bugzilla bug 139615.
> ## Msg Mdn Report strings
> MsgMdnDisplayed=Note: This Return Receipt only acknowledges that the message was displayed on the recipient's computer. There is no guarantee that the recipient has read or understood the message contents.
>-MsgMdnDispatched=The message was either printed, faxed, or forwarded without being displayed to the recipient. There is no guarantee that the recipient will read the message at a later time.

That's a false positive (or something...) - see bug 462697. Then there's the fun of that l10n note and its bug 335534 history; given the current state of localized, not localized, localized in SM but not Tb, localized but with the English string included too, I'm not quite sure how we handle that. Two or three dozen separate bugs for an unloved feature, maybe.

Comment 27

[Phil Ringnalda (:philor)]

Attachment: .properties for checkin [checked in]

Other than the mdn thing (removed from this patch), looks good. Better than the tree does right now, or I'd be saying "as checked in" instead of "for".

Comment 28

[Phil Ringnalda (:philor)]

Comment on attachment 345908 [details] [diff] [review]
.properties for checkin [checked in]

http://hg.mozilla.org/comm-central/rev/31c705ad9560

Comment 29

[Simon Paquet [:sipaq]]

Marking FIXED per the last checkin.

Result:
- 53 entities in .properties files were removed
- 169 entities in .dtd files were removed
- some empty lines, localization notes and string inclusions were removed as well

That should make life easier for new localizers.

Comment 30

[Phil Ringnalda (:philor)]

Pushed http://hg.mozilla.org/comm-central/rev/c1d2db709168 as a bustage fix - prefPanel-smime is actually used, it's just that the Security pane is the only account manager pane key that's built up from parts instead of used whole. And I'm such a crappy reviewer that I think I built with the patch in one objdir, then tested a build in a different objdir.

Comment 31

[Simon Paquet [:sipaq]]

Attachment: More strings to kill [Checkin: Comment 36]

Hey Phil, I found some strings to squash :)

Comment 32

[Simon Paquet [:sipaq]]

Comment on attachment 348386 [details] [diff] [review]
More strings to kill
[Checkin: Comment 36]

Phil, as promised, here are some (hopefully) helpful comments for your review.

>--- a/mail/locales/en-US/chrome/communicator/utilityOverlay.dtd
>+++ b/mail/locales/en-US/chrome/communicator/utilityOverlay.dtd
> 
>-<!ENTITY closeWindow.label "Close Window">

Added in bug 362138, which added strings for tabs, but never added the feature.

>--- a/mail/locales/en-US/chrome/messenger-smime/msgSecurityInfo.properties
>+++ b/mail/locales/en-US/chrome/messenger-smime/msgSecurityInfo.properties
>
>-EINoDecryptCert=The certificate used to encrypt the message cannot be found.
>-EIPasswordError=You did not enter your Master Password correctly.
>-EIInvalidCipher=The message was encrypted using an encryption strength that this version of your software does not support.
>-
>-## Signing Power Information string
>-SPCanLabel=Message Can Be Signed
>-SPCanHeader=You have chosen to digitally sign this message before sending it.
>-SPCan=When other people receive your signed message, they can verify that the message comes from you and that it has not been altered since you signed it.
>-
>-SPCannotLabel=Message Cannot Be Signed
>-SPCannotHeader=You cannot digitally sign this message.
>-
>-SPNoCert=You have not specified a valid certificate for creating digital signatures.
>-SPNoSigCert=The certificate you have specified for signing messages cannot be used for that purpose.
>-SPNoValidCert=The certificate you have specified for signing messages is not yet valid. Make sure your computer's clock is set correctly.
>-SPExpiredCert=The certificate you have specified for signing messages has expired.
>-SPRevokedCert=The certificate you have specified for signing messages has been revoked.
>-
>-## Encryption Power Information string
>-EPCanLabel=Message Can Be Encrypted
>-EPCanHeader=You have chosen to encrypt this message before sending it.
>-EPCan=If you have chosen to save copies of your outgoing messages, this message will be encrypted before being saved.
>-
>-EPCannotLabel=Message Cannot Be Encrypted
>-EPCannotHeader=This message cannot be encrypted.
>-
>-EPRecipientUnknown=You have not entered any recipients.
>-EPNoCerts=You do not have valid certificates for the following recipients:
>-EPClueless=There are unknown problems with this message.

Added back in 2005 with Bug 282399, unused since then as can be seen by http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/extensions/smime/resources/content/msgReadSecurityInfo.js&rev=1.1&root=/cvsroot and all its following versions.

>--- a/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
>+++ b/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
>
>-<!ENTITY Internet.box                   "Internet">

Obsoleted by the fix for bug 63941.

Comment 33

[Simon Paquet [:sipaq]]

Phil, anything more that you need from me aside from the information in comment 32 to complete the review of attachment 348386 [details] [diff] [review]?

Comment 34

[Phil Ringnalda (:philor)]

Comment on attachment 348386 [details] [diff] [review]
More strings to kill
[Checkin: Comment 36]

They certainly *seem* unused: fingers crossed that this bunch all really are :)

Comment 35

[Vlado Valastiak [:wladow]]

(In reply to comment #34)
> (From update of attachment 348386 [details] [diff] [review])
> They certainly *seem* unused: fingers crossed that this bunch all really are :)

Well, these strings were removed from /suite a month ago and there's no complaint so far, so go on ;)

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 348386 [details] [diff] [review]
More strings to kill
[Checkin: Comment 36]

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