Freeze nsIWebNavigation 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.

->0.9.6

-> 0.9.7

referingURI attribute needs to be spelt properly before freezing...

mass reassign of various rpotts bugs to me

retargeting

re comment 3: http://bugzilla.mozilla.org/attachment.cgi?id=134486&action=edit Thanks, timeless, for the cc.

ok, some comments reading this interface: o) The comment describing it (line 25) needs to be directly above the interface declaration to allow doxygen to extract documentation for it (i.e. below the interface forward declarations) o) The documentation for the /* loadURI() & reload() specific flags */ should use some doxygen-like style o) loadURI documentation does not document the "headers" parameter. It would be nice if it described the format of this header stream... Also, wouldn't this be easier to use if the headers were passed as a string? And should the uri maybe be passed as an nsIURI rather than wstring? o) Would it be useful to add a load function that takes a channel rather than a uri? that scales better when more things need to be configured than the current loadURI signature allows o) LOAD_FLAGS_MASK could use a description that describes what it really is.

hmm, there's also bug 123960 o) For the @return parts, each line that documents a return value should be prefixed with @return, that produces nicer doxygen documentation (otherwise all those lines end up as a single line)

62 * there is back session history available for navigation. 68 * there is forward session history available for navigation punctuation... I think those @returns are probably supposed to be @throws 185 * pending Javascript timeouts. case :) 209 * The currently loaded URI or null. when would this be null? 214 * The referring URI. but not null? I'd like to object to this freeze: 222 attribute nsISHistory sessionHistory; nsISHistory was frozen with a dependency on the flash frozen nsISHistoryListener. nsISHistoryListener has incorrect method case and IMO we should not propogate it into the new world. I'd rather we not freeze with a dependency on nsISHistory and instead replace that interface.

adding bug 170439 (since it would mean a documentation change), although I personally think it should just be wontfix. especially as I think it should be nsIURI :)

This patch contains a first pass over cleaning up the comments in nsIWebNavigation.idl. I'm unsure how to explain the meaning of LOAD_FLAGS_IS_REFRESH and LOAD_FLAGS_IS_LINK. LOAD_FLAGS_IS_REFRESH appears to be unused and can probably be removed, but LOAD_FLAGS_IS_LINK is used (by mailnews at least), and it seems to have an affect on the aIsContentPreferred boolean passed to nsIURILoader::openURI. Hmm...

Comment on attachment 162639 [details] [diff] [review] v0 patch besides LOAD_FLAGS_IS_REFRESH and LOAD_FLAGS_IS_LINK, do you see anything else that needs to be clarified before freezing this interface?

Comment on attachment 162639 [details] [diff] [review] v0 patch sorry for the delay... LOAD_FLAGS_IS_REFRESH does seem to be used: /docshell/base/nsDocShell.h, line 123 -- LOAD_REFRESH = MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, nsIWebNavigation::LOAD_FLAGS_IS_REFRESH), and http://lxr.mozilla.org/seamonkey/search?string=LOAD_REFRESH + * Specifies that the load should have the semantics of a link click. hm, wanna mention what the semantics of a link click _are_? :) I am not sure that freezing an interface with XXX comments in it is a good thing. + * LOAD_FLAGS_BYPASS_HISTORY + * Specifies that history should not be updated. this means that no history entry should be created, right? what will it mean to call goBack() after this? and how does this relate to REPLACE_HISTORY? what are the semantics of LOAD_FLAGS_CHARSET_CHANGE from an implementor's point of view? ("always use the cache if we have an entry"?) + * @param aURI + * The URI string to load. so characters above U+007F will be sent as url-escaped UTF-8? (in the HTTP and FTP case) I wonder whether this should be documented... on the other hand, it's probably obvious, since there is no charset to pass to NewURI loadURI takes a string for the uri to load, but an nsIURI object for the referring uri... ah, consistency :/ This stream must + * contain a "\r\n" sequence at the beginning? at the end? both? + * @throw NS_BINDING_ABORTED + * Indicating that the user canceled the reload. this is for the postdata case, right? I think you should mention in this method's (reload's) description somewhere that there can be cases where the user will be asked to confirm the reload, in cases where the request isn't idempotent (this describes all the cases where we prompt, right?). What does stop return when there is nothing to stop? marking r- for now

oh, wanted to mention one more thing I kinda prefer having the documentation for each flag, instead of in one block. semantically, and from the point of automatic IDL parsers, the single block is the wrong thing. quoting my example from IRC, if someone were to write autocompletion code using this IDL file, which would give the flag description in a tooltip or something, the single block would do rather the wrong thing. (this file and nsIWebProgress and nsIWPL should use the same style for this...)

> LOAD_FLAGS_IS_REFRESH does seem to be used: > /docshell/base/nsDocShell.h, line 123 -- LOAD_REFRESH = > MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL, > nsIWebNavigation::LOAD_FLAGS_IS_REFRESH), right.. but how is that used? you see.. i think it might be used internally, but i don't see it being used with nsIWebNavigation. it's unclear why we need to expose that flag on nsIWebNavigation. it's also unclear what it really means. i don't fully grok nsDocShell :( > + * Specifies that the load should have the semantics of a link click. > > hm, wanna mention what the semantics of a link click _are_? :) i wish... again, i don't fully grok nsDocShell. need to figure out what it means. the XXX comments exist because these need to be sorted out before freezing. this patch is just v0 because i'm not done.. wanted to get the rest of it reviewed and moved into the trunk. > + * LOAD_FLAGS_BYPASS_HISTORY > + * Specifies that history should not be updated. > > this means that no history entry should be created, right? what will it mean to > call goBack() after this? and how does this relate to REPLACE_HISTORY? very good question. we should figure that out and document it. > what are the semantics of LOAD_FLAGS_CHARSET_CHANGE from an implementor's point > of view? ("always use the cache if we have an entry"?) i have no idea. another thing to try to figure out. > + * @param aURI > + * The URI string to load. > > so characters above U+007F will be sent as url-escaped UTF-8? (in the HTTP and > FTP case) > > I wonder whether this should be documented... on the other hand, it's probably > obvious, since there is no charset to pass to NewURI yeah, we should document that. > loadURI takes a string for the uri to load, but an nsIURI object for the > referring uri... ah, consistency :/ i know... i really hate this interface, but it has to be frozen or we'll burn way too many people. > This stream must > + * contain a "\r\n" sequence > > at the beginning? at the end? both? ok, i'll clarify that. > + * @throw NS_BINDING_ABORTED > + * Indicating that the user canceled the reload. > > this is for the postdata case, right? I think you should mention in this > method's (reload's) description somewhere that there can be cases where the > user will be asked to confirm the reload, in cases where the request isn't > idempotent (this describes all the cases where we prompt, right?). yeah... good call! > What does stop return when there is nothing to stop? hmm... not sure.

ah, bug 123960 also mentions some issues... I also noticed the "you" in the comment, which I didn't really like. that bug seems to have some more issues which this patch doesn't address yet (for example, on "inferred internally").

this patch leaves in these comments: const unsigned long LOAD_FLAGS_BYPASS_CACHE = 0x0100; // Bypass the cache const unsigned long LOAD_FLAGS_BYPASS_PROXY = 0x0200; // Bypass the proxy const unsigned long LOAD_FLAGS_CHARSET_CHANGE = 0x0400; // Reload because of charset change, they seem redundant with the comments you add

bz: any idea about LOAD_REFRESH / semantics of a link click / LOAD_FLAGS_BYPASS_HISTORY / charset change? (see comment 15)

> any idea about LOAD_REFRESH It looks like the nsIWebNavigation flag for this is unused, as is the nsDocShellLoadInfo flag... except by refresh timers in certain circumstances. So yes, this is totally an internal detail of docshell. We should not be exposing it. > semantics of a link click The only places I see LOAD_LINK being treated differently from LOAD_NORMAL is in this shrimp hack code and the call to the URILoader. I thought the former got removed as part of a security fix? Or did that only land on aviary branch? I wish I could recall who the patch author there was.... Anyway, the URILoader call just means we'll use IsPreferred() instead of CanHandleContent() when looking for a handler. Not like those are too clearly defined either... There are a few consumers of this LOAD_LINK thing (notably actual link clicks and some code in mailnews that uses the nsIDocShellLoadInfo way of getting at these load flags). > LOAD_FLAGS_BYPASS_HISTORY Yeah... this seems to skip both the global and session history. I'm not quite sure why we have it. > charset change I don't quite recall what this does either... I think at least figuring out which parts we don't know is a good start, so we can start trying to try to figure them out.... For the rest, the load flag system is way too complicated, with at least 4 sets of load flags about (docshell, webnavigation, loadinfo, and necko). We should really only have two -- webnavigation and necko, with docshell mapping between the two. Loadinfo in particular really needs to go away. :(

bz: thanks for the comments... > > any idea about LOAD_REFRESH ... > So yes, this is totally an internal detail of docshell. We should not be > exposing it. alright then, that's what i'll do :) > The only places I see LOAD_LINK being treated differently from LOAD_NORMAL is in > this shrimp hack code and the call to the URILoader. I thought the former got > removed as part of a security fix? Or did that only land on aviary branch? I > wish I could recall who the patch author there was.... dveditz did that patch, and yeah... it doesn't appear to have landed on the trunk yet. > There are a few consumers of this LOAD_LINK thing (notably actual link clicks > and some code in mailnews that uses the nsIDocShellLoadInfo way of getting at > these load flags). One place in mailnews does use LOAD_FLAGS_IS_LINK, and that's here: http://lxr.mozilla.org/seamonkey/source/mailnews/imap/src/nsImapMailFolder.cpp#3563 > > LOAD_FLAGS_BYPASS_HISTORY > > Yeah... this seems to skip both the global and session history. I'm not quite > sure why we have it. It looks like the ActiveX and OSX Cocoa embedding use this flag. > > charset change > > I don't quite recall what this does either... This is set whenever the user requests that the current page be viewed using a different character encoding. I'm not sure what the semantics of it actually are though. > For the rest, the load flag system is way too complicated, with at least 4 sets > of load flags about (docshell, webnavigation, loadinfo, and necko). We should > really only have two -- webnavigation and necko, with docshell mapping between > the two. Loadinfo in particular really needs to go away. :( Agreed, but it's going to take some work to be sure.

So, are there any chances to add a loadChannel(nsIChannel, flags) method to this interface? It would load a channel (which has not been asyncOpen'd yet); similar semantics as loadURI.

I like that idea a lot, except that I don't think we should change the vtable of nsIWebNavigation. Adding that method means that we'd need to change the UUID, and that will break existing embedders. If we don't change the UUID then it makes it very difficult to create an embedding that optionally uses the new method. I'd much rather design nsIWebNavigation2 with that method and other improvements.

Note that the api, as written, has at least one major issue -- the URI passed to loadURI() doesn't know the origin charset. Don't we decide what to send to the server based on that charset, if set? If so, the interface, as written, can't be used to do something as simple as implement "open in new tab" on middle-clicking a link... It's really only usable for "load this URI the user typed in the URL bar" type things.

On the contrary, it only requires that the URL be properly formatted before it is sent through this API. We can document that any non-ASCII characters are converted to UTF-8 before being %-escaped (IDN handled using the normal ACE encoding rules).

Ah, ok. Sounds like this is freezable with some documentation, then. I agree that any improvements should go into a different interface.

I went ahead and checked in some of the comment cleanups from this bug: Checking in nsIWebNavigation.idl; /cvsroot/mozilla/docshell/base/nsIWebNavigation.idl,v <-- nsIWebNavigation.idl new revision: 1.21; previous revision: 1.20 done I took biesi's advice and split up the mega load flags comment block. There's more work to be done to freeze this interface. The XXX comments in the IDL file serve as a rough guide.

Adding this here so I don't forget. Boris Zbarsky wrote: > Darin Fisher wrote: > > As for nsIWebNavigation::LoadURI, that method should be treated like a URL > > string entered into the URL bar. > > That makes sense, but then we should clearly document this (and in particular > clearly document that strings that are not trusted to have come from the page in > question or the user should never be passed to this method).

We no longer freeze interfaces.