145800 - Implement download progress UI

Status: VERIFIED FIXED

(Camino Graveyard :: General, defect)

Description

[Winnie Lam]

Implement download progress UI.  See also bug 143093 - Hookup time elapsed, time
remaining, bytes downloaded in d/l progress window.

Comment 1

[Winnie Lam]

Please see bug in mozdev:  http://mozdev.org/bugs/show_bug.cgi?id=1035 about
invoking the "Save Link As" interface and the download dialog box.

Comment 2

[Winnie Lam]

Suggestion on the implementation of the "Saving File" window from bug 1036 on
mozdev.org (http://mozdev.org/bugs/show_bug.cgi?id=1036):

In the "Saving To:" section of the "Saving File" window, you should use the
method [NSString stringByAbbreviatingWithTildeInPath]. It would make the window
a lot more readable, for instance:
it would replace:
/Volumes/Mammoth/braxtons/Desktop/2001PLMP.pdf
with:
~/Desktop/2001PLMP.pdf
Also, by shortening the string, you reduce the chances that a long file path
would be larger than the rect that you're writing it to...

for reference:
http://developer.apple.com/techpubs/macosx/Cocoa/Reference/Foundation/ObjC_classic/Classes/NSString.html#//apple_ref/occ/instm/NSString/stringByAbbreviatingWithTildeInPath

Comment 3

[Winnie Lam]

Test cases for testing this feature:
http://www.mozilla.org/quality/browser/front-end/testcases/file-handling/downloading.html

Hewitt:  I believe this feature has been checked in for 0.2.8?  If so, please
change the status to Resolved Fixed.  

Comment 4

[Greg K.]

Using Chimera/20020602, neither ET nor ERT are hooked up. There is no Bytes
Downloaded text shown.

Comment 5

[saari (gone)]

Joe shouldn't this be resolved

Comment 6

[Greg K.]

*** Bug 151646 has been marked as a duplicate of this bug. ***

Comment 7

[sairuh (rarely reading bugmail)]

the download progress dlg is there, but several items don't appear implemented.
i filed bug 152629 to cover the non-functional buttons.

Comment 8

[saari (gone)]

Steve, is this a dupe of another bug?

Comment 9

[Steve Dagley]

Conrad was working on this but I don't recall the bug # (and I'm bust tag
teaming 143543 with Simon at the moment)

Comment 10

[Conrad Carlen (not reading bugmail)]

That's bug 156300. Now that my printing API code is in, I can start on that
again. Steve, as we discussed, there's plenty work on this issue to go around.
In bug 156300, I'm planning on hooking up the stubbed-out nsIWebProgressListener
used for downloading to the download progress dialog (after some refactoring) We
also need the helper app dialog implemented. Do you want to turn this bug into
that issue and deal with it?

Comment 11

[saari (gone)]

*** Bug 156300 has been marked as a duplicate of this bug. ***

Comment 12

[Simon Fraser [no longer active]]

Taking

Comment 13

[Simon Fraser [no longer active]]

Attachment: Patch to show the progress dialog for downloads

This patch allows the progress dialog to show up for downloads. I change the
way we drive the progress dialog for the File->Save case to be more similar to
downloading, so that we can share the same code. The changes in the patch are
as follows:

Made a new nsDownloadListener class that is an nsIDownload and
nsIWebProgressListener. nsCocoaBrowserService now makes one of these in its
nsIFactory CreateInstance method when asked. nsDownloadListener brings up the
download dialog, and proxies OnProgress notifications to it. This is the code
path for downloads.

nsHeaderSniffer (which is used when doing File->Save) itself now makes and
nsDownloadListener, and sets it as the web progress listener on the
webBrowserPersist. nsHeaderSniffer was moved from nsCocoaBrowserService.mm to
nsDownloadListener.mm.

I also hooked up the Cancel button in the dialog, so that should work now.

Comment 14

[Simon Fraser [no longer active]]

Attachment: Patch including new files

Comment 15

[Simon Fraser [no longer active]]

Things still to fix here:
1. Hook up helper app launching (other bugs exist)
2. Check in patch to make progress dialog localizable
3. Tweak dialog UI (fix the 'leave open' button toggle madness)
4. See if we can hook up pause/resume
5. Throttle the rate of progress updates, since they slow down the download
   on fast connections

Comment 16

[Mike Pinkerton (not reading bugmail)]

+    BOOL 			mDownloadIsPaused;
+    BOOL 			mSaveFileDialogShouldStayOpen;
+    BOOL 			mDownloadIsComplete;
+    long		 	aCurrentProgress;

why the huge indenting? also, there are tabs here

+  aCurrentProgress = curProgress; 				// fall back for stat calcs

again, tabs (as well as several other places in this file)

+ *  nsDownloadListener.h
+ *  Chimera
+ *
+ *  Created by Simon Fraser on Mon Jul 22 2002.
+ *  Copyright (c) 2001 __MyCompanyName__. All rights reserved.

Use the MPL in both the .mm and the .h?

+class nsDownloadListener : 	public nsIWebProgressListener,
+                            public nsIDownload

once again, tabs a-plenty in the .h and .mm files.

+nsDownloadListener::Init(...)
+{  
+  mController = [[ProgressDlgController alloc] initWithWindowNibName:
@"ProgressDialog"];

I would prefer a little bit of separation here. The nsDownloadListener probably
should live in the "cocoa embedding" layer that mozilla provides to encapsulate
gecko. In that case, it shouldn't be directly creating the progress dialog.
Could you make whoever creates this listener also create the controller and pass
it via a param to Init()? Also a protocol for the progress controller would help
reduce the depenency on a particular class impl.

Comment 17

[sairuh (rarely reading bugmail)]

bug numbers, wrt comment 15:

1. bug 151047 (maybe also bug 145807?): Hook up helper app launching (other bugs
exist)

2. bug 143093: Check in patch to make progress dialog localizable

3. remnant of bug 152629: Tweak dialog UI (fix the 'leave open' button toggle
madness)

4. remnant of bug 152629, see also bug 157826: See if we can hook up pause/resume

5. (this bug? couldn't find a separate one) Throttle the rate of progress
updates, since they slow down the download
   on fast connections

Comment 18

[Brian Ryner (not reading)]

Why does nsCocoaBrowserService still implement nsIDownload with this patch?

Comment 19

[Conrad Carlen (not reading bugmail)]

> I would prefer a little bit of separation here. The nsDownloadListener probably
should live in the "cocoa embedding" layer that mozilla provides to encapsulate
gecko.

  I would prefer a little more separation myself. I would like to see a factory
for the impl of nsIDownload split off into a separate file (the one containing
the impl). This object is a component override and therefore it should be a
piece which could be dropped in replaced with another impl without changing the
BrowserService. The way the BrowserService object implements a whole mess of
ifaces which have nothing to do with each other is not to my liking.
  The way the impl of nsIPromptService is done in PPEmbed, with minimal change
to the app (only a few lines to register the factory) is the way I think it
should be done. 

Comment 20

[Simon Fraser [no longer active]]

bryner: Because I forgot to remove it. New patch coming in the next hour or so.
conrad: I'm working on some changes along those lines, which also remove the
explicit instantiation of the progress window from the nsDownloadListener code.

Comment 21

[Simon Fraser [no longer active]]

Attachment: New patch making it easier for other cocoa embedders to use the download listener

Changes from the previous patch are mainly code reorganization to reduce the
dependencies between the progress UI and the download listener.

nsHeaderSniffer was moved to a new file; its code is the same, and it's still
made by the CHBrowserView to start a Save-initiated download.

I made a new file, DownloadFactories.mm, for factory methods creating objects
related to downloading (for now, just an nsIDownload factory).

DownloadProgressDisplay.mm/.h contain a C++ base class, nsDownloader, which has
on it the only few methods that the window controller needs to call, and an
Obj-C class that acts as a factory class for progress window controller
objects.
The header also specifies the DownloadProgressDisplay protocol, which a window
controller needs to conform to, to be able to use for downloading.

So progress windows are now instantiated by the nsIDownload::Init() method
calling CreateDownloadDisplay() on its base class nsDownload. That in turn asks
its DownloadProgressFactory (a factory of an Obj-C objects) to create the
progress window controller. The nsDownload is handed a factory when it's
created by DownloadListenerFactory (an XPCOM factory).

There are a few non-refactoring changes in the patch, relating to progress
dialog display. First, we have to make sure the dialog has been shown before
trying to set data on any of its outlets (fixes an issue with 'mFromText'
showing up in the dialog). Second, I had to add calls to [NSView display] on
the text fields after setting their values to get them to display right away.
This allows the dialog to show the right values even if it's just up for a
short time.

Comment 22

[Mike Pinkerton (not reading bugmail)]

+    id <DownloadProgressDisplay>  mDownloadDisplay;   // something that
implements the DownloadProgressDisplay protocol

I think the comment is self-evident, you can probably remove it

+@implementation DownloadProgressFactory : NSObject

are you sure you need to redeclare that it inherits from NSObject?

+  mDownloadDisplay = [mDownloadDisplayFactory createProgressWindowController];
+  [mDownloadDisplay setDownloadListener:this];

shouldn't |createProgressWindowController| take the d/l listener as a param
instead of forcing a caller to set it, and possibly get it wrong?

+#pragma mark -

will gcc warn about this unknown pragma? i think it will.

I still don't quite understand the rationale behind the nsDownloader baseclass.
If nsDownloadListener is the only thing that implements it, why have the extra
complexity? Why is it that bad that the ProgressController knows about
nsDownloadListener instead of nsDownload? I don't see how the embedding app is
affected by having refs to nsDownloadListener instead of nsDownload if it wants
to replace the controller we provide as part of the embedding layer. 

Comment 23

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 92512 [details] [diff] [review]
New patch making it easier for other cocoa embedders to use the download listener

smfr and i talked on aim. r=pink with some more explanitory comments

Comment 24

[Simon Fraser [no longer active]]

Patch checked in, with some class renaming to reduce confusion, a big comment
describing how it all works, some detabbing, and changing the licence of the new
files to MPL/LGPL/GPL.

I'll close this bug.

Comment 25

[Winnie Lam]

Verified with 07-24 build on OS 10.1.5.

1.  Single click invokes download dialog - PASS
2.  Cancel button - kills download dialog but file actually continues and
finishes downloading - PARTIAL - Filed bug 159162.
3.  Pause button - grayed out permanently (this has been logged in Bugzilla as a
polish issue) - PASS
4.  Leave Open/Close When Done buttons - toggle works - PASS
5.  Show File - grayed out before download is complete and works after download
is complete - PASS
6.  Open File - grayed out before download is complete and works after download
is complete - PASS
7.  Close button in title bar - same behavior as Cancel button, i.e. it kills
download dialog but file actually continues and finishes downloading - PARTIAL -
Filed bug 159162
8.  Minimize button in title bar - PASS
9.  Maximize button in title bar - PASS

Marking verified. 

Comment 26

[Winnie Lam]

Forgot to mention...

10.  Same look and feel as download dialog when context-clicking to download a
file - PASS

Comment 27

[sairuh (rarely reading bugmail)]

some more tests:

a. helper apps: able to save to machine, then open (via download window) with
app (tested w/pdf->Preview, bin->Stuffit Expander).

b. able to save via cmd+S, File > Save As, and context menu "save link as".

c. able to save as complete, save as html only and save as text. however, for
having as text (when chosing format from file picker), the d/l window remains
up/in progress --but i think there's already a bug on this...

going to retest some other scenarios.

(reopening to re-resolve since we don't really mark bugs as Closed --just
Verified. confusing, indeed.)

Comment 28

[sairuh (rarely reading bugmail)]

resolving again (sorry for the noise).

d. both ftp and http downloads appear fine.

(more tests...)

Comment 29

[sairuh (rarely reading bugmail)]

filed bug 159170 for the text format issue (c in comment 27).

e. if i leave the d/l window up and do another download, another d/l window
appears. iirc, this is expected behavior.

f. able to save framed content. (tho' you need to click or navigate to make a
frame active --in order to save just that one frame rather than the frameset.
see bug 159046 and bug 150049.)

whew. okay, re-verifying. am gonna do a few more tests anyhow (more edge-case),
just to be sure. as usual, if anyone else finds other regressions, feel free to
add/reopen as needed!