Downloading is broken in MfcEmbed and in Mac builds(Regression)

VERIFIED FIXED in mozilla1.3alpha

Status

()

Core
Embedding: APIs
P2
critical
VERIFIED FIXED
16 years ago
4 years ago

People

(Reporter: Jeff Doozan, Assigned: Alec Flett)

Tracking

({helpwanted, topembed-})

Trunk
mozilla1.3alpha
helpwanted, topembed-
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
Attempting to download a file using MfcEmbed results in the filepicker dialog,
but the progress bar never gets displayed, and the file is not saved.

Test Case:

Click: ftp://ftp.mozilla.org/pub/ls-lR.gz (note: it happens with HTTP files, too)

Expected results:

 - File picker dialog appears
 - User chooses file name/location
 - Progress dialog is displayed while the file is downloading
 - The file is saved at the location specified by the user

Actual Results:

 - File picker dialog appears
 - User chooses file name/location
 - No Progress dialog
 - The file is not saved

Updated

16 years ago
QA Contact: mdunn → depstein

Comment 1

16 years ago
-> Chak
Assignee: adamlock → chak

Comment 2

16 years ago
This feature has always worked fine and now it's broken.

Alec/Blake : Do you think this is related to the recent nsIDownload changes?



(Assignee)

Comment 3

16 years ago
ack, this might be me.
The basic problem is that we need an embedding-only dll to do some stub work to
make the progress dialog appear.

that said, we supposed to make sure that the file would continue to be saved as
before, but the progress dialog wouldn't be shown. We're certain the file isn't
being saved anyway?

Comment 4

16 years ago
Hi Alec : 

Yes, the file's not being saved.

Please take a look at the comments towards the top of
mfcembed/components/HelperAppDlg.cpp to see how MfcEmbed was handling downloads etc.

I'm not sure if that's still the right way in the current state of things?

Thanks
(Assignee)

Comment 5

16 years ago
for my own reference while working on this bug:
http://lxr.mozilla.org/seamonkey/source/embedding/tests/mfcembed/components/HelperAppDlg.cpp

Anyhow, the uriloader is supposed to be opening the initial progress dialog..
but what was SUPPOSED to happen for embedding was that the progress dialog was
going to stop opening, but the uriloader would still pump the download. I'll see
if I can figure out how all these things are hooked up. (I only know it starting
in the uriloader, I don't know where the initial uri load gets kicked off)

Comment 6

16 years ago
Rick : Do you know the answer to Alec's question regarding the uriloader..thanks
we're seeing the same thing on mac as well (bug 134523)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: topembed
OS: Windows NT → All
Hardware: PC → All
(doh, i meant 137622) ;)

Comment 9

16 years ago
Per Mike's comments above, this seems more than an MfcEmbed issue. 

Sending it Alec's way to address the core issue here.

Alec : Please reassign if you're not the owner...thanks
Assignee: chak → alecf
Summary: Downloading is broken in MfcEmbed (Regression) → Downloading is broken in MfcEmbed and in Mac builds(Regression)
*** Bug 137622 has been marked as a duplicate of this bug. ***
CC'ing Law since he set up the downloader UI as an embed component.
(Assignee)

Comment 12

16 years ago
ok, I'm on this. I think this is blake's fault though :) (or my fault for
reviewing :))
Status: NEW → ASSIGNED
embedding clients need this asap, it's a blocker and needs to be on the _branch_
by friday.
Severity: critical → blocker
(Assignee)

Comment 14

16 years ago
Created attachment 80680 [details] [diff] [review]
band-aid fix

Ah ha! I found the problem. mProgressWindowCreated is a flag which determines
if the progress window worked. In the embedded case, we don't have a progress
window available.. so we just need to fake out the external helper app handler.


here's a quick hack that will fix the problem for now. We really should just
rename this boolean variable to something like "mProgressValid" - because it is
a valid state to have no progress window.

I haven't tested this just yet because I wanted to get the patch up ASAP. Feel
free to test this, I'm going to fiddle with it now.
(Assignee)

Comment 15

16 years ago
so assuming this works (Still waiting on my mfcembed build) - what branch do I
check this into? 1.0? 0.9.9?
Priority: -- → P1
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 16

16 years ago
yep, that fixed it. Now I just need some reviews.
adding the usual nomination keywords.
Keywords: adt1.0.0, mozilla1.0

Updated

16 years ago
Attachment #80680 - Flags: superreview+

Comment 17

16 years ago
Comment on attachment 80680 [details] [diff] [review]
band-aid fix

sr=blake
> In the embedded case, we don't have a progress
window available.. so we just need to fake out the external helper app handler.

Assuming the embedding app wants a progress window, it can just provide or
register a component with the contact ID "@mozilla.org/download;1" which
implements the nsIDownload iface. I have a feeling I'll be needing to make one
soon for PPEmbed.

Comment 19

16 years ago
So, by incorrectly stating that a progress dialog was created, are we setting
ourselves up for code that relies on that progress dialog being there to fail,
in other words, code that may send notifications, but there is no one listening?

Seems like we should simply decouple the processing of the content from the
existence of the progress dialog in order to address this... is the better fix
to change:

1372   if (mProgressWindowCreated && !mCanceled)
1373   {
1374     nsMIMEInfoHandleAction action = nsIMIMEInfo::saveToDisk;

to

1372   if (!mCanceled)
1373   {
1374     nsMIMEInfoHandleAction action = nsIMIMEInfo::saveToDisk;

in
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1372

since we don't seem to care either way according to your patch, and so that the
rest of the code that looks at mProgressWindowCreated, for example:

http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1576

1576     if (!mProgressWindowCreated) 
1577       ShowProgressDialog();

will then be accurate? (I'm afraid of someone coming in and adding code at a
later time assuming that mProgressWindowCreated is actually telling the truth,
leading to regressions).
Sorry, "it can just provide or register a component with the contact ID
"@mozilla.org/download;1" which implements the nsIDownload iface." was supposed
to be a question.

In other words, if the embedding app wants a progress dialog:
 it can either do that? If so, is there anything else required?
or
 should we add that component to the embed package in case they want to use the
XUL progress dialog?
And, in addition to @mozilla.org/download;1, would you also need to implement 
@mozilla.org/download-manager;1? Yikes, that's a lot of work. Can somebody who
designed this summarize what's the minimum an embedding app needs to provide in
order to complete a download *with* a progress dialog? We then need a native
impl as an example of how an embeddor would do this but, in the short term, the
required bits need to be in the embedding package. 
what if the embedded app wants to use the _xul_ progress window that we bundle
up so nicely in embed.jar? why is that failing?

what exactly is the "download component"? are we not packaging something
correctly? I'm still a little confused why embedding doesn't just pick this up
from seamonkey.
(Assignee)

Comment 23

16 years ago
conrad: all you need to do is implement @mozilla.org/download;1 which implements
nsIDownload - the goal was to abstract out the concept of a "dialog" from the
external helper app handler, and allow a download object to create whatever kind
of progress object it wants (i.e. in the case of an embeddor, they could make a
progress dialog, in the case of seamonkey, they update the download manager)

syd: by "incorrectly" stating that a progress dialog is created, we are making
the code behave correctly. I think that it is LESS risky to do my patch, than to
remove the "if (mProgressWindowCreated..." because there may be OTHER cases
where ShowProgressDialog has NOT been called, where we really do need the state
of mProgressWindowCreated to be PR_FALSE. 

The real confusion is basically that the helper app handler no longer has the
concept of a progress window, so really this variable is named incorrectly. The
variable should be named "mProgressValid" as I stated in comment. I have every
intention of cleaning this up on the trunk - in fact I'll go so far as to say
that the above patch is for the BRANCH ONLY, unless ADT requests that it bake on
the trunk.

pinkerton: the reason is that the seamonkey version is now tied into the
download manager, and is built into the "appcomps" DLL, which is not part of
embedding. In order for embedded apps to use our XUL progress dialog, they will
have to create a stub object which instantiates the XUL progress dialog rather
than our download manager.
but we have the appcomps dll (which embedded apps do need for global history)
and it still doesn't work. what else could be failing?
Attachment #80680 - Flags: review+
Comment on attachment 80680 [details] [diff] [review]
band-aid fix

I see why this works but how about a quick search & replace on that variable
name? r=ccarlen

Comment 26

16 years ago
I think alec pretty much summed it up, but just to reiterate: no, embedding
clients don't have to implement nsIDownloadManager, only the tiny nsIDownload. 
I created the embedding version of nsIDownload but there was apparently no place
to stick it that wasn't brought in by seamonkey, so I was told to just not show
progress for downloads in embedded builds for the time being (but the download
still should have occurred, which is this bug).
Created attachment 80876 [details] [diff] [review]
package everything to use stub JS component

this registers the js component to handle downloads in PPEmbed and sets up
packaging for mac (stubbed out on other platforms in case they don't want the
XUL). 

Also fixes a problem with our embed.jar manifest where it assumed we were
running on unix. This caused mac packaging to miss a bunch of property files
and xul.

needs r/sr, in addition to alecf's patch. ASAP, please, we're on a tight
deadline.
Created attachment 80880 [details] [diff] [review]
same as above, but uses en-mac on mach-o builds
Attachment #80876 - Attachment is obsolete: true
Created attachment 80882 [details] [diff] [review]
take 3, hopefully full patch
Attachment #80880 - Attachment is obsolete: true
Created attachment 80883 [details] [diff] [review]
f'ing mozilla keeps trunctating the patch
Attachment #80882 - Attachment is obsolete: true
grrrr. here's the last bit that moz keeps trunctating

Index: mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul
===================================================================
RCS file: /cvsroot/mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul,v
retrieving revision 1.13.20.1
diff -u -2 -r1.13.20.1 nsHelperAppDlg.xul
--- mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul	10 Apr 2002 02:37:51 -0000	1.13.20.1
+++ mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.xul	24 Apr 2002 23:51:18 -0000
@@ -25,5 +25,5 @@
 -->
 
-<?xml-stylesheet href="chrome://navigator/skin/" type="text/css"?> 
+<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
 
 <?xul-overlay href="chrome://global/content/dialogOverlay.xul"?>

nothing else below that. just r/sr it.
Comment on attachment 80883 [details] [diff] [review]
f'ing mozilla keeps trunctating the patch

r=bryner
Attachment #80883 - Flags: review+
Attachment #80883 - Flags: superreview+
Comment on attachment 80883 [details] [diff] [review]
f'ing mozilla keeps trunctating the patch

sr=ben@netscape.com
Comment on attachment 80883 [details] [diff] [review]
f'ing mozilla keeps trunctating the patch

a=rjesup@wgate.com for branch checkin
Attachment #80883 - Flags: approval+

Comment 35

16 years ago
Is this needed for MachV, or solely for embedding. If needed for embedding, when
does it need to be on the 1.0 branch (pls provide a date).
Whiteboard: [adt3] [ETA Needed]
they need this today. asap. ETA an hour ago. this does not impact machV at all,
embedding only.
Whiteboard: [adt3] [ETA Needed] → [adt1] [ETA 4/24/02]
Keywords: topembed → topembed+
(Assignee)

Comment 37

16 years ago
I'm reassigning this to you pink, since you're doing the bulk of the work here.
I'm landing my other band-aid fix on the trunk now and will clean up in another bug.
(Assignee)

Comment 38

16 years ago
the new bug is bug 140136
Assignee: alecf → pinkerton
Status: ASSIGNED → NEW

Comment 39

16 years ago
adt1.0.0+ (on ADT's behalf) approval for checking in on the 1.0 branch. Pls land
this today, then add the fixed1.0.0 keyword.
Keywords: adt1.0.0 → adt1.0.0+
Whiteboard: [adt1] [ETA 4/24/02] → [adt1] [ETA - Needed NOW for embed]
fixed on branch. trunk upcoming
Status: NEW → ASSIGNED
Keywords: adt1.0.0+ → fixed1.0.0
landed on trunk (whew)
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 42

16 years ago
Comment on attachment 80680 [details] [diff] [review]
band-aid fix

a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #80680 - Flags: approval+

Comment 43

16 years ago
Just checked this with MFCEmbed 5/7/2002 nightly build from the trunk and it is
NOT working. Running the test case for the bug still does not generate the
progress dialog while the file is downloading. It does appear that files are
being downloaded so that is progress!
:)

Does alecf's patch fix this problem or should a new bug be opened on that? This
bug was intended to cover the progress dialog.
i downloaded the 5/8 PPEmbed build on the trunk and this works just fine for me.
i get all the d/l progress dialogs.

Comment 45

16 years ago
I missed Alecf's new bug here:

http://bugzilla.mozilla.org/show_bug.cgi?id=140136

That bug hasn't completed the review process to be checked in. Is that bug 
intended to address the download progress dialog issue?  Mike indicates that it 
appears in PPEmbed but it is not appearing in MFCEmbed. 

Comment 46

16 years ago
checked with MfcEmbed on WinNT4.0 with Mozilla 1.0.0 Gecko 20020509 build. I
wasn't able to download the nightly build with it. First, got a 'channel doesn't
have a prompt' assertion, then the open/save dialog. But when I tried saving it,
it either doesn't save (prog dlog doesn't appear) or I got a memory crash.
Reopening.

Here's the crash log:
nsDownloadManager::AssertProgressInfoFor(const char * 0x0353e0f0) line 269 + 48
bytes
nsDownloadManager::AssertProgressInfo() line 255 + 12 bytes
nsDownloadManager::Open(nsDownloadManager * const 0x03b3c840, nsIDOMWindow *
0x00000000) line 660
nsDownloadProxy::Init(nsDownloadProxy * const 0x0353c1b0, nsIURI * 0x035214c0,
nsILocalFile * 0x0353c420, const unsigned short * 0x00000000, const unsigned
short * 0x0353e770, __int64 1021064481500000, nsIWebBrowserPersist * 0x00000000)
line 79 + 34 bytes
nsExternalAppHandler::ShowProgressDialog() line 1475 + 78 bytes
nsExternalAppHandler::SaveToDisk(nsExternalAppHandler * const 0x03522084,
nsIFile * 0x00000000, int 0) line 1597
CHelperAppLauncherDialog::Show(CHelperAppLauncherDialog * const 0x03536950,
nsIHelperAppLauncher * 0x03522084, nsISupports * 0x00d7b460) line 244 + 16 bytes
nsExternalAppHandler::OnStartRequest(nsExternalAppHandler * const 0x03522080,
nsIRequest * 0x03527f30, nsISupports * 0x00000000) line 1169 + 87 bytes
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x03527e40,
nsIRequest * 0x03527f30, nsISupports * 0x00000000) line 229 + 34 bytes
nsFTPChannel::OnStartRequest(nsFTPChannel * const 0x03527f40, nsIRequest *
0x03526b10, nsISupports * 0x00000000) line 676 + 48 bytes
DataRequestForwarder::DelayedOnStartRequest(nsIRequest * 0x035264f0, nsISupports
* 0x00000000) line 280
DataRequestForwarder::OnDataAvailable(DataRequestForwarder * const 0x03526b14,
nsIRequest * 0x035264f0, nsISupports * 0x00000000, nsIInputStream * 0x03526700,
unsigned int 0, unsigned int 2720) line 356 + 19 bytes
nsOnDataAvailableEvent::HandleEvent() line 193 + 70 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x03523164) line 116
PL_HandleEvent(PLEvent * 0x03523164) line 596 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00d3e6a0) line 526 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x0455039a, unsigned int 49517, unsigned int 0,
long 13887136) line 1077 + 9 bytes
US
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 47

16 years ago
ack! nsDownloadManager isn't supposed to be part of the embedded build. I
thought appcomps.dll wasn't in the default embedded product.

Comment 48

16 years ago
It was our understanding that appcomps wasn't going to be needed. We were
including it for FTP but alecf fixed that bug a while ago.  

Comment 49

16 years ago
*** Bug 137643 has been marked as a duplicate of this bug. ***
well, it works in mac builds now. over to alecf for other issues.
Assignee: pinkerton → alecf
Status: REOPENED → NEW

Comment 51

16 years ago
Is this fixed on the trunk, as well as the branch? If so, pls resolve as fixed.
thanks!

Comment 52

16 years ago
Alecf checked in a patch for:

http://bugzilla.mozilla.org/show_bug.cgi?id=140136

but that doesn't seem to have solved the downloading dialog problem. I just
tested that in MFCEmbed June 4, 2002 nightly and you still don't get a
downloading dialog.

I don't mean to make a go on and on about this but I can't imagine too many
embedders using the Mozilla 1.0 source when it doesn't support downloading, at
least on Windows platforms.

Updated

16 years ago
Blocks: 126245

Comment 53

16 years ago
I'm seeing this problem in the ActiveX control as well for my work on bug 126245.

Can we have some kind of fallback behaviour so that
nsIHelperAppLauncherDialog::ShowProgressDialog is called on the embedding apps
implementation when nsIDownload is not available? 

It seems weird that embedders have to implement yet another object when there's
a ShowProgressDialog method on an interface they do implement. It doesn't appear
to be called anywhere.
(Assignee)

Comment 54

16 years ago
ah, that could work... I didn't know embeddors already implemented that. I'll
see what I can do.
Status: NEW → ASSIGNED

Comment 55

16 years ago
works for me with mozilla a1 on windows xp

Comment 56

16 years ago
Please read the summary and the comments. This bug affects embedded builds of
Mozilla, not the main Mozilla distribution. The fact that it WORKSFORME in
Mozilla is irrelevant to this bug.

Comment 57

16 years ago
Hi Andrew : Could you please attach the patch that K-Meleon folks came up with
to fix this issue for review by Alec/others...thanks
(Assignee)

Updated

16 years ago
Target Milestone: mozilla1.0 → mozilla1.1beta
(Assignee)

Comment 58

16 years ago
ugh, I was just looking at this.. it looks really messy to me right now, but
maybe I just need to wrap my head around this. It looks to ME like we're using
nsIHelperAppLauncherDialog in:
  OnStartRequest() we call Show() to ask if we can save to disk
  PromptForSaveToFile to prompt where we're saving the file

but we're NOT using it for progress. I'm now wondering if all this nsIDownload
junk belongs in seamonkey's nsIHelperAppLauncherDlg implementation.. if I can
find it :)
I wonder if we used to do it that way, before the nsIDownload stuff landed.
(Assignee)

Comment 59

16 years ago
moving bugs to mozilla 1.2alpha that missed 1.1beta
Target Milestone: mozilla1.1beta → mozilla1.2alpha
(Assignee)

Comment 60

15 years ago
this is no longer a blocker - downloads work now, they just don't show progress.
Lowering to "critical"
Severity: blocker → critical
Priority: P1 → P2
(Assignee)

Comment 61

15 years ago
From what I understand, downloading is no longer broken, so I'm punting this to
1.3alpha and/or hoping someone else will take care of it... (or fix the subject
line to tell us what is really going on)
Keywords: helpwanted
Target Milestone: mozilla1.2alpha → mozilla1.3alpha

Comment 62

15 years ago
Clearing status whiteboard since it applied only to the part of the bug that is
now fixed.  This remains open only due to the progress dialog now.

Removing the topembed+ so that triage can happen again here.
Keywords: fixed1.0.0, topembed+ → topembed
Whiteboard: [adt1] [ETA - Needed NOW for embed]

Comment 63

15 years ago
Topembed triage: minusing this bug.

We should probably close this bug as fixed and open a new one regarding the
progress meter implementation.  Opinions?  Alec?
Keywords: topembed → topembed-
(Assignee)

Comment 64

15 years ago
yeah, this bug is a mess at this point. I don't even know what we should be
doing here, and I wonder if the fix might fall out of the mime work that might
eventually be done by darin and friends.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago15 years ago
Resolution: --- → FIXED

Comment 65

15 years ago
Last I checked, this is still only half-fixed per the initial report as the 
progress dialog isn't appearing in Windows builds. However, the focus was the 
actual downloading of the file and that does appear to be working consistently. 
So, a new bug focused just on the progress meter issue is probably in order. 
Hopefully, that will keep it from falling off the radar. Who wants to file the 
bug?  Alecf?
:)
FWIW, since this bug was active, PPEmbed (Mac builds) got its own native
download UI impl and doesn't have this problem.

Comment 67

15 years ago
verified against MfcEmbed in Mozilla 1.2b Mozilla/5.0 (Windows; U; WinNT4.0;
en-US; rv:1.2b) Gecko/20021026 build. Download works fine, but as noted,
progress dialog isn't working. Will close out this bug and file one addressing
the prog dlog in MfcEmbed.
Status: RESOLVED → VERIFIED

Comment 68

15 years ago
submitted bug 177408 for prog dlog not working in MfcEmbed. Didn't see any bug
submitted for this, but if there is, please dup to it. Thanks.
You need to log in before you can comment on or make changes to this bug.