Closed Bug 157133 Opened 22 years ago Closed 22 years ago

HTTP Interfaces need to be frozen

Categories

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

x86
Windows 2000

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.3alpha

People

(Reporter: dougt, Assigned: darin.moz)

References

Details

(Keywords: arch, topembed+)

Attachments

(1 file, 7 obsolete files)

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.
Blocks: 70229
Blocks: 157137
darin
Assignee: dougt → darin
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.2beta
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 ;-)
Summary: HTTP Interfaces need to be frozen → [meta] HTTP Interfaces need to be frozen
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.
Severity: normal → major
Priority: P3 → P2
Whiteboard: [nsIHttpChannel, nsIHttpHeaderVisitor]
QA Contact: mdunn → depstein
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.
Depends on: 170648
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
Attachment #100601 - Flags: superreview+
Comment on attachment 100601 [details] [diff] [review]
move uploadStream to nsIUploadChannel

r=neeti
Attachment #100601 - Flags: review+
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
Attachment #100616 - Flags: superreview+
Whiteboard: [nsIHttpChannel, nsIHttpHeaderVisitor]
Comment on attachment 100616 [details] [diff] [review]
move nsIHttpHeaderVisitor into a separate IDL file.

r=neeti
Attachment #100616 - Flags: review+
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.
Attachment #100875 - Flags: superreview+
Comment on attachment 100875 [details] [diff] [review]
move contentEncodings and applyConversion to a new interface

r=neeti
Attachment #100875 - Flags: review+
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..."
Attachment #101145 - Flags: superreview+
Comment on attachment 101145 [details] [diff] [review]
Move documentURI to a new interface

r=rpotts@netscape.com
Attachment #101145 - Flags: review+
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.
Summary: [meta] HTTP Interfaces need to be frozen → HTTP Interfaces need to be frozen
-> 1.2final... most likely going to be marked frozen as is.
Target Milestone: mozilla1.2beta → mozilla1.2final
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.
Target Milestone: mozilla1.2final → mozilla1.3alpha
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.
Attachment #100601 - Attachment is obsolete: true
Attachment #100616 - Attachment is obsolete: true
Attachment #100875 - Attachment is obsolete: true
Attachment #101145 - Attachment is obsolete: true
Attachment #106388 - Flags: review?(dougt)
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!

Attachment #106388 - Attachment is obsolete: true
> 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.
Attachment #106689 - Flags: superreview?(bzbarsky)
Attachment #106689 - Flags: review?(dougt)
> 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.
Attachment #106689 - Attachment is obsolete: true
Attachment #106748 - Flags: superreview?(bzbarsky)
Attachment #106748 - Flags: review?(dougt)
> 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.
Attachment #106748 - Attachment is obsolete: true
Attachment #106758 - Flags: superreview?(bzbarsky)
Attachment #106758 - Flags: review?(dougt)
Attachment #106758 - Flags: superreview?(bzbarsky) → superreview+
Attachment #106689 - Flags: superreview?(bzbarsky) → superreview-
Attachment #106748 - Flags: superreview?(bzbarsky) → superreview-
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
Attachment #106758 - Flags: review?(dougt) → review+
patch checked in w/ dougt's comments addressed.

marking FIXED
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → 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.
Attachment #106388 - Flags: review?(dougt)
Attachment #106689 - Flags: review?(dougt)
Attachment #106748 - Flags: review?(dougt)
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.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.