Closed Bug 99642 Opened 23 years ago Closed 14 years ago

Freeze nsIWebBrowserPersist

Categories

(Core Graveyard :: Embedding: APIs, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX
Future

People

(Reporter: chak, Unassigned)

References

()

Details

(Keywords: topembed+, Whiteboard: edt_b3, edt_a3)

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.
QA Contact: mdunn → depstein
Blocks: 98417
No longer blocks: 98417
Depends on: 77909
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.
QA Contact: depstein → dsirnapalli
Composer folks might needs some changes to this interface
OS: Windows 2000 → All
Hardware: PC → All
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.
Depends on: 11632
Target Milestone: --- → mozilla0.9.7
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
Depends on: 110135
-> 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Blocks: 101001
Target Milestone: mozilla0.9.8 → Future
No longer blocks: 101001
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
Depends on: 170722
Blocks: 157137
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").
Depends on: 129332, 174167
Depends on: 177329
Nominating API freeze work as topembed.
Keywords: topembed
topembed+, freezing APIs is a Good Thing
Keywords: topembedtopembed+
Whiteboard: edt_b3, edt_a3
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.)
Target Milestone: Future → ---
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... :)
Target Milestone: --- → mozilla1.5alpha
-> me, i want to push this along
Assignee: adamlock → darin
Depends on: 266613
Target Milestone: mozilla1.5alpha → mozilla1.8beta
Status: NEW → ASSIGNED
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.
Blocks: 268520
Priority: -- → P3
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... :(
Target Milestone: mozilla1.8beta1 → mozilla1.9alpha
Depends on: 317377
Target Milestone: mozilla1.9alpha → Future
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: dsirnapalli → apis
We no longer freeze interfaces.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.