Last Comment Bug 421746 - lots of unused strings in phishing-afterload-warning-message.dtd
: lots of unused strings in phishing-afterload-warning-message.dtd
Status: RESOLVED FIXED
[GFB][mentor=gavin][lang=DTD]
:
Product: Toolkit
Classification: Components
Component: Safe Browsing (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 22
Assigned To: Mike de Boer [:mikedeboer]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-08 16:53 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2014-05-27 12:25 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove unused strings from phishing-afterload-warning-message.dtd (1.70 KB, patch)
2013-02-25 08:58 PST, Mike de Boer [:mikedeboer]
no flags Details | Diff | Review
proper fix that removes unused strings from all DTDs (9.91 KB, patch)
2013-02-26 05:31 PST, Mike de Boer [:mikedeboer]
dolske: review+
mark.finkle: review+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2008-03-08 16:53:49 PST
Damjan pointed out on IRC that there appears to be some string duplication between browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd and browser/locales/en-US/chrome/browser/safebrowsing/blockedSite.properties . 

I noticed that most of the strings in phishing-afterload-warning-message.dtd appear to be unused. I guess they weren't removed in the patch for bug 400324 (and in fact this is probably the bug that Ryan said he would file in bug 400324#c3 comment 3).
Comment 1 Philip Chee 2013-02-04 02:40:00 PST
Sounds like a Good First Bug[TM] to me.
Comment 2 Mike de Boer [:mikedeboer] 2013-02-25 08:58:26 PST
Created attachment 717912 [details] [diff] [review]
Remove unused strings from phishing-afterload-warning-message.dtd
Comment 3 Justin Dolske [:Dolske] 2013-02-26 00:34:09 PST
Comment on attachment 717912 [details] [diff] [review]
Remove unused strings from phishing-afterload-warning-message.dtd

Wow. There are _4_ copies of this file in the tree. :(

browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd
browser/metro/locales/en-US/chrome/phishing.dtd
mobile/android/locales/en-US/chrome/phishing.dtd
mobile/xul/locales/en-US/chrome/phishing.dtd

The latter 3 are all identical; phishing-afterload-warning-message.dtd has some wording changes in safeb.blocked.phishingPage.title + .shortDesc + .longDesc.

Ideally we'd have this in a shared location and use the same strings for everything. Let's not solve that here, but let's also keep the differences in these... 4, sigh... files minimal.

So: please remove these entities from the other files too.
Comment 4 Justin Dolske [:Dolske] 2013-02-26 00:41:00 PST
Comment on attachment 717912 [details] [diff] [review]
Remove unused strings from phishing-afterload-warning-message.dtd

># HG changeset patch
># User mikedeboer <info@mikedeboer.nl>
># Date 1361810777 -3600
># Node ID cf107a464e5151ca6191ce50dfb7368618bbfc21
># Parent  a0a2f97ef16cd95350ad9750cb542ed2f3fb740c

Kudos for getting all the useful info into the patch header, so that whoever eventually lands this for you doesn't have to add it manually.

But a couple tiny nits that might have escaped you:

* You probably want to use "Mike de Boer" instead of "mikedeboer"
* I'd also suggest using your actual Hg account (@mozilla.com) instead of mikedeboer.nl for consistency. (And if you want your commits labeled as mikedeboer.nl, I believe you can change your Hg account to be that instead of your MoCo account. Fine either way, just personal preference)

See https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Configuration
Comment 5 Mike de Boer [:mikedeboer] 2013-02-26 03:56:21 PST
Justin: I just filed bug 845269 to change my public key to use the proper email address and changed my Hg settings. The next patch will contain the proper info.
+ thanks for pointing out that I am 'allowed' to take a broader scope and change the other DTDs as well. Since they're part of another subrepo (metro, android)  I wasn't sure if it was my place to take it. I will now.
Comment 6 Mike de Boer [:mikedeboer] 2013-02-26 05:31:39 PST
Created attachment 718391 [details] [diff] [review]
proper fix that removes unused strings from all DTDs
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-26 09:12:28 PST
(In reply to Justin Dolske [:Dolske] from comment #4)
> * I'd also suggest using your actual Hg account (@mozilla.com) instead of
> mikedeboer.nl for consistency. (And if you want your commits labeled as
> mikedeboer.nl, I believe you can change your Hg account to be that instead
> of your MoCo account. Fine either way, just personal preference)

FWIW, there's no need to have your HG "user name" setting (the thing that shows up in patches) match your LDAP account/SSH username. The former shows up in HG logs/patches, the latter in pushlog when you push, but many people use different "identities" for those.
Comment 8 Justin Dolske [:Dolske] 2013-02-26 15:38:33 PST
Comment on attachment 718391 [details] [diff] [review]
proper fix that removes unused strings from all DTDs

This doesn't reeealy need additional review, but I'll bounce it over to mfinkle for the /mobile parts. Just to do it by the book. ;)
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-02-27 13:50:51 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/da4d55e4d679
Comment 10 Gregory Szorc [:gps] 2013-02-28 10:03:03 PST
https://hg.mozilla.org/mozilla-central/rev/da4d55e4d679

Note You need to log in before you can comment on or make changes to this bug.