Remove obsolete/unused entities in Thunderbird string files

RESOLVED FIXED in Thunderbird 3.0b2

Status

Thunderbird
General
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sipaq, Assigned: sipaq)

Tracking

Trunk
Thunderbird 3.0b2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

9 years ago
Created attachment 344375 [details] [diff] [review]
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
Attachment #344375 - Flags: review?(philringnalda)
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.
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?
(Assignee)

Comment 4

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

Comment 5

9 years ago
Created attachment 344522 [details] [diff] [review]
Removal patch for .dtd files - v2

Updated patch with credits.memory and the Mac strings brought back.
Attachment #344375 - Attachment is obsolete: true
Attachment #344522 - Flags: review?(philringnalda)
Attachment #344375 - Flags: review?(philringnalda)
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.
Attachment #344522 - Flags: review?(philringnalda) → review+
(Assignee)

Comment 7

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

Comment 10

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

Comment 12

9 years ago
Phil, could you or someone else please do the checkin for me?
Keywords: checkin-needed
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.
Keywords: checkin-needed
Created attachment 344839 [details] [diff] [review]
Removal patch for .dtd files (as checked in)

http://hg.mozilla.org/comm-central/rev/e6eac490c2dd
Attachment #344522 - Attachment is obsolete: true
Attachment #344839 - Flags: review+

Updated

9 years ago
Target Milestone: --- → Thunderbird 3.0b1
(Assignee)

Comment 15

9 years ago
Created attachment 345005 [details] [diff] [review]
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.
Attachment #345005 - Flags: review?(philringnalda)

Comment 16

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

Comment 17

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

Comment 19

9 years ago
Ugh, I totally missed the first stringbundle. Expect a new patch with the issues raised by you and Joey.
Created attachment 345076 [details] [diff] [review]
remove forgotten comments for dtds [checked in]

these 2 comments were not removed with corresponding entities
Attachment #345076 - Flags: review?(philringnalda)
Comment on attachment 345076 [details] [diff] [review]
remove forgotten comments for dtds [checked in]

Thanks for doing a better review than I did!
Attachment #345076 - Flags: review?(philringnalda) → review+
(Assignee)

Comment 22

9 years ago
Created attachment 345132 [details] [diff] [review]
Removal patch for .properties files - v2

New patch with review comments from philor and jminta incorporated.
Attachment #345005 - Attachment is obsolete: true
Attachment #345132 - Flags: review?(philringnalda)
Attachment #345005 - Flags: review?(philringnalda)
Comment on attachment 345076 [details] [diff] [review]
remove forgotten comments for dtds [checked in]

http://hg.mozilla.org/comm-central/rev/6be903922c66
Attachment #345076 - Attachment description: remove forgotten comments for dtds → remove forgotten comments for dtds [checked in]
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.
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+.)
Depends on: 462697
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.
Created attachment 345908 [details] [diff] [review]
.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".
Attachment #345132 - Attachment is obsolete: true
Attachment #345908 - Flags: review+
Attachment #345132 - Flags: review?(philringnalda)
Attachment #345908 - Attachment description: .properties for checkin → .properties for checkin [checked in]
Comment on attachment 345908 [details] [diff] [review]
.properties for checkin [checked in]

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

Comment 29

9 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
Depends on: 463003
Depends on: 463971
(Assignee)

Comment 31

9 years ago
Created attachment 348386 [details] [diff] [review]
More strings to kill
[Checkin: Comment 36]

Hey Phil, I found some strings to squash :)
Attachment #348386 - Flags: review?(philringnalda)
(Assignee)

Comment 32

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

Comment 33

9 years ago
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 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 :)
Attachment #348386 - Flags: review?(philringnalda) → review+
(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 ;)
Keywords: checkin-needed
Comment on attachment 348386 [details] [diff] [review]
More strings to kill
[Checkin: Comment 36]

http://hg.mozilla.org/comm-central/rev/4f114dacf59e
Attachment #348386 - Attachment description: More strings to kill → More strings to kill [Checkin: Comment 36]
Keywords: checkin-needed
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2
You need to log in before you can comment on or make changes to this bug.