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

RESOLVED FIXED in mozilla1.0

Status

()

Core
Embedding: APIs
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Adam Lock, Assigned: Adam Lock)

Tracking

Trunk
mozilla1.0
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [from bugscape])

Attachments

(11 attachments)

(Assignee)

Description

17 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.
(Assignee)

Comment 1

17 years ago
Created attachment 32430 [details] [diff] [review]
Patch adds webbrowser persist to factory list

Updated

17 years ago
QA Contact: mdunn → depstein
(Assignee)

Comment 2

17 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

17 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)

Comment 4

17 years ago
Created attachment 32758 [details] [diff] [review]
Work in progress. Does most of the above points, but with some things still to resolve
(Assignee)

Updated

17 years ago
Blocks: 76585
(Assignee)

Updated

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

Updated

17 years ago
Target Milestone: mozilla0.9.1 → mozilla0.9.2
(Assignee)

Updated

17 years ago
Blocks: 36419

Comment 5

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

Comment 6

17 years ago
1.0
Target Milestone: mozilla0.9.2 → mozilla1.0
(Assignee)

Updated

17 years ago
Blocks: 11632

Comment 7

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

Comment 8

17 years ago
Created attachment 40652 [details] [diff] [review]
Practically everything in place now, just web progress and bugs to sort out.
(Assignee)

Comment 9

17 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 10

17 years ago
Created attachment 41212 [details] [diff] [review]
Another day's worth of work. Shape is almost there but some problems with subframes and node fixup in this diff
(Assignee)

Comment 11

17 years ago
Created attachment 41215 [details]
nsIWebBrowserPersist.idl on its own for clarity
(Assignee)

Comment 12

17 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

17 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

17 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

17 years ago
Created attachment 41339 [details] [diff] [review]
Working patch

Comment 16

17 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

17 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

17 years ago
Created attachment 41637 [details] [diff] [review]
Same patch with small fixup against 0.9.2 branch
(Assignee)

Comment 19

17 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

17 years ago
Whiteboard: adam is on vacation until early august 2001
(Assignee)

Updated

17 years ago
Blocks: 92534

Comment 20

17 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

17 years ago
*** Bug 92534 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 22

17 years ago
Created attachment 48446 [details] [diff] [review]
Patch that builds on Mac & Win32 - just Unix to go

Updated

17 years ago
Blocks: 98550

Updated

17 years ago
Blocks: 98995
(Assignee)

Updated

17 years ago
Blocks: 99642

Comment 23

17 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
(Assignee)

Comment 25

17 years ago
Created attachment 49924 [details] [diff] [review]
New version fixes Conrad's issues. Submitting this one for sr
(Assignee)

Comment 26

17 years ago
Created attachment 49925 [details]
A sanity tester for web browser persist - written in HTML & JS - part 1
(Assignee)

Comment 27

17 years ago
Created attachment 49926 [details]
A sanity test for web browser persist - written in HTML & JS - part 2

Comment 28

17 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

17 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

17 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

17 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

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

Comment 33

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

Comment 34

17 years ago
I'm going to add nsbranch-.
Keywords: nsbranch → nsbranch-
(Assignee)

Comment 35

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

Updated

17 years ago
No longer blocks: 98550
You need to log in before you can comment on or make changes to this bug.