Make nsWebBrowserPersist into a component, sort out refcount, progress issues

RESOLVED FIXED in mozilla1.0

Status

defect
RESOLVED FIXED
18 years ago
2 months ago

People

(Reporter: adamlock, Assigned: adamlock)

Tracking

Trunk
mozilla1.0
x86
All
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [from bugscape])

Attachments

(11 attachments)

Assignee

Description

18 years ago
nsWebBrowserPersist is currently an object internal to the webbrowser module. 
Add an entry to the module's factory list so it can be created by anyone.

Updated

18 years ago
QA Contact: mdunn → depstein
Assignee

Comment 2

18 years ago
Expanding bug to cover a few more things:

1. Refcounting. The webbrowser persist object currently destroys itself when 
it's done. This may run contrary to what people may expect. Consider adding a 
complete property to nsIWebBrowserPersist that people can poll to know when they 
can release the object.
2. Progress notifications don't indicate current progress (except for 
begin and end). Consider switching to nsIWebProgressListener, or adding some new 
flags and parameters.
Summary: Make nsWebBrowserPersist into a component → Make nsWebBrowserPersist into a component, sort out refcount, progress issues
Assignee

Comment 3

18 years ago
3. saveCurrentURI should be done away with. saveURI should accept nsnull to mean 
the current URI so there is no need for a seperate method.
4. Change webbrowser persist so a single object can handle multiple downloads - 
i.e. make it use load groups. This allows nsIWebBrowserPersist to give back sane 
progress complete messages.
Assignee

Updated

18 years ago
Blocks: 76585
Assignee

Updated

18 years ago
Target Milestone: --- → mozilla0.9.1

Updated

18 years ago
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Assignee

Updated

18 years ago
Blocks: 36419

Comment 5

18 years ago
Changed QA contact to dsirnapalli. Dharma's working with web browser persist.
QA Contact: depstein → dsirnapalli
Assignee

Comment 6

18 years ago
1.0
Target Milestone: mozilla0.9.2 → mozilla1.0
Assignee

Updated

18 years ago
Blocks: 11632

Comment 7

18 years ago
saving the whole page would really be a cool feature :)
Assignee

Comment 9

18 years ago
Points about most recent cut.

1. Refcounting is pretty sane now. Web browser persist hangs around until it's 
released - it doesn't try and release itself when its done.
2. Progress notifications now happen via the nsIWebProgressListener interface 
but I've still got to stick in the percentage completed notifications.
3. saveCurrentURI is done away with. New properties on nsIWebBrowserPersist are 
"status" and "listener". Status indicates current progress for those who want to 
poll for such info, listener is set by the caller if they want callbacks via 
nsIWebProgressListener.
4. A single web browser persist object handles all URIs linked to a single 
document.

Still to do:

1. Try out saving multi-frame documents. Should work but you never know.
2. Fix percentage complete notifications.
3. Go through code looking for redundant crap and TODO notes.
4. Fix up mfcEmbed with a progress listener to see how well it works from the 
client side.
5. Maybe put some flags onto persist interface which determines whether URIs are 
grabbed from cache or always fetched from the network.
Assignee

Comment 12

18 years ago
The IDL for the nsIWebBrowserPersist is attached for review. The new version 
allows the caller to listen to progress via a registered nsIWebProgressListener 
or to poll for changes to the currentState attribute.

The persistFlags property allows the caller to say whether content comes from 
cache or from the network and a few other flags for saving redirected content 
and IFrames.

Comment 13

18 years ago
adam, I thought this is important to our embedding customer. If that's the case
can we get it done in 0.9.2 branch?
Keywords: topembed
Assignee

Comment 14

18 years ago
Is the old webbrowser persist good enough for the 0.9.2 branch or is there a 
pressing need for the new version? I now have a working patch (which follows 
this comment) but it represents nearly 1000 lines of new or changed code and is 
bound to have bugs in it.
Assignee

Comment 15

18 years ago
Posted patch Working patchSplinter Review

Comment 16

18 years ago
Adam, do you think the patch worth a try?

The part I am concerned is Download Progress which is very important for 
embedding app (try to imagine the users right click on a large file such as 
mozilla tar ball and select save as, it will take a long time for the file to 
save), will it be simple to seperate this part out from the patch so that we 
can take less risk?

I will let Glen decide whether we want to take the risk. If not I will probably 
remove the progress dialog box and do sillent saving like the way Mfcembed does.
Assignee

Comment 17

18 years ago
I will try and make a patch against 0.9.2 for you to try out for Monday. The
basic functionality appears okay but I am concerned that whatever you were using
the old version for might suddenly not work anymore.
Assignee

Comment 19

18 years ago
Mac associates extra information with each file, so patch will have to be 
modified slightly for the Mac to add calls to 
nsILocalFile::SetFileTypeAndCreator.

Updated

18 years ago
Whiteboard: adam is on vacation until early august 2001
Assignee

Updated

18 years ago
Blocks: 92534

Comment 20

18 years ago
pulling topembed, and comment about adam being on vacation. we should move
forward w/ this on the trunk though.
Keywords: topembed
Whiteboard: adam is on vacation until early august 2001
Assignee

Comment 21

18 years ago
*** Bug 92534 has been marked as a duplicate of this bug. ***

Updated

18 years ago
Blocks: 98550

Updated

18 years ago
Blocks: 98995
Assignee

Updated

18 years ago
Blocks: 99642

Comment 23

18 years ago
candidate for nsbranch+ 
Keywords: nsbranch, topembed
Whiteboard: [from bugscape]
Looking at the latest patch + the complete impl you mailed me, looks great.
Minor things:

(1) saveURI, if given NULL for aURI will save the current URI (in nsWebBrowser's
impl of the iface, but not in nsWebBrowserPersist's). That should be in the doc
somewhere. Is the only implementation callers use directly be that of nsWebBrowser?
(2) In nsWebBrowserPersist::GetInterface(), why is the case for
nsIProgressEventSink special-cased instead of going though QueryInterface().
nsIProgressEventSink is in the object's iface map.

Since those are minor, r=ccarlen

Comment 28

18 years ago
Comment on attachment 49924 [details] [diff] [review]
New version fixes Conrad's issues. Submitting this one for sr

sr=rpotts@netscape.com

Looks good.  Let's check it in and deal with the bugs :-)

What is the 'cancel' semantic?  If a new page is loaded while the current page is being saved (ie. Cancel() is called on the docShell) should the saveDocument operation be cancelled too?
Attachment #49924 - Flags: superreview+
Assignee

Comment 29

18 years ago
Thanks all, fix is checked into the trunk. Anyone who's interested can start
beating on it to shake out any bugs that may remain.

Rick, the cancelSave method basically sets a flag that aborts the stream
download the next time nsIStreamListener::OnDataAvailable is called. At that
point all open streams are closed and the nsIWebBrowserPersist::result attribute
is set to NS_BINDING_ABORTED. There is no relationship between how the
webbrowser persist object and the docshell work - they're totally independent of
each other. If you cancel in the docshell, the web browser persist object keeps
on going, even if it's downloading the same content. If you cancel in the
docshell and tell the web browser persist object to save the docshell's half
completed DOM document, that's what it will save without trying to get the rest.
I'll leave this bug open until I know if this fix is wanted for the branch.

Comment 30

18 years ago
Looks like this is fixed and has reviews.  If so, and you think it's a critical
bug, please mark nsbranch+ so it gets on the PDT radar.   If PDT marks it as
PDT+ you can check it into the branch.

Comment 31

18 years ago
Some forewarning would have been nice. :-)

I'm now scrambling to get galeon to compile again. But I'm not complaining
very loud, we really wanted this to land so we can stop using the stream transfer
service and it's non-overridable progress dialog.

Comment 32

18 years ago
chris, judson - is this topembed bug needed on the branch?  

Comment 33

18 years ago
no. I'm removing "topembed" keyword.
Keywords: topembed

Comment 34

18 years ago
I'm going to add nsbranch-.
Keywords: nsbranchnsbranch-
Assignee

Comment 35

18 years ago
Patch is not going in the branch so I'm marking this fixed.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Updated

18 years ago
No longer blocks: 98550

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.