355554 - File downloads should honor Vista's parent control setting

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect)

Description

[Scott MacGregor]

Part of the work to integrate Firefox with Vista's builtin parental controls:

http://www.microsoft.com/windowsvista/community/parentalcontrols.mspx

Vista has a system wide parental control setting for disabling file downloads that should be honored by all desktop applications. Firefox needs to check this global setting and disable file downloads if it is set.

Comment 1

[Scott MacGregor]

Here's another doc describing this feature:

http://blogs.msdn.com/ie/archive/2006/03/01/541701.aspx

Comment 2

[Ian Tommins ('thelem')]

> I am not sure what we should or shouldn't block

We should block whatever IE blocks. The user experience should be consistant whatever browser is being used, and it shouldn't be possible to circumvent restrictions in IE by installing Firefox. Do Microsofts restrictions already block anything in Firefox?

Comment 3

[Peter Simonyi]

> Do Microsofts restrictions already block anything in Firefox?

According to "Al Billings [MSFT]", 'basic' parental controls will apply to Firefox.
http://blogs.msdn.com/ie/archive/2006/03/01/541701.aspx#541839

Of course, he doesn't say what "basic" means.

Comment 4

[Doug Turner (:dougt)]

This will require string changes and therefore I didn't continue implementing it for FF2. 

Comment 5

[Jay Patel [:jay]]

Not blocking for 1.8.1.2, but we need marketing/products input on whether such string changes might be worth doing for FF2 to support Vista parental controls.

schrep:  Is this something we would take in a future release if we coordinated the string changes with all locales?

Comment 6

[Mike Schroepfer]

I think we should think strongly about trying to get something like this in a future release.  

If we plan this far enough in advance and are smart about reusing strings and doing fallbacks this should be possible.  The first step obviously is to get an implementation on trunk that were happy with before we can worry about getting it ported to the branch.

I agree I don't think we can accomplish this in time for the 1812 release.

Comment 7

[Axel Hecht [:Pike]]

From a localization point of view, the safest approach would be to just add the strings early in the development cycle, and add the English version of those to all locales, too, together with an announcement to .l10n about that and to then accept patches in bugs.

Comment 8

[Doug Turner (:dougt)]

mconnor, i remember chatting with you about cleaning up the file download code at some point.  When this happens, i strongly urge you to consider the ability to block file downloads.

Right now the file download code is scattered throughout the code base and blocking each place is somewhat a pita.  

Comment 9

[Axel Hecht [:Pike]]

Would a general download manager cleanup patch be more likely to make it onto the branch?

Comment 10

[Marcia Knous [:marcia]]

Spent a little time with dougt testing this.

Need to decide how we will handle downloading images. Current behavior on IE on Vista RTM if the "Block downloads" pref is on in PC.

1. Download an image from a website using "Save Target As" ->IE throws up block dialog warning.
2. Download an image from a website using "Save Picture As"->IE allows you to save the image.

I realize that the pref says "Block File Downloads" but wondering why IE would have inconsistent behavior here.

Comment 11

[Marcia Knous [:marcia]]

Testing with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.2) Gecko/2007020823 Firefox/2.0.0.2, I checked the radio button to block downloads for a standard user. I then switched to a standard account, and Firefox did not block several downloads, both from mozilla.com and caminobrowser.org. It did block some downloads I tried from download.com.

Comment 12

[Robert Strong (they/them - no direct email)]

(In reply to comment #10)
> Spent a little time with dougt testing this.
> 
> Need to decide how we will handle downloading images. Current behavior on IE on
> Vista RTM if the "Block downloads" pref is on in PC.
> 
> 1. Download an image from a website using "Save Target As" ->IE throws up block
> dialog warning.
> 2. Download an image from a website using "Save Picture As"->IE allows you to
> save the image.
> 
> I realize that the pref says "Block File Downloads" but wondering why IE would
> have inconsistent behavior here.
If you can view the image it allows saving. If it is a link to a file whether it is an image, exe, etc. it should block the download.

Comment 13

[Robert Strong (they/them - no direct email)]

(In reply to comment #11)
> Testing with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.2)
> Gecko/2007020823 Firefox/2.0.0.2, I checked the radio button to block downloads
> for a standard user. I then switched to a standard account, and Firefox did not
> block several downloads, both from mozilla.com and caminobrowser.org. It did
> block some downloads I tried from download.com.
I believe this is correct in that until this bug is fixed Firefox will not block downloads.

Comment 14

[Robert Strong (they/them - no direct email)]

(In reply to comment #12)
> (In reply to comment #10)
> >...
> If you can view the image it allows saving. If it is a link to a file whether
> it is an image, exe, etc. it should block the download.
In case this explanation isn't clear if you can view it then there is little preventing you from saving the image any number of different ways (e.g. screenshot, save page, etc.) so it doesn't make sense to block it under these conditions.

Comment 15

[entropy]

>We should block whatever IE blocks. The user experience should be consistant
whatever browser is being used, and it shouldn't be possible to circumvent
restrictions in IE by installing Firefox. Do Microsofts restrictions already
block anything in Firefox?

Trying to block downloads in firefox wouldn't really help anything with circumventing restrictions. If the user can view pages then he/she can pretty much download any file they want. Just blocking the download manager wouldn't help anything, one would just use a seperate download manager to download the files(wget or something like that would suffice and wouldn't need to be installed so even with all the restrictions a user would be able to use it)

Is implementing this into Firefox really worth the effort?

Comment 16

[nrlz]

Windows XP has a group policy config for restricting file downloads. Is that supported too?

Comment 17

[Rob]

I don't understand why this "bug" needs to be addressed anyway.  I understand that Firefox wants to be universally accepted, but it's not like you guys are stuck doing exactly what Microsoft wants.

Last I knew, Firefox wasn't owned by Microsoft, nor was it licensed to do everything in accordance with Microsoft's standards and security features.

If people need Microsoft's security to be set a certain way, then shouldn't they really be using a first party program such as Internet Explorer rather than an open source third party program?

Comment 18

[Jim Mathies [:jimm]]

Attachment: download manager patch v.1

A fairly simple patch that picks up the HTTP 450 status code in nsDownload’s STATE_START state change event, fails the download and sets the state to a new download manager state - DOWNLOAD_BLOCKED. I've also added a binding and some strings so these entries display their own message. Attached is a screen shot. It might be nice to swap out the generic error icon for a "blocked by parental controls proxy" icon, or alternatively, just go with the default blank file icon used in other downloads.

The other place I've seen bad handling of the 450 code is in Live Bookmark's subscriber feature, I'll work something up for that as well.

Comment 19

[Jim Mathies [:jimm]]

Attachment: download manager window

Comment 20

[Shawn Wilsher :sdwilsh]

Comment on attachment 272874 [details] [diff] [review]
download manager patch v.1

>Index: toolkit/components/downloads/public/nsIDownloadManager.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/downloads/public/nsIDownloadManager.idl,v
>retrieving revision 1.13
>diff -u -p -8 -r1.13 nsIDownloadManager.idl
>--- toolkit/components/downloads/public/nsIDownloadManager.idl	2 Jul 2007 17:25:30 -0000	1.13
>+++ toolkit/components/downloads/public/nsIDownloadManager.idl	18 Jul 2007 21:22:54 -0000
>@@ -55,16 +55,17 @@ interface mozIStorageConnection;
> interface nsIDownloadManager : nsISupports {
>   // Download States
>   const short DOWNLOAD_NOTSTARTED       = -1;
>   const short DOWNLOAD_DOWNLOADING      = 0;
>   const short DOWNLOAD_FINISHED         = 1;
>   const short DOWNLOAD_FAILED           = 2;
>   const short DOWNLOAD_CANCELED         = 3;
>   const short DOWNLOAD_PAUSED           = 4;
>+  const short DOWNLOAD_BLOCKED          = 5; // parental controls
I'm not sure we need the parental controls comment here - are there other reasons this can be blocked.  Also, a more in-depth comment about how this will only happen on windows would be nice.

>+  if (aStateFlags & STATE_START) {
>+    nsresult rv;
>+    nsCOMPtr<nsIHttpChannel> channel = do_QueryInterface(aRequest, &rv);
>+    if ( channel ) {
nit: no space after or before parans

also, you should be doing NS_SUCCEEDED(rv) && channel here I think, or just not pass in rv to do_QueryInterface.

>+      PRUint32 status = 0;
no need to assign to 0 here, just declare

>+      rv = channel->GetResponseStatus(&status);
>+      if ( status == 450 ) { // Blocked by parental control proxies
check that rv succeeded and check the status.  Is there no constant that we can check against (I'm not a huge fan of magic numbers.

>+        // We must send dl-start to get the download listed
>+        if (mDownloadState == nsIDownloadManager::DOWNLOAD_NOTSTARTED) {
>+          SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
>+          mDownloadManager->mObserverService->NotifyObservers(this, "dl-start", nsnull);
>+        }
Ug - I'd rather fix the UI to add downloads if they aren't added (which I think I've already done in my UI rewrite).  We shouldn't be doing this if it was not started.

>+        // Don't cancel if download is already finished
>+        if ( mDownloadState != nsIDownloadManager::DOWNLOAD_FINISHED ) {
>+          if (mCancelable)
>+            mCancelable->Cancel(NS_BINDING_ABORTED);
>+        }
you should just check for mCancelable here.  FinishDownload (which will be called if the state is DOWNLOAD_FINISHED) sets mCancelable to nsnull.

>+        // Fail the download - DOWNLOAD_BLOCKED
>+        rv = mDownloadManager->FinishDownload(this, nsIDownloadManager::DOWNLOAD_BLOCKED,
>+                                     "dl-failed");
nit: spacing

>   if (aStateFlags & STATE_STOP) {
this should be an else if now (although arguably it doesn't matter - just easier to read)

I'll look at the rest later.

Comment 21

[Shawn Wilsher :sdwilsh]

This is pretty low hanging fruit to get a nice win for OS integration.

Comment 22

[Robert Strong (they/them - no direct email)]

Jim, what about prompting to get permission to download prior to download like IE does?

Comment 23

[Jim Mathies [:jimm]]

> Jim, what about prompting to get permission to download prior to download like
IE does?

Some form of prompt is possible, but might be annoying. One way to do this would be to replace the "Retry" link with something like "Get Approval" or similar. That could then make the neccassary call into windows to approve the download, and trigger the retry after it's approved. Is that in line with what you were thinking?

Comment 24

[Robert Strong (they/them - no direct email)]

I thought IE does this via their notification bar though I may be wrong... if possible, that seems ideal

Comment 25

[Jim Mathies [:jimm]]

(Rob, I'll go take a look and maybe snap a screenshot and post back.)

>+  const short DOWNLOAD_BLOCKED          = 5; // parental controls
>I'm not sure we need the parental controls comment here - are there other
>reasons this can be blocked.  Also, a more in-depth comment about how this will
>only happen on windows would be nice.

I'll just take it out I think, seeing as how nothing else is commented. However, I think with Leopard we might have Apple support. It looks like they've implemented features similar to Vista, so I'm not convinced this will be win32 specific. (Although I can't seem to find any reference to developer apis via a few searches.) 
http://www.apple.com/macosx/leopard/features/parentalcontrols.html

>+        if (mDownloadState == nsIDownloadManager::DOWNLOAD_NOTSTARTED) {
>+          SetState(nsIDownloadManager::DOWNLOAD_DOWNLOADING);
>+          mDownloadManager->mObserverService->NotifyObservers(this, "dl-start", nsnull);
>+        }
>Ug - I'd rather fix the UI to add downloads if they aren't added (which I think
>I've already done in my UI rewrite).  We shouldn't be doing this if it was not
>started.

Leave this be for now and look into addressing it when your UI comes out? What's the time frame on your UI landing?

Comment 26

[Zach Lipton [:zach]]

Leopard does not currently have support for allowing/disallowing downloads with it's parental controls so that's not an issue for DM here, although mac parental controls support in general is something we'll probably want to look at.

Comment 27

[Jim Mathies [:jimm]]

Attachment: download blocked

This is the only thing I could find, which actually kind of sucks because it doesn't give you the ability to override it. The dialog comes up in a number of cases -

- clicking on a link to an exe or other type of executable file 
- right-click, save as a link that points to offending image or exe content

clicking on links to offending sites will result in the parental controls html page being displayed, which offers the opportunity to override. I'll post a shot of that as well here in a sec.

Comment 28

[Jim Mathies [:jimm]]

Attachment: pc override request

There's a bug on this prompt posted - bug 353098, but it looks like everyone there decided to leave it alone. The prompt isn't 'pretty', but the override does work once you get past it.

Comment 29

[Jim Mathies [:jimm]]

Also for reference, other bugs related to this - bug 232816, bug 353098, bug 355557, bug 372905, bug 372914, & bug 372920.

Comment 30

[Jim Mathies [:jimm]]

Zach - are you saying it'll block the file download but it won't give you you the chance to override? If so, i'm curious how file downloads that get blocked look in firefox on osx, and would this patch possibly address any 'ugliness' there?

Comment 31

[Zach Lipton [:zach]]

No, I'm simply saying that Leopard does not, at present, have any UI in its parental controls feature related to file downloads at all; you cannot block downloads. It does have a more general way to block access to websites in general (both whitelist and blocklist based), but we don't currently honor that, and I don't have details on how that feature works. 

Comment 32

[Jim Mathies [:jimm]]

Attachment: download manager patch v.2

I haven't added the option to "approve" the download here, which would be handed by a call into the Vista parental controls apis. That seems best suited to the interface being developed in the parent bug, but since there's no guarantee that'll make into FF3, I didn't want to hold this patch up. It should be fairly simple to add once we get that finished up though.

Comment 33

[Shawn Wilsher :sdwilsh]

Comment on attachment 274578 [details] [diff] [review]
download manager patch v.2

>Index: toolkit/components/downloads/public/nsIDownloadManager.idl
you need to rev the uuid

>+      if (NS_SUCCEEDED(rv) && status == 450) { // HTTP 450 - Blocked by parental control proxies
nit: 80 chars

>+
>+        // Cancel using the provided object
>+        if (mCancelable)
>+          mCancelable->Cancel(NS_BINDING_ABORTED);
please cast to void since you are not checking the return variable
(void)mCancelable->Cancel(NS_BINDING_ABORTED);

>+        // if there's a progress dialog open for the item,
>+        // we have to notify it that we're cancelling
nit: canceling

>+        if (mDialog) {
>+          nsCOMPtr<nsIObserver> observer = do_QueryInterface(mDialog);
>+          observer->Observe(this, "oncancel", nsnull);
cast to void

>-  dl.setAttribute("image", "moz-icon://" + aFile + "?size=32");
>+  if (aState != nsIDownloadManager.DOWNLOAD_BLOCKED)
>+    dl.setAttribute("image", "moz-icon://" + aFile + "?size=32");
>+  else
>+    dl.setAttribute("image", "moz-icon://" + BLOCKED_ICON + 
"?size=32");
You shouldn't be using a moz-icon url here - the chrome one will be fine.
Also, don't capitalize the file name please.

>@@ -115,16 +119,22 @@ function downloadCompleted(aDownload) 
>       var listItem = getDownload(aDownload.id)
>       var oldImage = listItem.getAttribute("image");
>       // I tack on the content-type here as a hack to bypass the cache which seems
>       // to be interfering despite the fact the image has 'validate="always"' set
>       // on it. 
>       listItem.setAttribute("image", oldImage + "&contentType=" + contentType);
>     } catch (e) { }
>     
>+    if (aDownload.state == nsIDownloadManager.DOWNLOAD_BLOCKED) {
>+      var img = BLOCKED_ICON;
>+      var listItem = getDownload(aDownload.id);
Technically, I think listItem is already declared here because of how var's scoping works.  If you want to change the above declaration to let and do the same here, I'd love that ;)

>+      if ( listItem ) listItem.setAttribute("image", img);
The other code doesn't check if it exists first, why are you?

>+    case "dl-blocked":
>+      var dl = aSubject.QueryInterface(Components.interfaces.nsIDownload);
>+      if (getDownload(dl.id))
>+        return;
>+
>+      // Add blocked item to the UI, we need the target though for retries.
>+      var uri = Components.classes["@mozilla.org/network/util;1"]
>+                          .getService(Components.interfaces.nsIIOService)
>+                          .newFileURI(dl.targetFile);
>+      var itm = createDownloadItem(dl.id, uri.spec, dl.displayName,
>+                                   dl.source.spec, dl.state, "",
>+                                   dl.percentComplete);
>+      gDownloadsView.insertBefore(itm, gDownloadsView.firstChild);
>+
>+      // switch view to it
>+      gDownloadsView.selectedIndex = 0;
>+      downloadCompleted(dl);
>+      break;
could this not share code with dl-start bits?

>+  observerService.addObserver(gDownloadObserver, "dl-blocked", false);
you need to remove this observer in the destructor, otherwise we leak it.

Also, I didn't see an entry to add the blocked image to the jar.mn entries for the skin.

Almost there :)

Comment 34

[Jim Mathies [:jimm]]

Attachment: download manager patch v.3

Nice catches, especially on the observer and the uid. I've made all the changes, a couple comments - 

> Also, I didn't see an entry to add the blocked image to the jar.mn entries for
>the skin.

It's using the standard error icon, which is already checked in.

> could this not share code with dl-start bits?

I'm not sure I like this honestly, placing a 'hidden' if statement at the end of the block which checks the value so as to add the final download complete call, but I made the change anyway since it's nothing major. You'll be in there mucking around with it anyway, so I'll go with your preference.

Any recs on who could do a superreview?

Comment 35

[Jim Mathies [:jimm]]

Comment on attachment 274589 [details] [diff] [review]
download manager patch v.3

nope, there's actually two statements in that cose block. that won't work. i'll revert and submit again.

Comment 36

[Jim Mathies [:jimm]]

Attachment: download manager patch v.3

resubmitted. The dl-start has
gActiveDownloads.push(dl);
&
the dl-blocked has the added 
downloadCompleted(dl);

Comment 37

[Shawn Wilsher :sdwilsh]

Comment on attachment 274592 [details] [diff] [review]
download manager patch v.3

>   nsCOMPtr<mozIStorageStatement> stmt;
>   nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>     "DELETE FROM moz_downloads "
>     "WHERE state = ?1 "
>     "OR state = ?2 "
>-    "OR state = ?3"), getter_AddRefs(stmt));
>+    "OR state = ?3 "
>+    "OR state = ?5"), getter_AddRefs(stmt));
You mean 4, right? :p


>   NS_ENSURE_SUCCESS(rv, rv);
>   for (PRUint32 i = 0; i < 3; ++i) {
and you'll have to update this

>   nsCOMPtr<mozIStorageStatement> stmt;
>   nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>     "SELECT COUNT(*) "
>     "FROM moz_downloads "
>     "WHERE state = ?1 "
>     "OR state = ?2 "
>-    "OR state = ?3"), getter_AddRefs(stmt));
>+    "OR state = ?3 "
>+    "OR state = ?5"), getter_AddRefs(stmt));
>   NS_ENSURE_SUCCESS(rv, rv);
>   for (PRUint32 i = 0; i < 3; ++i) {
ditto

>+        rv = mDownloadManager->FinishDownload(this, nsIDownloadManager::DOWNLOAD_BLOCKED,
>+                                              "dl-blocked");
nit: can you have the second param on a new line here please?


>+          <xul:image class="downloadTypeIcon" validate="always" style="width: 32px; max-width: 32px; height: 32px; max-height: 32px;" xbl:inherits="src=image"/>
I should have caught this last time.  I know the other code does this with the style attribute, but it isn't necessary.  Please remove it.

>+    dl.setAttribute("image", blockedIcon + "?size=32");
you also don't need the ?size=32 here :)

>+      let (listItem = getDownload(aDownload.id)) {
Actually, all you have to do is |let listItem = getDownload(aDownload.id);|

(In reply to comment #34)
> It's using the standard error icon, which is already checked in.
But it isn't packed on platforms other than windows, so we need something for them.

> Any recs on who could do a superreview?
toolkit doesn't need it, but you'll need a peer.  Request review from either gavin or mano next.

Comment 38

[Jim Mathies [:jimm]]

Attachment: download manager patch v.4

Comment 39

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

Comment on attachment 274650 [details] [diff] [review]
download manager patch v.4

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

>@@ -1454,22 +1458,54 @@ nsDownload::OnStateChange(nsIWebProgress

>-  if (aStateFlags & STATE_STOP) {
>+  if (aStateFlags & STATE_START) {
>+    nsresult rv;
>+    nsCOMPtr<nsIHttpChannel> channel = do_QueryInterface(aRequest, &rv);
>+    if (NS_SUCCEEDED(rv) && channel) {

A null check + rv check is redundant, just do one or the other. The rest of this code seems to favor rv checks.
 
>+        // Cancel using the provided object
>+        if (mCancelable)
>+          (void)mCancelable->Cancel(NS_BINDING_ABORTED);

I think casting to void like this is silly, but I guess that's this file's style :(

>+        // if there's a progress dialog open for the item,
>+        // we have to notify it that we're canceling
>+        if (mDialog) {
>+          nsCOMPtr<nsIObserver> observer = do_QueryInterface(mDialog);
>+          (void)observer->Observe(this, "oncancel", nsnull);

Is it possible for the dialog to not be a nsIObserver? This pattern is already in the code this was copied from, so I suppose not, but it probably wouldn't hurt to add a null check here and avoid crashing.

>@@ -1519,17 +1555,17 @@ nsDownload::OnStateChange(nsIWebProgress

>-    if (addToRecentDocs) {
>+    if (mDownloadState != nsIDownloadManager::DOWNLOAD_BLOCKED && addToRecentDocs) {

Seems more logical to have this check for FINISHED rather than !BLOCKED (although perhaps this whole code block should be in the earlier "if IsInFinalStage" block?).

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

>+<!ENTITY blocked.label                    "Blocked by Parental Controls">

Does "Parental Controls" really deserve to be capitalized? If it's the name of a Vista feature or something we should probably add a localization note to that effect, so that localizers know to use the proper term.

>Index: toolkit/mozapps/downloads/content/download.xml

I wonder whether some of these bindings (including the one you're adding) could be combined, they seem to have a lot of things in common... separate bug, I suppose.

>Index: toolkit/mozapps/downloads/content/downloads.js

>@@ -107,24 +111,30 @@ function downloadCompleted(aDownload) 

>+      let (listItem = getDownload(aDownload.id)) {
>+        var oldImage = listItem.getAttribute("image");
>+        // I tack on the content-type here as a hack to bypass the cache which seems
>+        // to be interfering despite the fact the image has 'validate="always"' set
>+        // on it. 
>+        listItem.setAttribute("image", oldImage + "&contentType=" + contentType);
>+      }
>     } catch (e) { }
>     
>+    if (aDownload.state == nsIDownloadManager.DOWNLOAD_BLOCKED) {
>+      let listItem = getDownload(aDownload.id);
>+      listItem.setAttribute("image", blockedIcon);
>+    }

Not sure what this change is about - why not just declare listItem inside the first try block, with "var"?

I haven't tested this patch, I'm assuming it's been well tested by you and/or Shawn. Have you considered writing unit tests for this functionality? I suppose the Vista integration part might make that a bit tricky, but it'd be nice to get some automated test coverage of the newly added code.

r=me with these comments addressed.

Comment 40

[Jim Mathies [:jimm]]

>A null check + rv check is redundant, just do one or the other.

will do.

>I think casting to void like this is silly..

agreed.

>wouldn't hurt to add a null check..

good point, will do.

>Seems more logical to have this check for FINISHED..

I'll look at it and see if I can clean it up a bit.

>Does "Parental Controls" really deserve to be capitalized?

Both Microsoft and Apple capitalize it, so I think we should do the same. I'll add a comment.

>I wonder whether some of these bindings..

That's getting overhauled by Shawn a bit too. I'd prefer to leave it alone for now until he's done.

>Not sure what this change is about..

Conflicting review requests here. :)

>I haven't tested this patch, I'm assuming it's been well tested by you..

Yes using a cgi script that returns the 450 status code. It's really tough getting debug builds (and even local release builds) up and running under a limited account on Vista. It should work just fine though. I will go back and test a nightly after it gets checked in to make sure. I've been meaning to get back and figure out why locally built release builds don't launch under these accounts, but haven't had the time. The exe will spawn, but main never gets called, seems to have something to do with dll loading.

>Have you considered writing unit tests for this functionality?

Haven't written a unit test yet, might make for a good exercise. I'll look into it.

Comment 41

[Jim Mathies [:jimm]]

Attachment: download manager patch v.5

removed bitrot from all the download manager work and implemented gavin's requests.

Comment 42

[Jim Mathies [:jimm]]

Comment on attachment 277440 [details] [diff] [review]
download manager patch v.5

Also, toolkit/themes/pinstripe/global/icons/Error.png needs to be copied over from winstripe.

Comment 43

[Shawn Wilsher :sdwilsh]

Comment on attachment 277440 [details] [diff] [review]
download manager patch v.5

>+  <binding id="download-blocked" extends="chrome://mozapps/content/downloads/download.xml#download-base">
>+    <content>
>+      <xul:hbox flex="1">
>+#ifdef SHOW_ICONS
>+        <xul:vbox pack="start">
>+          <xul:image class="downloadTypeIcon" validate="always"
>+                     xbl:inherits="src=image"/>
Instead of using an if in the JS, why not just create a new class for this download state ("blocked" perhaps?) and have this xul:image have |class="downloadTypeIcon blocked"|?  You can also drop the xbl:inherits by doing this.

>+            <xul:button class="retry mini-button" tooltiptext="&cmd.retry.label;"
>+                        command="cmd_retry" ondblclick="event.stopPropagation();"/>
So, I'm not sure we want to offer retry here.  However, as it stands, retry won't work anyway:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.102#853

>+const blockedIcon = "chrome://global/skin/icons/Error.png";
This doesn't exist in pinstripe, *and*, you shouldn't hardcode skin images.  use list-style-image and -moz-image-region.

>++  skin/classic/global/icons/Error.png                                (icons/Error.png)
ah, I see you are adding it to pinstripe here.

Comment 44

[Jim Mathies [:jimm]]

>Instead of using an if in the JS, why not just create a new class for this
>download state ("blocked" perhaps?) and have this xul:image have
>|class="downloadTypeIcon blocked"|?  You can also drop the xbl:inherits by
>doing this.

Thought of that but choose a simpler route as I'm not 100% up on xul bindings. I'll try to work something up.
 
>+            <xul:button class="retry mini-button" tooltiptext="&cmd.retry.label;"
>+      command="cmd_retry" ondblclick="event.stopPropagation();"/>
>
>So, I'm not sure we want to offer retry here.  However, as it stands, retry
>won't work anyway:

It works just fine with this patch. Note I've added blocked to the if statement in the retry method.
 

Comment 45

[Shawn Wilsher :sdwilsh]

Comment on attachment 277440 [details] [diff] [review]
download manager patch v.5

(In reply to comment #44)
> It works just fine with this patch. Note I've added blocked to the if statement
> in the retry method.
Whoops, not sure how I missed that - sorry.

r=sdwilsh with the other comment then.

Comment 46

[Jim Mathies [:jimm]]

Attachment: download manager patch v.6

I believe this is what you're looking for Shawn, let me know if I missed anything.

Comment 47

[Shawn Wilsher :sdwilsh]

Comment on attachment 277473 [details] [diff] [review]
download manager patch v.6

yup, just remove the |validate="always"| on the xul:image in the binding since it isn't necessary for this.  You've already got r from gavin, so this is good to checkin once that is fixed.

Comment 48

[Jim Mathies [:jimm]]

Attachment: download manager patch v.7

Comment 49

[Shawn Wilsher :sdwilsh]

I'm not sure if this can be tested automagically, but we could certainly use a litmus test.

Thanks for doing this Jim!

Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
new revision: 1.16; previous revision: 1.15
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.103; previous revision: 1.102
Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd;
new revision: 1.9; previous revision: 1.8
Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
new revision: 1.24; previous revision: 1.23
Checking in toolkit/mozapps/downloads/content/download.xml;
new revision: 1.35; previous revision: 1.34
Checking in toolkit/mozapps/downloads/content/downloads.css;
new revision: 1.10; previous revision: 1.9
Checking in toolkit/mozapps/downloads/content/downloads.js;
new revision: 1.83; previous revision: 1.82
Checking in toolkit/themes/pinstripe/global/jar.mn;
new revision: 1.33; previous revision: 1.32
Checking in toolkit/themes/pinstripe/mozapps/downloads/downloads.css;
new revision: 1.17; previous revision: 1.16
Checking in toolkit/themes/winstripe/mozapps/downloads/downloads.css;
new revision: 1.21; previous revision: 1.20
Checking in toolkit/themes/pinstripe/global/icons/Error.png;
initial revision: 1.1

Comment 50

[Marcia Knous [:marcia]]

Litmus Triage Team: tomcat will handle this test case.

Comment 51

[Marcia Knous [:marcia]]

http://litmus.mozilla.org/show_test.cgi?id=4374 has been added to the Test Suite to cover this case.

Comment 52

[Daniel Veditz [:dveditz]]

not blocking a 2.0.0.x branch release.

Comment 53

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verfified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0 ID:2008051206a