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

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Adam Lock]

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.

Comment 1

[Adam Lock]

Attachment: Patch adds webbrowser persist to factory list

Comment 2

[Adam Lock]

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.

Comment 3

[Adam Lock]

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.

Comment 4

[Adam Lock]

Attachment: Work in progress. Does most of the above points, but with some things still to resolve

Comment 5

[David Epstein]

Changed QA contact to dsirnapalli. Dharma's working with web browser persist.

Comment 6

[Adam Lock]

1.0

Comment 7

[Markus Hübner]

saving the whole page would really be a cool feature :)

Comment 8

[Adam Lock]

Attachment: Practically everything in place now, just web progress and bugs to sort out.

Comment 9

[Adam Lock]

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.

Comment 10

[Adam Lock]

Attachment: Another day's worth of work. Shape is almost there but some problems with subframes and node fixup in this diff

Comment 11

[Adam Lock]

Attachment: nsIWebBrowserPersist.idl on its own for clarity

Comment 12

[Adam Lock]

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

[Marek Z. Jeziorek]

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?

Comment 14

[Adam Lock]

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.

Comment 15

[Adam Lock]

Attachment: Working patch

Comment 16

[Ed]

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.

Comment 17

[Adam Lock]

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.

Comment 18

[Adam Lock]

Attachment: Same patch with small fixup against 0.9.2 branch

Comment 19

[Adam Lock]

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

Comment 20

[Judson Valeski]

pulling topembed, and comment about adam being on vacation. we should move
forward w/ this on the trunk though.

Comment 21

[Adam Lock]

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

Comment 22

[Adam Lock]

Attachment: Patch that builds on Mac & Win32 - just Unix to go

Comment 23

[Marek Z. Jeziorek]

candidate for nsbranch+ 

Comment 24

[Conrad Carlen (not reading bugmail)]

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 25

[Adam Lock]

Attachment: New version fixes Conrad's issues. Submitting this one for sr

Comment 26

[Adam Lock]

Attachment: A sanity tester for web browser persist - written in HTML & JS - part 1

Comment 27

[Adam Lock]

Attachment: A sanity test for web browser persist - written in HTML & JS - part 2

Comment 28

[rpotts (gone)]

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

sr=[e-mail]

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?

Comment 29

[Adam Lock]

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

[Cindy Roberts]

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

[Philip Langdale]

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

[lchiang]

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

Comment 33

[Judson Valeski]

no. I'm removing "topembed" keyword.

Comment 34

[lchiang]

I'm going to add nsbranch-.

Comment 35

[Adam Lock]

Patch is not going in the branch so I'm marking this fixed.