201625 - expose request and response versions via new nsIHttpChannel2 interface

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Daniel Brooks [:db48x]]

I've got a patch, but since it creates a new file, I won't be able to make an
acutal diff.

Comment 1

[Daniel Brooks [:db48x]]

Attachment: patch

Comment 2

[Daniel Brooks [:db48x]]

Attachment: new file, nsIHttpChannel2.idl

it goes in netwerk/protocol/http/, of course.

Comment 3

[Daniel Brooks [:db48x]]

Comment on attachment 120183 [details]
new file, nsIHttpChannel2.idl

requesting r= on both attachments to this bug.

Comment 4

[Daniel Brooks [:db48x]]

Comment on attachment 120183 [details]
new file, nsIHttpChannel2.idl

might as well request sr while I'm at it.

Comment 5

[Darin Fisher]

Comment on attachment 120183 [details]
new file, nsIHttpChannel2.idl


>/** 
> * Adds to the frozen nsIHttpChannel
> */

remove the word frozen.  better to just say:

  "Extends nsIHttpChannel"

>  readonly attribute PRUint8 requestVersion;
>  readonly attribute PRUint8 responseVersion;

PRUint8?  huh?	if you are meaning to just pass the values straight from
nsHttpRequestHead/nsHttpResponseHead, then you will also need to define
the constants for HTTP/1.0 and HTTP/1.1, but constants wouldn't be very
flexible.  how about something more general like, minorVersion/majorVersion?

   readonly attribute unsigned long requestVersionMajor;
   readonly attribute unsigned long requestVersionMinor;

   readonly attribute unsigned long responseVersionMajor;
   readonly attribute unsigned long responseVersionMinor;

or you could even provide it as a string.  i'm not sure which would be
best.

also, NOTE: that the consumer of this interface will not be able to detect
HTTP/0.9 responses since we write the version as HTTP/1.0.  i'm not sure if
this impacts how you plan to use this interface, which i'm very curious
about.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120182 [details] [diff] [review]
patch

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp
>+NS_IMETHODIMP
>+nsHttpChannel::GetRequestVersion(PRUint8 *version)
>+{
>+  NS_ENSURE_ARG_POINTER(version);

This is a semi-religious issue, but imo that should just be
NS_PRECONDITION(version, "Null out param") (and maybe aVersion? I don't recall
what the arg-naming convention in that file is).

The only way you'd get a null |version| is if someone who has no clue about COM
writes some C++ code.

Same for the other method.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120183 [details]
new file, nsIHttpChannel2.idl

What darin said.  ;)

Comment 8

[Daniel Brooks [:db48x]]

It'll be used by livehttpheaders and page info. Both string and major/minor
would be equally useful for that purpose, so I'll pick... major/minor. Seems
like that'd be the most generally useful. On the other hand, a single byte
should provide enough space for future http versions, so it would certainly work
for me.

It is dissapointing to hear that a .9 version gets stored as a 1.0, but that's
life (and I don't suppose there are many of those around any more.)

bz: heh. I copied that from somewhere else in the file, it doesn't make any
difference to me. Anyway, I'll make those changes, just as soon as I figure out
why in the world I didn't leave my linux machine plugged in to the net instead
of my windows machine :P

Comment 9

[Daniel Brooks [:db48x]]

Attachment: better

Comment 10

[Daniel Brooks [:db48x]]

Attachment: the new file

Comment 11

[Daniel Brooks [:db48x]]

Comment on attachment 120278 [details]
the new file

requesting r= and sr= on the two new files

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120277 [details] [diff] [review]
better

Did the linebreaks in that file get fucked up?	This patch is almost certainly
not what you want to be doing; take a look at it... ;)

Comment 13

[Daniel Brooks [:db48x]]

erm, no, that's not what I want to do.

Comment 14

[Daniel Brooks [:db48x]]

Attachment: diff -uw :P

it's not like I changed any whitespace so that should work

Comment 15

[Darin Fisher]

may i ask why this is needed?  it just occured to me while working on proxy
failover that our request version might change as a result of failing over from
a proxy connection to a direct connection.  such would not be a big problem if
these methods were only accessed between OnStartRequest and OnStopRequest.  is
that the case?  is this for page-info or something?

Comment 16

[Daniel Brooks [:db48x]]

Yes, it's for page info. Can't you just store the new request version if it's
required when the failover occurs?

Comment 17

[Daniel Brooks [:db48x]]

so, darin, what's the word?

Comment 18

[Darin Fisher]

spoke to db48x over irc... seems best to utilize nsIHttpChannelInternal for
this... we can introduce nsIHttpChannel2 later if we need to freeze more stuff.
 "Internal" suffix comes from nsIPrefBranch and nsIPrefBranchInternal where only
the former is frozen and the latter is for stuff internal to mozilla.

Comment 19

[Daniel Brooks [:db48x]]

Attachment: patch

modifies nsIHttpChannelInternal instead of adding nsIHttpChannel2

Please double check that the data is returned ok, in my last patch I forgot
which language this was and returned the value instead of using the out param.
:P

My modified copy of livehttpheaders works ok, but double checking is good.

Comment 20

[Daniel Brooks [:db48x]]

Comment on attachment 124459 [details] [diff] [review]
patch

request r=

Comment 21

[Darin Fisher]

Comment on attachment 124459 [details] [diff] [review]
patch

>Index: netwerk/protocol/http/public/nsIHttpChannelInternal.idl

>+    readonly attribute unsigned long requestVersionMinor;
>+    readonly attribute unsigned long requestVersionMajor;

nit: list major before minor.


>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+nsHttpChannel::GetRequestVersionMajor(PRUint32 *version)
>+{
>+  NS_PRECONDITION(version, "Null out parameter");
>+  switch(mRequestHead.Version())
>+  {
>+    case NS_HTTP_VERSION_1_0:
>+    case NS_HTTP_VERSION_1_1:
>+      *version = 1;
>+      break;
>+    case NS_HTTP_VERSION_0_9:
>+    default:
>+      *version = 0;
>+      break;
>+  }
>+  return NS_OK;
>+}

 majorVersion = mRequestHead.Version() / 10;
 minorVersion = mRequestHead.Version() % 10;

less code :-)

also, would folks really ever want just the major
version number without the minor version as well?

how about functions like this instead:

  GetRequestVersion(PRUint32 *major, PRUint32 *minor);
  GetResponseVersion(PRUint32 *major, PRUint32 *minor);

i understand that these are less friendly to javascripters,
but it seems better to coalesce the operations like this.

Comment 22

[Daniel Brooks [:db48x]]

> majorVersion = mRequestHead.Version() / 10;
> minorVersion = mRequestHead.Version() % 10;
>
>less code :-)



>also, would folks really ever want just the major
>version number without the minor version as well?

maybe not, but you can tell what version you're working with by just examining
the minor version, since all three possible version numbers have a unique minor
part.

>how about functions like this instead:
>
>  GetRequestVersion(PRUint32 *major, PRUint32 *minor);
>  GetResponseVersion(PRUint32 *major, PRUint32 *minor);
>
>i understand that these are less friendly to javascripters,
>but it seems better to coalesce the operations like this.

perhaps.

I suppose I can do that. I can kill the NS_PRECONDITIONs and only assign to
pointers that are not null, so you can still get just one of the two if needed.
and there's really no need to return a status or anything, since it can't really
fail.

let me see if it compiles, then I'll attach it. It's so nice to have my dsl back. :)

Comment 23

[Daniel Brooks [:db48x]]

Attachment: patch

Comment 24

[Daniel Brooks [:db48x]]

Comment on attachment 127236 [details] [diff] [review]
patch

requesting r= and sr=

Comment 25

[Darin Fisher]

Comment on attachment 127236 [details] [diff] [review]
patch

>Index: netwerk/protocol/http/public/nsIHttpChannelInternal.idl

>+    void GetRequestVersion(out unsigned long major, out unsigned long minor);
>+    void GetResponseVersion(out unsigned long major, out unsigned long minor);

methods in IDL should be named using interCaps, so these methods should be:

    void getRequestVersion(out unsigned long major, out unsigned long minor);
    void getResponseVersion(out unsigned long major, out unsigned long minor);

r=darin with that change.

Comment 26

[Daniel Brooks [:db48x]]

the Get would still be capitolized in the implementation, because it's c++, right?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127236 [details] [diff] [review]
patch

> the Get would still be capitolized in the implementation, because it's c++, right?

Yes.

Please add null-checks before accessing mResponseHead (which can certainly be
null) and maybe mRequestHead, and sr=me

Comment 28

[Daniel Brooks [:db48x]]

Attachment: patch w/ those two last minute corrections

Comment 29

[Daniel Brooks [:db48x]]

Attachment: patch

at bz's request, initialize the out params and return something

Comment 30

[Daniel Brooks [:db48x]]

bz checked this in

Comment 31

[Daniel Brooks [:db48x]]

Comment on attachment 120278 [details]
the new file

clean up old request