230449 - Make helper app progress dialog more customizeable

Status: NEW

(Firefox :: File Handling, defect)

Description

[Scott MacGregor]

The progress dialog invoked by the helper app service today is very browser
centric. Some of the features in it don't make much sense when you are opening
mail attachments in seamonkey. 

For instance, there is no notion of pausing/resuming saving of attachments for
an imap connection. So this button really should not be visible.

I'm filing this bug to look into supporting flags for controlling UI elements in
the progress dialog such as the presence of the Pause/Resume button. The
question is who has the ability to set such a flag? We could cheat in the
exthandler and have it look at the url scheme. Maybe a set of prefs which list
which protocols support pause and resume.

Or we could figure out a way for the mailnews code which opens the url to pass
in a flag for the current load which could later be checked by the progress dialog. 

I'd also like to eventually change the fact that we show a convoluted, private
imap/mailbox url in the "opening from" portion of the dialog, instead showing a
more friendly mail string such as the name of the account. But I suspect that
will be harder. 

cc'ing the heavy hitters for exthandler\uriloader stuff

Comment 1

[Boris Zbarsky [:bzbarsky]]

> For instance, there is no notion of pausing/resuming saving of attachments for
> an imap connection.

According to our network apis, all channels have a suspend method and there is
no way to really tell whether a channel is suspendable.

Worse yet, proper implementation of some of the rest of this helper app code
WILL require all channels to implement suspend/resume; any that don't will
simply break sometime in the not too far future.

None of this is relevant to the main purpose of this bug, directly, I guess....

ccing darin in case he has any ides on how this should work.

Comment 2

[Darin Fisher]

for the most part, i think it is only the mailnews protocols that lack support
for Suspend/Resume.

there is unfortunately no way to know if a protocol supports Suspend/Resume. 
one option is to define a new bit that is returned from nsIProtocolHandler::
protocolFlags.  if this bit is not set, then the download manager could assume
that Suspend/Resume are not supported.  this will not work well for older
drop-in channels that don't understand that they need to set this bit to get
their Suspend/Resume called, but probably that is an edge case.

or, the download manager could just try calling Suspend followed by Resume to
test if it works.  that's pretty hackish, but it would work with all of our
channels.

another option is to actually implement Suspend/Resume for the mailnews
protocols.  it should not be too difficult to do that since most of the channels
have a nsInputStreamPump under-the-hood, which supports Suspend/Resume.

Comment 3

[Scott MacGregor]

hah :)...I don't believe there is anyway to implement suspend and resume for
imap. If you pause the download and somehow block the imap connection then you
are going to freeze out all other imap actions on that folder such as selecting
another message.

Looking at your protocol flag suggestion, could we turn that around and have the
protocol set a bit flag if it does not support suspend / resume? Then you don't
have to worry about older drop in protocols suddenly not showing pause/resume in
the progress dialog because they don't know about this flag?

Thanks for the input/discussion fellas.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

mscott: Maybe it would be better if, for thunderbird, you just implemented your
own progress dialog (starting with forking seamonkey's)?

> If you pause the download and somehow block the imap connection then you
> are going to freeze out all other imap actions on that folder such as selecting
> another message.

couldn't you open a new connection to the server?

I would really prefer it to be implemented. The current plan for the helper app
code is to Suspend the channel once OnStartRequest is called and only call
Resume once the user chooses an action. That way, there's no need to save a file
in $TEMP, and it is possible to show a "View in browser" radiobutton in the
helper app dialog (for content-disposition:attachment, mostly). Also, it would
be possible to fix the content decoding usefully.

>i think it is only the mailnews protocols that lack support
>for Suspend/Resume.

there's also stream converters...

>another option is to actually implement Suspend/Resume for the mailnews
>protocols.  

only IMAP lacks that; there's a bug filed about it (bug 217434)

>I'd also like to eventually change the fact that we show a convoluted, private
> imap/mailbox url in the "opening from" portion of the dialog, instead showing 
> a more friendly mail string such as the name of the account.

Should we maybe use request->GetName on the request to find out what to show?
hm... that will probably fall apart for non-ascii urls.

Comment 5

[Darin Fisher]

scott, yeah...  i hadn't thought about the fact that the IMAP connection
wouldn't be used for anything else until the attachment is fully downloaded. 
spawning a new connection for each attachment is probably too costly, right?

biesi, shouldn't we try to avoid forking if possible.  i know there's lots in
the exthandler code that mailnews doesn't need (e.g., content-disposition), but
i think we should make forking our last option.  of course, it's not my decision
to fork or not fork, but it certainly seems like a lot of code to fork :-/

Comment 6

[[e-mail]]

> I would really prefer it to be implemented. The current plan for the helper
> app code is to Suspend the channel once OnStartRequest is called and only
> call Resume once the user chooses an action. That way, there's no need to
> save a file in $TEMP, and it is possible to show a "View in browser"
> radiobutton in the helper app dialog (for content-disposition:attachment,
> mostly). Also, it would be possible to fix the content decoding usefully.
What do we actually need from the channel to show the helper app dialog - I know
for HTTP we actually have to start downloading to get the information, but I
think IMAP already knows the file name and MIME type...

Comment 7

[Scott MacGregor]

Christian, yeah I could fork it for thunderbird but I was hoping to improve the
experience for seamonkey actually :), although tbird would benefit from it too. 

As far as making imap support pause/resume, I don't believe this is likely
anytime soon. IMAP only allows one connection per folder. You aren't allowed to
have multiple connections open. I read David's comments in the bug you cited, I
think he misunderstood...we can maybe figure out a way to stop the socket but
then you are effectively frozen out of doing any other imap operations in that
folder while the download is paused. I think making this a requirement of all
channels/protocols is a bad idea. 

Now, if a channel could advertise if it did indeed support pause/resume then you
could probably fix 69938 and 57342 for the urls opened by channels where this is
possible.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

ok btw... for the case of the pause button, I intend to make that actually
Cancel() the channel and use nsIResumableChannel for the resume. The button
would then only show up if the channel is qi'able to nsIResumableChannel and has
a non-null entity id (see that interface).
That way, resume would work even after the connection to the ISP gets lost and a
new IP is assigned; and you would be able to resume after a mozilla restart.

it would solve the main issue of comment 0 (that imap shows a pause button that
doesn't work)

Comment 9

[Darin Fisher]

biesi:

do we really have reports that Suspend leads to connections being dropped by
servers?  (bug on file?)  HTTP servers are usually ok leaving the connection
open for a long time.  i think it might make more sense to do the resumable
channel stuff only if normal Suspend fails (which requires some special
detection code in the stream listener).  the other issue with resumable channel
stuff is that in many cases it may not be possible to restart the channel.  the
data may have been dynamically generated, or it might require the user to
navigate a form (think Java SDK here) in order to start a new download (one
unique cookie per download).

Comment 10

[Darin Fisher]

i should add that the resumable channel stuff was originally meant as a partial
solution for resuming a download after the browser crashes.  it wasn't really
meant to be a generic solution to pause/resume.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

darin:
see bug 138664 and 150353. caused by disconnecting the modem, hence getting a
new ip.

your comment does not address the other reason, being able to pause the download
and resume it after a mozilla restart. but actually I realized that that's not
needed, I can save the entity id in any case...

>solution for resuming a download after the browser crashes.  it wasn't really
>meant to be a generic solution to pause/resume.

hm...
indeed, it would be possible to use Suspend/Resume while the browser is active,
and only use nsIResumableChannel otherwise... would be a bit easier, and not
cause problems with downloads that can not be restarted.

(only when the browser crashes? what if the user pauses a download himself, and
quits mozilla?)

doing that would not help this bug at all, of course :)

Comment 12

[Darin Fisher]

>(only when the browser crashes? what if the user pauses a download himself, and
>quits mozilla?)

yeah, i didn't mean to exclude those cases ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

So... it looks like we've gotten a little far afield on this pause/resume mess
(which we _do_ need implemented somehow, per comment 4.  We don't care whether
the actual network transport is paused, but the notifications to this listener
need to pause).

The real questions here are:

1)  Does the channel need to pass information to the exthandler?
2)  If so, what sort of information and how?

Comment 14

[[e-mail]]

Does anyone know whether this was fixed by the toolkit download manager?
I know it does at least support the idea of resumable downloads.

Comment 15

[Robert Kaiser]

If the download manager sets .resumable correctly, the new progress window does IIRC honor that and only show the button(s) for resumable downloads.

It's a bit more unclear to me what else this bug is requesting, and I suspect it might be better to file separate bugs on those other issues with crisp descriptions in comment #0.