Closed
Bug 77909
Opened 23 years ago
Closed 23 years ago
Make nsWebBrowserPersist into a component, sort out refcount, progress issues
Categories
(Core Graveyard :: Embedding: APIs, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.0
People
(Reporter: adamlock, Assigned: adamlock)
References
Details
(Whiteboard: [from bugscape])
Attachments
(11 files)
2.69 KB,
patch
|
Details | Diff | Splinter Review | |
22.62 KB,
patch
|
Details | Diff | Splinter Review | |
37.46 KB,
patch
|
Details | Diff | Splinter Review | |
78.61 KB,
patch
|
Details | Diff | Splinter Review | |
3.63 KB,
text/plain
|
Details | |
87.77 KB,
patch
|
Details | Diff | Splinter Review | |
88.53 KB,
patch
|
Details | Diff | Splinter Review | |
92.14 KB,
patch
|
Details | Diff | Splinter Review | |
94.39 KB,
patch
|
rpotts
:
superreview+
|
Details | Diff | Splinter Review |
1.17 KB,
text/plain
|
Details | |
3.68 KB,
text/plain
|
Details |
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•23 years ago
|
QA Contact: mdunn → depstein
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
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.
Updated•23 years ago
|
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Comment 5•23 years ago
|
||
Changed QA contact to dsirnapalli. Dharma's working with web browser persist.
QA Contact: depstein → dsirnapalli
Comment 7•23 years ago
|
||
saving the whole page would really be a cool feature :)
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 10•23 years ago
|
||
Assignee | ||
Comment 11•23 years ago
|
||
Assignee | ||
Comment 12•23 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•23 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•23 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•23 years ago
|
||
Comment 16•23 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•23 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 18•23 years ago
|
||
Assignee | ||
Comment 19•23 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•23 years ago
|
Whiteboard: adam is on vacation until early august 2001
Comment 20•23 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•23 years ago
|
||
*** Bug 92534 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 22•23 years ago
|
||
Comment 23•23 years ago
|
||
candidate for nsbranch+
Comment 24•23 years ago
|
||
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
Assignee | ||
Comment 25•23 years ago
|
||
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Comment 27•23 years ago
|
||
Comment 28•23 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•23 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•23 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•23 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•23 years ago
|
||
chris, judson - is this topembed bug needed on the branch?
Assignee | ||
Comment 35•23 years ago
|
||
Patch is not going in the branch so I'm marking this fixed.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•