421746 - lots of unused strings in phishing-afterload-warning-message.dtd

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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]

Sounds like a Good First Bug[TM] to me.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Remove unused strings from phishing-afterload-warning-message.dtd

Comment 3

[Justin Dolske [:Dolske]]

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]]

Comment on attachment 717912 [details] [diff] [review]
Remove unused strings from phishing-afterload-warning-message.dtd

># HG changeset patch
># User mikedeboer <[e-mail]>
># 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]]

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]]

Attachment: proper fix that removes unused strings from all DTDs

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da4d55e4d679

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/da4d55e4d679