Strings for blocking malicious, potentially unwanted, and uncommon downloads

RESOLVED FIXED in Firefox 35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 35
x86_64
Windows 7
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
This is a strings-only patch for bug 1062966.
(Assignee)

Comment 1

4 years ago
Created attachment 8484483 [details] [diff] [review]
The patch
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8484483 - Flags: review?(mak77)
(Assignee)

Updated

4 years ago
Iteration: --- → 35.1
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Comment on attachment 8484483 [details] [diff] [review]
The patch

Review of attachment 8484483 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/downloads/downloads.dtd
@@ +69,5 @@
> +<!-- LOCALIZATION NOTE (cmd.unblock.label):
> +     This command is shown in the context menu when malware is blocked, or as a
> +     text link when uncommon or potentially unwanted downloads are blocked.
> +
> +     Note: This string and those related to it were pre-landed, so they cannot

"those related to it" does not really make clear which ones they are, I could not tell learnmore or removefile are "related" to unblock.
what about adding a very simple oneline note like
"note: this string doesn't yet exist in the UI. See bug 1053890."
to _each_ of the localization notes instead?
all of these notes will be removed on landing so shouldn't be a big deal and the bug can contain enough information.

@@ +76,5 @@
> +<!ENTITY cmd.unblock.label                "Unblock">
> +<!ENTITY cmd.unblock.accesskey            "U">
> +<!-- LOCALIZATION NOTE (cmd.removeFile.label):
> +     This command is shown in the context menu when malware is blocked, or as a
> +     command icon when uncommon or potentially unwanted downloads are blocked.

how can text be shown _as_ a command "icon"? did you mean "as label of a command icon", or as a tooltip?

@@ +81,5 @@
> +     -->
> +<!ENTITY cmd.removeFile.label             "Remove File">
> +<!ENTITY cmd.removeFile.accesskey         "m">
> +
> +<!-- LOCALIZATION NOTE (blocked.label):

nit: sometimes you added a new line before each block of string, sometimes not, please make it coherent. Since these strings are related you could just newline before and after them, not in between.

@@ +84,5 @@
> +
> +<!-- LOCALIZATION NOTE (blocked.label):
> +     Shown before the file name for blocked downloads.
> +     -->
> +<!ENTITY blocked.label                    "BLOCKED:">

I wonder if some localization would want to append it rather than prefix it (in such a case should be moved to properties and have a %S). I'm not sure if such a problem may exist in some locale, it's clear our intent here is to make this very visible and thus keep it prefixed in both ltr and rtl. Since I didn't look at the code, please ensure won't be problematic to manage this as a dtd entity for rtl locales (thus that "before" is indeed what happens in both modes)

::: browser/locales/en-US/chrome/browser/downloads/downloads.properties
@@ +39,5 @@
>  stateDirty=Blocked: May contain a virus or spyware
>  
> +# LOCALIZATION NOTE (blockedMalware, blockedUnwanted):
> +# These strings are shown in the panel for some types of blocked downloads, and
> +# are immediately followed by the "Learn More" link, thus they end with a dot.

"they _must_ end with a period."

@@ +42,5 @@
> +# These strings are shown in the panel for some types of blocked downloads, and
> +# are immediately followed by the "Learn More" link, thus they end with a dot.
> +#
> +# Note: These strings and those related to them were pre-landed, so they cannot
> +# be tested in the first version of Aurora 34.  See bug 1053890 for mockups.

I'd also oneline this note and dupe it below in the other localization note

@@ +48,5 @@
> +blockedUnwanted=This file may harm your computer.
> +
> +# LOCALIZATION NOTE (unblockHeader, unblockTypeMalware,
> +# unblockTypePotentiallyUnwanted, unblockTypeUncommon, unblockTip,
> +# unblockButtonContinue, unblockButtonCancel):

some indentation here would help readability

@@ +50,5 @@
> +# LOCALIZATION NOTE (unblockHeader, unblockTypeMalware,
> +# unblockTypePotentiallyUnwanted, unblockTypeUncommon, unblockTip,
> +# unblockButtonContinue, unblockButtonCancel):
> +# These strings are displayed in the dialog shown when the user asks a blocked
> +# download to be unblocked.  The severity of the threat is higher for files

"The severity of the threat is expressed in descending order by the unblockType* strings, it is higher for..."
Attachment #8484483 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 3

4 years ago
Created attachment 8485642 [details] [diff] [review]
Updated patch

This addresses the review comments, while the final wording and styles are being worked on in bug 1053890.
Attachment #8484483 - Attachment is obsolete: true
Attachment #8485642 - Flags: review?(mak77)
Attachment #8485642 - Flags: review?(mak77) → review+
(Assignee)

Comment 4

4 years ago
Created attachment 8486334 [details] [diff] [review]
Latest strings
Attachment #8485642 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2f56e8e59a97
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
should this be uplifted?
That's the goal, we should do so ASAP.
(Assignee)

Comment 10

4 years ago
Comment on attachment 8486334 [details] [diff] [review]
Latest strings

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #8)
> should this be uplifted?

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #9)
> That's the goal, we should do so ASAP.

Does this mean someone is available to take bug 1062966 during this iteration? If we don't end up implementing this blocker in time, and we don't enable Application Reputation in Firefox 34 anyways, there's little point in uplifting the strings.
Attachment #8486334 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mmc)
Flags: needinfo?(gavin.sharp)
> 
> Does this mean someone is available to take bug 1062966 during this
> iteration?

Not by me, I'm sorry. The backend for this was significantly more complicated than existing Safebrowsing checks and I burned out on writing it without implementation assistance.
Flags: needinfo?(mmc)
(Assignee)

Comment 12

4 years ago
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #11)
> > Does this mean someone is available to take bug 1062966 during this
> > iteration?
> 
> Not by me, I'm sorry. The backend for this was significantly more
> complicated than existing Safebrowsing checks and I burned out on writing it
> without implementation assistance.

Sorry to hear that. The whole project definitely turned out to be much more complicated than anyone expected. The fact is, as I already mentioned to Gavin, that the unblock mechanism is not a trivial chunk of work as well. Since I know you've been keeping track of the project, my needinfo request wasn't suggesting you should do the work yourself, but rather an invitation to discuss with Gavin about what the release plan for the feature could realistically look like, based on the people available to work on it.

I'm working on higher priority Firefox 34 Loop bugs at the moment, so I'll only be able to provide reviews, but I won't be able to do active development.
Thanks Paolo, I understand and in the same boat. I'll talk to Gavin.
Comment on attachment 8486334 [details] [diff] [review]
Latest strings

This patch isn't a good candidate for uplift:

1. We don't take string changes on Aurora and this patch is entirely string changes.
2. As per comment 10 and comment 11 there's little point in taking this change without bug 1062966, which I don't think has an owner.

If there is a strong reason for taking this change in 34 specifically, please ping me to discuss.
Attachment #8486334 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
The plan was to be aggressive and get this into 34, but that just wasn't possible. We'll get this and bug 1062966 for Firefox 35.
Flags: needinfo?(gavin.sharp)
(Assignee)

Updated

4 years ago
Blocks: 1068656
(Assignee)

Updated

4 years ago
Blocks: 1068660
You need to log in before you can comment on or make changes to this bug.