393303 - Create a blocked string for dirty downloads (virus scan failed)

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P2)

Description

[Rob Arnold [:robarnold]]

This is now used for both parental controls and downloads with uncleanable viruses

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

https://bugzilla.mozilla.org/show_bug.cgi?id=103487#c54 appears to be what this bug was spun off from.  I kinda suspect that splitting this into two states might be right thing to do, rather than just switching the string, which is currently "Blocked by Parental Controls".

This seems guaranteed to be confusing to the end-user; requesting blocking-1.9.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #1)
> bug was spun off from.  I kinda suspect that splitting this into two states
> might be right thing to do, rather than just switching the string, which is
> currently "Blocked by Parental Controls".

I would agree. One case is "you have decided to not show this content" and the other case is "this content has been deemed unsafe by your AV software", which shouldn't be handled by the same user facing message.

Comment 3

[Shawn Wilsher :sdwilsh]

Morphing - we need a new string though.

I'll make the new state DOWNLOAD_DIRTY, but that has no affect on the string presented to the user.

Madhava - suggestions for strings?

Comment 4

[Shawn Wilsher :sdwilsh]

How about "Blocked by third-party file scanner"?

Comment 5

[Madhava Enros [:madhava]]

How about even more direct?

"Blocked: may contain virus or spyware"

Comment 6

[Shawn Wilsher :sdwilsh]

Stephen - can you comment on the grammar of this please.

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This should do the trick.  I can't test that it works since my windows build is broken still, but I did test that it doesn't break things on my mac.

Gavin, could you test this when you review it please?

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

Strictly speaking, "may" is for permission-oriented verbiage, while "might" is for possibilities; vernacular allows for either, but I'd prefer "might".

Comment 9

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

Comment on attachment 289691 [details] [diff] [review]
v1.0

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp

> nsDownloadManager::CleanUp()

>-  for (PRUint32 i = 0; i < 4; ++i) {
>+  for (PRUint32 i = 0; i < 5; ++i) {

Use NS_ARRAY_LENGTH(states)?

> nsDownloadManager::GetCanCleanUp(PRBool *aResult)

>-      "OR state = ?4"), getter_AddRefs(stmt));
>+      "OR state = ?4 "
>+      "OR state = ?5"), getter_AddRefs(stmt));
>   NS_ENSURE_SUCCESS(rv, rv);
>   for (PRUint32 i = 0; i < 4; ++i) {

Here too (forgot to change the 4).

Don't you also need to update removable and canceledOrFailed on download-base in download.xml (although it appears that canceledOrFailed is no longer in use)? And onDownloadStateChange in DownloadProgressListener.js?

>Index: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties

> stateBlocked=Blocked by Parental Controls
>+stateDirty=Blocked: may contain virus or spyware

Having the colon in one of the "blocked" strings but not the other seems kind of odd, but I guess it doesn't much matter. I can't think of a better way of putting it.

Patch tests out fine otherwise.

Comment 10

[Reed Loden [:reed]]

Comment on attachment 289691 [details] [diff] [review]
v1.0

>+stateDirty=Blocked: may contain virus or spyware

Capitalize "may" here. Maybe even "Blocked: Download may contain a virus or spyware".

However, stephend made a good point. I did some research, and common use seems to be a toss-up between may versus might. http://www.economist.com/research/styleGuide/index.cfm?page=673903 makes me want to use "may" here, though.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses review comments, and updates the string accordingly.

Comment 12

[Shawn Wilsher :sdwilsh]

I don't forsee myself having time to land this, so...

Comment 13

[Reed Loden [:reed]]

Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
/cvsroot/mozilla/toolkit/components/downloads/public/nsIDownloadManager.idl,v  <--  nsIDownloadManager.idl
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.148; previous revision: 1.147
done
Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp,v  <--  nsDownloadScanner.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js,v  <--  DownloadProgressListener.js
new revision: 1.29; previous revision: 1.28
done
Checking in toolkit/mozapps/downloads/content/download.xml;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/download.xml,v  <--  download.xml
new revision: 1.40; previous revision: 1.39
done
Checking in toolkit/mozapps/downloads/content/downloads.css;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.css,v  <--  downloads.css
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.106; previous revision: 1.105
done
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties,v  <--  downloads.properties
new revision: 1.15; previous revision: 1.14
done

Comment 14

[Reed Loden [:reed]]

Relanded.

Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
/cvsroot/mozilla/toolkit/components/downloads/public/nsIDownloadManager.idl,v  <--  nsIDownloadManager.idl
new revision: 1.25; previous revision: 1.24
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.150; previous revision: 1.149
done
Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp,v  <--  nsDownloadScanner.cpp
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js,v  <--  DownloadProgressListener.js
new revision: 1.31; previous revision: 1.30
done
Checking in toolkit/mozapps/downloads/content/download.xml;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/download.xml,v  <--  download.xml
new revision: 1.42; previous revision: 1.41
done
Checking in toolkit/mozapps/downloads/content/downloads.css;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.css,v  <--  downloads.css
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.108; previous revision: 1.107
done
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties,v  <--  downloads.properties
new revision: 1.17; previous revision: 1.16
done

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007120705 Minefield/3.0b2pre and AVG Free 7.5.503, with the eicar.zip testfile.