Freeze nsIWebBrowserPersist Please refer to http://www.mozilla.org/projects/embedding/EmbedInterfaceFreeze.html for the issues to be addressed, if any, for this interface. Please follow the guidelines outlined in "How to mark an interface as frozen?" section of the document above.

New web browser persist code is in but I'll probably have to wait to see if there are issues that force the interface to change further before I can continue with this. The interface already has pretty reasonable documentation but I haven't viewed after it's gone through Doxygen yet so I might fix any issues I find doing that for the time being.

change qa contact to Dharma.

Composer folks might needs some changes to this interface

I might like to see these added to the API (I'm investigating): outputEncodingFlags (get/set; defaults to 0) outputPreferredWidth (get/set; defaults to 72) contentType (get/set; defaults to "text/html") doReplace (get/set; defaults to true)

-> 0.9.7 This interface can't be frozen until any issues shaken out by bug 11632 and the editor group have been resolved.

Brade & Ben, do either of you have any comments to make about the current form of this interface? If not I'll give the documentation a makeover and submit this interface to be frozen.

Yes, we need some changes to this. I'd like Akkana to help with this if that's ok. See comment #4 for some items Composer will need.

Bug 110135 opened to cover editor issues

-> 0.9.8

With all due respect, this interface is not ready for freezing. In particular, its treatment of pages with postdata that the user wants read from cache is abysmal. It should either take a cache key or an instance of the upcoming nsIWebPageDescriptor that Rick is working on. The current implementation will silently and unconditionally repost when someone tries to saveURI with postdata from the cache, if I understand the way this stuff works correctly...

Results of API freeze meeting: * There is some issues about cache keys and cache flags. Essentially without a cache key, postdata may get resubmitted in the call to saveUri. * Webbrowserpersist does not accept cache keys, but it's not clear how the caller would actually provide them. Once this is clarified, there is no point extending the interface to support them. * Need to clarify wording in documentation to indicate the issue about cache flags and postdata. * Need to add comment reserving a block of flags (i.e. indicate that values in that range should not be used) * There was talk of turning the contenttype arg in saveDocument from a string into an AString, but there seems little point given the way in which it is used (in param, short length) * Making the persist object more modular (e.g. callbacks for determining whether to save a node, url type etc.) is work that can be saved for nsIWebBrowserPersist2. Otherwise I believe nsIWebBrowserPersist is ready for saving. Does anyone else have any issues to add, especially concerning the editor/upload side of things?

Oops, "Once this is clarified" -> "Until this is clarified"

> it's not clear how the caller would actually provide them. We could use nsIWebPageDescriptor... What about error handling? If the persistence object is asked to save data from some URL, should it just save it? What if it's an HTTP 404 response? Should callers be able to control what happens in such cases? What about an error response that does not include any savable content (eg FTP). Perhaps the error handling discussion should take place in bug 99639? It seems like many web progress listeners would want error notifications...

Also see bug 170722

Adding two more dependencies: bug 129332 ("Cancelling should clean up because the caller has no way to do it") and bug 174167 ("Error handling behavior needs to be defined").

Nominating API freeze work as topembed.

topembed+, freezing APIs is a Good Thing

Adam, this is a topembed+ bug that we think is important to get fixed. Removing the "future" target so that you can schedule it to an upcoming milestone (or argue the case for it not to be as important as EDT folks might think.)

These other bugs involve defining extra flags. Assuming the nsIWebBrowserPersist interface behaviour does not change when new flags are created, and short of ambiguity or such in the documentation, this interface could probably be frozen right now.

Um... There is also the question of error handling. Eg if I request that webbrowserpersist persist a page and it comes back with a 404 error, what should happen? This is probably also a matter of a flag, but there might need to be a way to pass in a window object so that error dialogs could be modal to that window object... It may also be that this need is already covered by the progressListener member variable; I've not dug into this much, to be truthful. Quite simply, here is what I need as a user of the interface -- I need to be able to say "persist this URI, and if you get a network error (HTTP 404, FTP error, whatever) put up an error dialog telling the user so". It may be enough to say "persist this URI, and if you get a network error (HTTP 404, FTP error, whatever) notify me so that I can put up an error dialog and maybe cancel the save operation".

What is wrong with the current behaviour? Doesn't the OnStateChange tell you there has been a network error, that you can find more info about by querying the nsIHttpChannel, e.g. for the response code? If this is a user land issue, it should be reassigned to the UI.

To repeat, "It may also be that this need is already covered by the progressListener member variable; I've not dug into this much, to be truthful." So I have no idea what the current behavior is. If it already does what it needs to do, so much the better. When does the OnStateChange fire? Does it fire _before_ any data is sent to a file? (I would assume that STATE_TRANSFERRING does, but STATE_STOP does not; the only one that really gives a useful status code per the comments is STATE_STOP.) This is all in reference to bug 160454, btw. So yes, there will be userland work done on this; I just want to make sure that the userland work that needs to happen is possible... :)

-> me, i want to push this along

note: I'd kinda like to extend saveURI/saveChannel to allow an nsIOutputStream as aFile argument. but that can be done w/o breaking compat, so it can be done even after freezing. (use case: tell a channel to resume at a specified offset; give wbp a stream with PR_APPEND set (or what that's called)) or would people think this should be done w/ a flag? in which case, it would block freezing this.

We also have the issue of saveURI taking a cache key... I'm not sure there are reasonable ways to get those, and I'm not sure we want to expose that detail on this api. We need to come up with a reasonable way of saving data from cache given a POST result or something...

bz: i think the "cache key" business is nicely abstract. i'm nearly ok with freezing nsICachingChannel, which would expose it to consumers.

But there is no way to get the channel for the "currently loaded page" as things stand... Further, it's not clear to me that just having a cache key guarantees you the ability to get data properly if POST data is also needed...

Also, note that I think it's _too_ abstract. There is no indication in the interface as to what constitutes a "valid" cache key....

(In reply to comment #27) > But there is no way to get the channel for the "currently loaded page" as things > stand... As I recall, a shistory entry works as cache key. > Further, it's not clear to me that just having a cache key guarantees > you the ability to get data properly if POST data is also needed... once setCacheToken gets implemented, it should...

> As I recall, a shistory entry works as cache key. How do I get that using frozen interfaces, given an nsIWebNavigation or nsIDOMWindow? Apart from that, that's not what's implemented. The persist code will QI that arg to nsIWebPageDescriptor, and if that succeeds will use impl details of the descriptor to get an nsISHEntry and than the cache key on that. If the "cache key" passed in here does not QI to nsIWebPageDescriptor, it'll be just used as-is. Also, we're talking about SetCacheKey, not SetCacheToken here. So it looks to me like properly doing a saveURI on a post response involves having not only the cache key, but also the post data stream and all that fun stuff... :(

We no longer freeze interfaces.