1388233 - Do not use hard-coded 'Firefox' string in phishing-afterload-warning-message.dtd

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Henry Chang [:hchang]]

This is a followup bug for bug 1375277 to remove the use of hard coded 'Firefox' string. Use &brandShortName instead.

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

Review commit: https://reviewboard.mozilla.org/r/165914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165914/

Comment 2

[Francesco Lodolo [:flod]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

https://reviewboard.mozilla.org/r/165914/#review171068

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:37
(Diff revision 1)
>  <!ENTITY safeb.blocked.phishingPage.shortDesc2 "This web page at <span id='phishing_sitename'/> has been reported as a deceptive site and has been blocked based on your security preferences.">
>  <!ENTITY safeb.blocked.phishingPage.longDesc2 "<p>Deceptive sites are designed to trick you into doing something dangerous, like installing software, or revealing your personal information, like passwords, phone numbers or credit cards.</p><p>Entering any information on this web page may result in identity theft or other fraud.</p>">
>  
>  <!ENTITY safeb.blocked.harmfulPage.title "The site ahead may contain malware">
>  <!-- Localization note (safeb.blocked.harmfulPage.shortDesc) - Please don't translate the contents of the <span id="harmful_sitename"/> tag. It will be replaced at runtime with a domain name (e.g. www.badsite.com) -->
> -<!ENTITY safeb.blocked.harmfulPage.shortDesc "Firefox blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">
> +<!ENTITY safeb.blocked.harmfulPage.shortDesc "&brandShortName; blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">

At this point you need to change ID for the string
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:38
(Diff revision 1)
>  <!ENTITY safeb.blocked.phishingPage.longDesc2 "<p>Deceptive sites are designed to trick you into doing something dangerous, like installing software, or revealing your personal information, like passwords, phone numbers or credit cards.</p><p>Entering any information on this web page may result in identity theft or other fraud.</p>">
>  
>  <!ENTITY safeb.blocked.harmfulPage.title "The site ahead may contain malware">
>  <!-- Localization note (safeb.blocked.harmfulPage.shortDesc) - Please don't translate the contents of the <span id="harmful_sitename"/> tag. It will be replaced at runtime with a domain name (e.g. www.badsite.com) -->
> -<!ENTITY safeb.blocked.harmfulPage.shortDesc "Firefox blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">
> +<!ENTITY safeb.blocked.harmfulPage.shortDesc "&brandShortName; blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">
>  <!ENTITY safeb.blocked.harmfulPage.longDesc "">

Do we really need this empty string? I seriously doubt we do.

Comment 3

[Francesco Lodolo [:flod]]

Adding a comment from the previous bug 

(In reply to Ton from comment #32)
> (In reply to Wes Kocher (:KWierso) from comment #29)
> > https://hg.mozilla.org/mozilla-central/rev/fa94b9e702ec
> 
> 1) No newline at end of file (phishing-afterload-warning-message.dtd)
> 2) I always wondered why/how a site could steal "credit cards" rather than
> "credit card data/info/details" (there’s 5 other occurrences in the tree for
> similar strings, but still)
> 3) The "site ahead" is probably a little new to localizers, but should be
> clear enough (not vital, but perhaps something to think about when choosing
> words)

I absolutely agree on point 2. 

Is the text coming from a copy review? I know a lot of work went into refining the message for these pages for Photon.

Side note: preferences switched to using "website" instead of "site", but I don't know if that should be extended to new messages as well.

Comment 4

[Francesco Lodolo [:flod]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

https://reviewboard.mozilla.org/r/165914/#review171072

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:35
(Diff revision 1)
>  <!ENTITY safeb.blocked.phishingPage.title2 "Deceptive Site!">
>  <!-- Localization note (safeb.blocked.phishingPage.shortDesc2) - Please don't translate the contents of the <span id="phishing_sitename"/> tag. It will be replaced at runtime with a domain name (e.g. www.badsite.com) -->
>  <!ENTITY safeb.blocked.phishingPage.shortDesc2 "This web page at <span id='phishing_sitename'/> has been reported as a deceptive site and has been blocked based on your security preferences.">
>  <!ENTITY safeb.blocked.phishingPage.longDesc2 "<p>Deceptive sites are designed to trick you into doing something dangerous, like installing software, or revealing your personal information, like passwords, phone numbers or credit cards.</p><p>Entering any information on this web page may result in identity theft or other fraud.</p>">
>  
>  <!ENTITY safeb.blocked.harmfulPage.title "The site ahead may contain malware">

Another issue pointed out by Ton: there's no such thing as a <span> in the string

Comment 5

[François Marier [:francois]]

(In reply to Francesco Lodolo [:flod] from comment #3)
> Is the text coming from a copy review? I know a lot of work went into
> refining the message for these pages for Photon.

Yes, the copy was reviewed in bug 1358536, though some of the copy changes from that bug are dependent on the Photon update which hasn't happened yet.

Comment 6

[Francesco Lodolo [:flod]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

https://reviewboard.mozilla.org/r/165914/#review171548

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165914/diff/1-2/

Comment 8

[Francesco Lodolo [:flod]]

(In reply to François Marier [:francois] from comment #5)
> Yes, the copy was reviewed in bug 1358536, though some of the copy changes
> from that bug are dependent on the Photon update which hasn't happened yet.

Can you clarify when these updates are supposed to happen?

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

https://reviewboard.mozilla.org/r/165914/#review173406

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:35
(Diff revision 2)
>  <!ENTITY safeb.blocked.phishingPage.title2 "Deceptive Site!">
>  <!-- Localization note (safeb.blocked.phishingPage.shortDesc2) - Please don't translate the contents of the <span id="phishing_sitename"/> tag. It will be replaced at runtime with a domain name (e.g. www.badsite.com) -->
>  <!ENTITY safeb.blocked.phishingPage.shortDesc2 "This web page at <span id='phishing_sitename'/> has been reported as a deceptive site and has been blocked based on your security preferences.">
>  <!ENTITY safeb.blocked.phishingPage.longDesc2 "<p>Deceptive sites are designed to trick you into doing something dangerous, like installing software, or revealing your personal information, like passwords, phone numbers or credit cards.</p><p>Entering any information on this web page may result in identity theft or other fraud.</p>">
>  
> -<!ENTITY safeb.blocked.harmfulPage.title "The site ahead may contain malware">
> +<!ENTITY safeb.blocked.harmfulPage.title2 "The site ahead may contain malware">

This string doesn't change, you should keep the existing ID. There's no need for the two strings to be "in sync".

Comment 10

[François Marier [:francois]]

(In reply to Francesco Lodolo [:flod] (traveling, slow reply until Aug 18) from comment #8)
> (In reply to François Marier [:francois] from comment #5)
> > Yes, the copy was reviewed in bug 1358536, though some of the copy changes
> > from that bug are dependent on the Photon update which hasn't happened yet.
> 
> Can you clarify when these updates are supposed to happen?

I have no idea. That would be a question for whoever does the prioritization of Photon tasks.

Comment 11

[Henry Chang [:hchang]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165914/diff/2-3/

Comment 12

[Francesco Lodolo [:flod]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

https://reviewboard.mozilla.org/r/165914/#review174448

String wise (changes to phishing-afterload-warning-message.dtd) this looks good, but you seem to have errors in the code now. I wonder if it's safer to get someone more familiar with this code to review it now.

::: browser/base/content/blockedSite.xhtml:129
(Diff revision 3)
>            el = document.getElementById("errorLongDescText_unwanted");
>            el.remove();
>          }
>  
>          if (error !== "harmful") {
> -          el = document.getElementById("errorTitleText_harmful");
> +          el = document.getElementById("errorTitleText_harmful2");

I still see the same ID in the XHTML code (only the string ID inside changed).

::: browser/base/content/blockedSite.xhtml:131
(Diff revision 3)
>          }
>  
>          if (error !== "harmful") {
> -          el = document.getElementById("errorTitleText_harmful");
> +          el = document.getElementById("errorTitleText_harmful2");
>            el.remove();
> -          el = document.getElementById("errorShortDescText_harmful");
> +          el = document.getElementById("errorShortDescText_harmful2");

Same here: this ID is not available in the XHTML file.

Comment 13

[Henry Chang [:hchang]]

(In reply to Francesco Lodolo [:flod] (traveling, slow reply until Aug 18) from comment #12)
> Comment on attachment 8894736 [details]
> Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.
> 
> https://reviewboard.mozilla.org/r/165914/#review174448
> 
> String wise (changes to phishing-afterload-warning-message.dtd) this looks
> good, but you seem to have errors in the code now. I wonder if it's safer to
> get someone more familiar with this code to review it now.
> 
> ::: browser/base/content/blockedSite.xhtml:129
> (Diff revision 3)
> >            el = document.getElementById("errorLongDescText_unwanted");
> >            el.remove();
> >          }
> >  
> >          if (error !== "harmful") {
> > -          el = document.getElementById("errorTitleText_harmful");
> > +          el = document.getElementById("errorTitleText_harmful2");
> 
> I still see the same ID in the XHTML code (only the string ID inside
> changed).
> 
> ::: browser/base/content/blockedSite.xhtml:131
> (Diff revision 3)
> >          }
> >  
> >          if (error !== "harmful") {
> > -          el = document.getElementById("errorTitleText_harmful");
> > +          el = document.getElementById("errorTitleText_harmful2");
> >            el.remove();
> > -          el = document.getElementById("errorShortDescText_harmful");
> > +          el = document.getElementById("errorShortDescText_harmful2");
> 
> Same here: this ID is not available in the XHTML file.

I think I accidentally pushed some garbage to reviewboard and 
the review flag was automatically set by the system.

Sorry about that :(

Comment 14

[Henry Chang [:hchang]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165914/diff/3-4/

Comment 15

[Henry Chang [:hchang]]

Added Francois for reviewing the blockedSite.xhtml changes.

Comment 16

[Francesco Lodolo [:flod]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

https://reviewboard.mozilla.org/r/165914/#review174994

As said, string wise this looks good. Thanks!

Comment 17

[François Marier [:francois]]

Comment on attachment 8894736 [details]
Bug 1388233 - Use &brandShortName; instead of hard-coded 'Firefox'.

https://reviewboard.mozilla.org/r/165914/#review175048

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce623b556809
Use &brandShortName; instead of hard-coded 'Firefox'. r=flod,francois

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 20

[Ton [:Tonnes]]

> +<!ENTITY safeb.blocked.harmfulPage.shortDesc2 "&brandShortName; blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">
> \ No newline at end of file

So a) still no newline, and b) it can still steal credit cards rather than credit card info? Would that require another bug to reword it consistently?

Comment 21

[Francesco Lodolo [:flod]]

Newline is not really an issue, message is going to be reworded anyway at some point
https://bugzilla.mozilla.org/show_bug.cgi?id=1388233#c10