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

RESOLVED FIXED in Firefox 22

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: Gavin, Assigned: mikedeboer)

Tracking

Trunk
Firefox 22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [GFB][mentor=gavin][lang=DTD])

Attachments

(1 attachment, 1 obsolete attachment)

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

5 years ago
Sounds like a Good First Bug[TM] to me.
Whiteboard: [GFB][mentor=gavin][lang=DTD]
(Assignee)

Updated

5 years ago
Assignee: nobody → mdeboer
(Assignee)

Comment 2

5 years ago
Created attachment 717912 [details] [diff] [review]
Remove unused strings from phishing-afterload-warning-message.dtd
Attachment #717912 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #717912 - Flags: review? → review?(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.
Attachment #717912 - Flags: review?(dolske)
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
(Assignee)

Comment 5

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

Comment 6

5 years ago
Created attachment 718391 [details] [diff] [review]
proper fix that removes unused strings from all DTDs
Attachment #717912 - Attachment is obsolete: true
Attachment #718391 - Flags: review?(dolske)
(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 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. ;)
Attachment #718391 - Flags: review?(mark.finkle)
Attachment #718391 - Flags: review?(dolske)
Attachment #718391 - Flags: review+
Attachment #718391 - Flags: review?(mark.finkle) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/da4d55e4d679
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/da4d55e4d679
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.