113902 - Make webbrowserperist more modular

Status: RESOLVED WONTFIX

(Core Graveyard :: Embedding: APIs, defect)

Description

[Adam Lock]

The Webbrowserpersist object contains all the code for storing URIs and fixing
up nodes during persistence. It would be useful to move the URI storage and
cloning functionality into another object where it could be overridden for
callers who wanted to modify this behaviour, for example to optionally not fix
up stuff that would otherwise be fixed up.

The new objects interface would probably have methods equivalent to the existing
StoreURIAttribute & CloneNodeWithFixedUpURIAttributes and a new ShouldPersistURI
method, so that:

1. The new object is called when a URI is to be stored
2. The new object can prevent any URI from being persisted
3. The new object is asked to clone and fixup nodes that need changing during
persistence.

nsDOMWalker could also be turned into an object so it may be used independently
from the webbrowserpersist object.

There should also be new "don't save" flag that does the DOM walking part of
saving but doesn't actually save anything to disk.

Comment 1

[David Epstein]

changed qa contact to Dharma. Web Browser Persist is his area.

Comment 2

[Adam Lock]

I created a patch for bugscape 11524 which gives an indication of my thoughts
for this bug. Namely a callback object that webbrowserpersist calls at opportune
times to modify nodes or paths as they were being persisted. The patch for this
bug would use a similar callback mechanism but it would be more sophisticated.

http://bugscape.netscape.com/show_bug.cgi?id=11524

Comment 3

[Adam Lock]

Bug 120659 will implement simple upload functionality to remote URIs. This bug
should also implement callback hooks so the client can provide their own
outputstreams for each URI if the default functionality is insufficient.

Comment 4

[Adam Lock]

Attachment: Sample IDL for the callback

Kathy can you take a look at this sample nsIWebBrowserPersistCallback interface
to see it makes sense to you? Basically I see the implementor of this object
setting this before persisting something and being called to supply their own
values.

The object is probably going to be set via some "persistCallback" property on
nsIWebBrowserPersist. This will initially point to the default callback object
which makes persist behave like it does now, but which can be replaced another
to to modify the behaviour. You can always keep a pointer to the original and
call that for methods you're not interested in overiding.

Comment 5

[Akkana Peck]

This seems like a complicated interface for a callback function, yet I don't
think it has all the smarts that will be required for link rewriting.

Link rewriting depends on knowledge of which nodes can contain URIs in which of
their attributes.  I think maybe the aURIType in nsIWebBrowserPersistCallback is
a start on that, but adding the full list would make the interface much more
complicated.

The editor has a class which encapsulates those rules: nsURIRefObject.  I
suggest that we move nsURIRefObject (and nsIURIRefObject) out of the editor so
that it will be available to embeddors, add the link rewriting code to it (which
should be fairly simple since it already knows how to get to the URIs contained
within a node), then we can simplify nsIWebBrowserPersistCallback's interface
and have it use a nsURIRefObject to do that part of the work.  nsIURIRefObject's
interface is open to redesign if it needs to be changed to make it easier for
the persist code to deal with -- so far its only client is a small amount of
editor code which is subject to change anyway.

I don't understand what makeOutputStream is; is the same callback class being
used to do both link rewriting and the actual writing of the resulting file?

Comment 6

[Adam Lock]

The reason for makeOutputStream is so the caller can supply their own streams
that the data is written into, for example:

1. To make access to a authenticated server more efficient. e.g. caller
authenticates just once and hands out streams on demand rather than relying on
the default that might try and authenticate once for each URI.
2. Custom streams. e.g. caller wants to upload to a database. It implements a
database outputstream object and hand out instances of these for the the persist
object to write to.

As for control over fixing up elements... There is already a fixupNode method at
the bottom. This is called during persistence, allowing you to clone and modify
the node as it is saved.

I can also add a method that is called as each element in the DOM is traversed.
I need to think about the form this would take.

Comment 7

[Adam Lock]

This bug is going to run and run. Perhaps we need to pinpoint the key features
needed and implement a 'lite' callback interface to begin with.

Comment 8

[Adam Lock]

Attachment: As far as I got last time...

Non function diff showing how far I got last time... Gives some insight into
how I was splitting out functionality into a callback so it could be overridden
by the caller.

Comment 9

[Adam Lock]

I'll mark this WONTFIX for the time being.