Needed Functionality Accessing and motification of response headers and status Accessing and motification of request method changing the USER AGENT string programatically. using nsIHTTPChannel to determing if channel is http.

darin

would be good to add nsIHttpChannel::uploadStreamHasHeaders so consumers can easily determine if nsIHttpChannel::uploadStream contains headers. otherwise a consumer would have to read the stream to see if headers are contained... would be simple for the http channel to provide this since the value is already stored as a member var.

somewhere out there is a bug to move the useragent stuff out of this interface. I'll try to do that this month. I don't want http to freeze with it.

timeless: i believe nsIHttpProtocolHandler is under review because embedders are using the UA methods. i'm hopeful that we'll be able to avoid freezing this interface in favor of encouraging embedders to just get/set the pref values corresponding to the UA. anyways, please cc me on the bug having to do with moving the UA methods off of nsIHttpProtocolHandler when you find it. thx!

OK, after yesterday's API review meeting, we agreed not to freeze nsIHttpProtocolHandler. In fact, we also talked about moving the UA code to another interface, which would probably be implemented by another service. HTTP is not the right service for this. It only needs to know what UA string to send over the wire... that's it! As for the other interfaces, the plan is to freeze nsIHttpChannel and nsIHttpHeaderVisitor, but this will require some work. nsIHttpChannel needs the following revisions: 1- remove referrerType, make referrer a simple attribute 2- remove documentURI -> means that imagelib needs to use load groups and that cookies needs another way to determine the toplevel document URI. docshell or perhaps the docloader should be able to provide an API for this given the load group. 3- consider adding a LOAD_TOPLEVEL_DOCUMENT_URI load flag to nsIChannel although we may not need this. 4- move uploadStream to nsIUploadChannel 5- move isNoStoreResponse and isNoCacheResponse to nsICachingChannel 6- move contentEncodings and applyConversion to a new interface (e.g., nsIEncodedChannel) since FTP and other protocols could easily implement these methods. 7- move nsIHttpHeaderVisitor into a separate IDL file. so, this is definitely a meta bug ;-)

juan: please verify that i listed everything from yesterday's meeting, thx!

as for change #3, we do in fact need way of distinguishing link clicks from inline loads if we intend to eliminate the referrerType and the documentURI. so, that's where the proposal to add LOAD_TOPLEVEL_DOCUMENT_URI comes from. hmm... perhaps the option of suppressing the referrer on link click should be handled by the docshell instead. afterall, it is the only code that sets REFERRER_LINK_CLICK.

i think we should nix task #5. no-store and no-cache are very HTTP specific things. the interpretation of these flags is also very protocol specific. these are nothing more than shortcuts to the response headers (because several different response headers can mean the same thing as no-cache). since there aren't any other protocols that i can think of that have identical flags, i just don't see the point in moving these to nsICachingChannel. - no-cache corresponds to a zero freshness lifetime, which is exposed indirectly off of nsICachingChannel::cacheToken (QI to nsICacheEntryInfo). there is still a cache entry, but it must be validated before on each normal load. - no-store means the content should be considered sensitive, and should therefore not be stored persistently. docshell is the only current consumer of these methods because it must determine whether or not to save layout history state, and these just simplify the docshell code.

Comment on attachment 100601 [details] [diff] [review] move uploadStream to nsIUploadChannel >Index: docshell/base/nsWebShell.cpp > #include "nsIHttpChannel.h" // add this to the ick include list...we need it to QI for post data interface >+#include "nsIUploadChannel.h" the comment for nsIHttpChannel.h seems wrong... this file also plays with nsIHttpChannel::{requestMethod,referrer}. perhaps best to just kill the comment altogether :-/ >Index: netwerk/base/public/nsIStreamIO.idl these changes look like they're meant for a different bug. >Index: netwerk/base/public/nsIUploadChannel.idl >+// Notes: >+// Do we want to have expose contentType and contentLength? >+// What about a getter for the nsIFile? yeah, we need to sort these things out. otherwise, patch seems right on. thx! r/sr=darin

Comment on attachment 100601 [details] [diff] [review] move uploadStream to nsIUploadChannel r=neeti

Comment on attachment 100616 [details] [diff] [review] move nsIHttpHeaderVisitor into a separate IDL file. looks good, just don't forget to update the MANIFEST_IDL. sr=darin

Comment on attachment 100616 [details] [diff] [review] move nsIHttpHeaderVisitor into a separate IDL file. r=neeti

i think the behavior of SetRequestHeader/SetResponseHeader might need to change. currently, setting a header will append the value to the existing header (unless the header is on the do not append list). this is probably not desired in many cases. perhaps the best way to deal with this would be to either add an additional parameter to the setters, like aReplace or aMerge, _or_ perhaps it would be better to simply make the setters always replace. in mozilla, the merging feature is only used when processing <meta HTTP-EQUIV> tags and multipart/x-mixed-replace headers. perhaps MergeRequestHeader and MergeResponseHeader?

both those above patch have been checked into the trunk.

Comment on attachment 100875 [details] [diff] [review] move contentEncodings and applyConversion to a new interface sr=darin, but could you please add a comment to nsIEncodedChannel stating when these methods may be called. like, applyConversion can only be set before or during OnStartRequest (calling it during OnDataAvailable is an error). likewise, contentEncodings only exist during or after OnStartRequest.

Comment on attachment 100875 [details] [diff] [review] move contentEncodings and applyConversion to a new interface r=neeti

move contentEncodings and applyConversion to a new interface - checked in.

I just checked in an additional bustage fix for this checkin (in addition to the one dougt checked in) -- the new *_QUERY_INTERFACE11 macros in nsISupportsImpl.h weren't being called by the *_ISUPPORTS11 macros -- they were calling the *QUERY_INTERFACE10 macros.

I don't know where else this stuff is used in JS, but at least contentAreaUtils.js uses the contentEncodings attribute. I strongly suspect that this patch breaks file saving in some cases because the attribute will be undefined (since the channel is QIed to nsIHttpChannel there, not nsIEncodingChannel), thus we will never see any encodings.

LXR indicates that only contentAreaUtils.js is affected. i checked in the required fix.

Comment on attachment 101145 [details] [diff] [review] Move documentURI to a new interface sr=darin (thanks doug!) looks like there is a minor typo in the comments for nsIHttpChannelInternal. "if you are any feature..."

Comment on attachment 101145 [details] [diff] [review] Move documentURI to a new interface r=rpotts@netscape.com

Move documentURI to a new interface checked in.

thanks doug!! there are only small issues remaining: 1) are we happy with the behavior of SetRequestHeader and SetResponseHeader? currently, these functions are additive (i.e., they append to the existing header value). sometimes this makes sense, but often it seems contrary to the name of the function. we'll probably want to either add another param like |aReplace| to these functions or possibly add a new function named Merge/AddRequestHeader and Merge/AddResponseHeader. or, we could just leave it as is. 2) it would be good to move allowPipelining and redirectionLimit up into the request attributes section. this is very insignificant, but nice to have. otherwise, i think nsIHttpChannel and nsIHttpHeaderVisitor are done.

um, may i ask what nsIHttpHeaderVisitor is? pretend I've only read the idl file and can't figure it out. Also, does nsIHttpHeaderVisitor qualify as a new file that could be trilicensed?

>um, may i ask what nsIHttpHeaderVisitor is? pretend I've only read the idl file and can't figure it out. Maybe the method needs a comment. Darin, can you add something like that? >Also, does nsIHttpHeaderVisitor qualify as a new file that could be trilicensed? It is the same license as the nsIHttpChannel.idl, which may be wrong. Changing license because of a move like this probably isn't the greatest idea.

yes, i'll fill out the documentation for nsIHttpHeaderVisitor when i get the chance.

-> 1.2final... most likely going to be marked frozen as is.

pushing this out to moz 1.3 (since we have time) and because i really don't want to freeze nsIHttpChannel w/o giving careful consideration to the semantics of SetRequestHeader and SetResponseHeader.

this patch makes the following change: setRequestHeader(header,value) -> setRequestHeader(header,value,merge) if merge, then value is added to any existing values for the given header. if not merge, then old header values are overwritten. same change was made for setResponseHeader.

I'm not clear on what the "case-insensitive" part means for requestMethod. If I set it to "head" and then get it, will I get "head" back, or "HEAD"? If I get it, am I guaranteed that it will be a certain case? Perhaps better would be to clarify that case does not matter when setting but when getting it'll always come up uppercased (if that's true)? > + * sent as the referrer for a plaintext HTTP request). The implementation > + * is not required to throw an exception if the referrer URI is rejected. Does that mean that it _could_? Perhaps we need a @throws here if so, if we want to define the "reasonable" exceptions that could be thrown (eg SECURITY_ERR). > + * Get the value of a particular request header (case insensitive). Which part is case-insensitive -- the name or the value? Perhaps it would be better to move the "case insensitive" qualifier into the @param or @return (and please _do_ add an @return). > + * Set the value of a particular request header (case insensitive). Same. What is case-insensitive? Move to the appropriate @param section(s), please. All of this applies to the response headers too, of course. > + * new channel support nsIHttpChannel, then it will be assigned a value "new channel supports" > - rv = clone->SetUserPass(NS_LITERAL_CSTRING("")); > + rv = clone->SetUserPass(nsCString()); Why this change? What exactly does it mean to have multiple things in Content-Type or Content-length? Looks pretty good overall!

> Why this change? nevermind :) > What exactly does it mean to have multiple things in Content-Type or > Content-length? it doesn't mean anything... that's why these headers don't support merging.

> it doesn't mean anything... that's why these headers don't support merging. Oh, I see... It's just that CanAppendToHeader has the logic written in a confusing way (I would have done: return header != this && header != that && header != other; ). Why does Accept-Charset not support merging, out of curiousity?

hmm.. i'm not sure why Accept-Charset is in that list actually :( in fact, i don't think it should be there. in mozilla that header is pref controlled, but a call to SetRequestHeader("Accept-Charset", "foo", PR_TRUE) should succeed. seems wrong.. and yeah, you're suggested way of writing that function is definitely clearer.

ok, i fixed up the nsHttpHeaderArray::CanAppendToHeader() method as described.

> The request can be taylored "tailored" (sorry I missed this the first time) > + * @param aHeader > + * The case-insensitive value of the response header to query (e.g., > + * "Set-Cookie"). That should be "name", not "value". Same for setResponseHeader. I'm sorta wondering why isNo*Response are functions instead of readonly attributes (again, apologies for not noticing this the first time). I guess it makes the C++ a little clearer..... Looks great other than those nits. ;)

> I'm sorta wondering why isNo*Response are functions instead of readonly > attributes (again, apologies for not noticing this the first time). I guess it > makes the C++ a little clearer..... yes, it makes the C++ code read better.

fixed documentation errors.

Comment on attachment 106758 [details] [diff] [review] patch v1.3: revised per latest comments from bz do you want to say that setting the referrer after AsyncOpen does nothing? also, javadoc the visitRequestHeaders

patch checked in w/ dougt's comments addressed. marking FIXED

in summary, the following interfaces were marked frozen for mozilla 1.3: nsIHttpChannel nsIHttpHeaderVisitor the IID of nsIHttpChannel was rev'd in an effort to avoid problems w/ third-party software using the old interface (although this is by no means a complete solution to _that problem_). finally, the interfaces are now added to dist/sdk/necko instead of dist/include/necko.

Verified patch1.3 against Mozilla 1.3b Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20021216. Checked idl files. Looks good. Just one small nit in nsIHttpChannel.idl: params for get/setResponseHeader() need to be synced. Use 'header', 'value', etc, instead of 'aHeader' and 'aValue' in the comments to correspond with param inputs.