Don't convert HEAD requests to GETs on 303 redirects.

Don't convert HEAD requests to GETs on 303 redirects.

BUG=102130
TEST=URLRequestTestHTTP.Redirect*Tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108060

[mmenke, 2011-10-28 19:53:24]

http://codereview.chromium.org/8418024/diff/3001/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (left):

http://codereview.chromium.org/8418024/diff/3001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:354: self.EchoAllHandler,
Removed these because they're included in get_handlers.

[willchan no longer on Chromium, 2011-10-28 19:55:34]

A little swamped, redirecting to wtc.

[julian.reschke, 2011-10-29 07:34:22]

Code change looks good to me (assuming that the string comparison using != is
correct here).

[wtc, 2011-10-31 19:21:00]

Patch Set 6 LGTM.  I suggest some changes and have some
questions for testserver.py  below.  Thanks.

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (left):

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:359: self.EchoAllHandler,

Why did you remove self.EchoAllHandler from post_handlers
and put_handlers?  The reason is not obvious to me.

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:270: head_handlers):

Nit: list 'head_handlers' and 'do_HEAD' in alphabetical order
in this file.

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:911: data = self._ReplaceFileData(data,
query)

Just wanted to confirm that this statement should NOT be inside
the if statement above.

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:954: self.send_header('Content-Length',
len(data))

Just curious: are HEAD responses forbidden to have a
Content-Length header (of value 0)?

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1372: """This is the catch-all response
handler for head requests that aren't

Capitalize "HEAD".

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1376: self.send_header('Content-type',
'text/html')

Since there is no |contents|, it seems that we should omit
the Content-type response header, too.

[julian.reschke, 2011-10-31 19:46:49]

On 2011/10/31 19:21:00, wtc wrote:
> ...
>
http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
> net/tools/testserver/testserver.py:954: self.send_header('Content-Length',
> len(data))
> 
> Just curious: are HEAD responses forbidden to have a
> Content-Length header (of value 0)?
> ...

HEAD is a special-cased GET where the payload is omitted; so in this (and only
this) case Content-Length does not describe the actual response length, but the
length of what a GET would return. As such, Content-Length can be zero, non-zero
or missing.

[wtc, 2011-10-31 19:59:40]

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:954: self.send_header('Content-Length',
len(data))

julian.reschke: thank you for the reply.

mmenke: based on julian.reschke's reply, I believe your changes
to this function (FileHandler) for HEAD requests are
incorrect.

I believe that the only change this function needs for
HEAD requests is to omit the actual sending of file data
(I guess that's the self.wfile.write(data) statment on
line 958 below).

The new DefaultHeadResponseHandler function below has
the same problem.

[mmenke, 2011-10-31 20:46:40]

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (left):

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:359: self.EchoAllHandler,
On 2011/10/31 19:21:00, wtc wrote:
> 
> Why did you remove self.EchoAllHandler from post_handlers
> and put_handlers?  The reason is not obvious to me.

It's in get_handlers already, so there's no reason to add it again.  Had a
comment to this effect earlier, but uploaded multiple CLs since then, burying
it.

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:270: head_handlers):
On 2011/10/31 19:21:00, wtc wrote:
> 
> Nit: list 'head_handlers' and 'do_HEAD' in alphabetical order
> in this file.

Done.

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:911: data = self._ReplaceFileData(data,
query)
On 2011/10/31 19:21:00, wtc wrote:
> 
> Just wanted to confirm that this statement should NOT be inside
> the if statement above.

It didn't matter, either way.  Removed the if statement, as per your comment
below.

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:954: self.send_header('Content-Length',
len(data))
On 2011/10/31 19:59:41, wtc wrote:
> 
> julian.reschke: thank you for the reply.
> 
> mmenke: based on julian.reschke's reply, I believe your changes
> to this function (FileHandler) for HEAD requests are
> incorrect.
> 
> I believe that the only change this function needs for
> HEAD requests is to omit the actual sending of file data
> (I guess that's the self.wfile.write(data) statment on
> line 958 below).
> 
> The new DefaultHeadResponseHandler function below has
> the same problem.

Done.  You're right.  I misread the RFC.

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1372: """This is the catch-all response
handler for head requests that aren't
On 2011/10/31 19:21:00, wtc wrote:
> 
> Capitalize "HEAD".

Done.

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1376: self.send_header('Content-type',
'text/html')
On 2011/10/31 19:21:00, wtc wrote:
> 
> Since there is no |contents|, it seems that we should omit
> the Content-type response header, too.

Not done (As per spec - "The Content-Type entity-header field indicates the
media type of the entity-body sent to the recipient or, in the case of the HEAD
method, the media type that would have been sent had the request been a GET."). 
'Content-length' added instead.

[mmenke, 2011-10-31 20:49:25]

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1376: self.send_header('Content-type',
'text/html')
On 2011/10/31 20:46:40, Matt Menke wrote:
> On 2011/10/31 19:21:00, wtc wrote:
> > 
> > Since there is no |contents|, it seems that we should omit
> > the Content-type response header, too.
> 
> Not done (As per spec - "The Content-Type entity-header field indicates the
> media type of the entity-body sent to the recipient or, in the case of the
HEAD
> method, the media type that would have been sent had the request been a
GET."). 
> 'Content-length' added instead.

On second thought, you're right, with a 0 content-length.  Removed.

[mmenke, 2011-10-31 20:52:14]

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8418024/diff/7004/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1376: self.send_header('Content-type',
'text/html')
On 2011/10/31 20:49:25, Matt Menke wrote:
> On 2011/10/31 20:46:40, Matt Menke wrote:
> > On 2011/10/31 19:21:00, wtc wrote:
> > > 
> > > Since there is no |contents|, it seems that we should omit
> > > the Content-type response header, too.
> > 
> > Not done (As per spec - "The Content-Type entity-header field indicates the
> > media type of the entity-body sent to the recipient or, in the case of the
> HEAD
> > method, the media type that would have been sent had the request been a
> GET."). 
> > 'Content-length' added instead.
> 
> On second thought, you're right, with a 0 content-length.  Removed.

Or maybe not?  Honestly, I have no idea.  Suppose a 0-length file could
technically be called a text file.

[wtc, 2011-10-31 23:02:50]

Patch Set 7 LGTM.  I have one suggested change, but I'm not
sure if it'll work.

http://codereview.chromium.org/8418024/diff/7008/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8418024/diff/7008/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1363: self.send_header('Content-type',
'text/html')

Nit: capitalize "Type".

http://codereview.chromium.org/8418024/diff/7008/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1375: self.send_header('Content-Length', '0')

An alternative to sending a Content-Length of 0 is to
copy more code from DefaultResponseHandler() above:

    contents = "Default response given for path: " + self.path
    ...
    self.send_header("Content-Length", len(contents))

Essentially, make this function the same as DefaultResponseHandler
except that self.wfile.write(contents) is omitted.

I wonder if we can just use DefaultResponseHandler for
HEAD requests, and add an if around self.wfile.write(contents):

    if (self.command != 'HEAD'):
      self.wfile.write(contents)

[mmenke, 2011-10-31 23:29:12]

http://codereview.chromium.org/8418024/diff/7008/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8418024/diff/7008/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1363: self.send_header('Content-type',
'text/html')
On 2011/10/31 23:02:51, wtc wrote:
> 
> Nit: capitalize "Type".

Done.  Fixed a bunch of other cases of that, too.

http://codereview.chromium.org/8418024/diff/7008/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1375: self.send_header('Content-Length', '0')
On 2011/10/31 23:02:51, wtc wrote:
> 
> An alternative to sending a Content-Length of 0 is to
> copy more code from DefaultResponseHandler() above:
> 
>     contents = "Default response given for path: " + self.path
>     ...
>     self.send_header("Content-Length", len(contents))
> 
> Essentially, make this function the same as DefaultResponseHandler
> except that self.wfile.write(contents) is omitted.
> 
> I wonder if we can just use DefaultResponseHandler for
> HEAD requests, and add an if around self.wfile.write(contents):
> 
>     if (self.command != 'HEAD'):
>       self.wfile.write(contents)

The last suggestion sounds like a good idea to me.  Done.

[wtc, 2011-10-31 23:51:16]

Patch Set 8 LGTM!  Thanks. (Didn't mean to make you do all
that cleanup of 'Content-type'.)

[commit-bot: I haz the power, 2011-11-01 01:12:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/8418024/6017

[commit-bot: I haz the power, 2011-11-01 02:32:06]

Change committed as 108060