Closed
Bug 421746
Opened 13 years ago
Closed 8 years ago
lots of unused strings in phishing-afterload-warning-message.dtd
Categories
(Toolkit :: Safe Browsing, defect)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
Firefox 22
People
(Reporter: Gavin, Assigned: mikedeboer)
Details
(Whiteboard: [GFB][mentor=gavin][lang=DTD])
Attachments
(1 file, 1 obsolete file)
9.91 KB,
patch
|
Dolske
:
review+
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Sounds like a Good First Bug[TM] to me.
Whiteboard: [GFB][mentor=gavin][lang=DTD]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mdeboer
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #717912 -
Flags: review?
Assignee | ||
Updated•8 years ago
|
Attachment #717912 -
Flags: review? → review?(dolske)
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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•8 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•8 years ago
|
||
Attachment #717912 -
Attachment is obsolete: true
Attachment #718391 -
Flags: review?(dolske)
Reporter | ||
Comment 7•8 years ago
|
||
(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•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #718391 -
Flags: review?(mark.finkle) → review+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 9•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/da4d55e4d679
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/da4d55e4d679
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Updated•7 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•