Delete net::GetHeaderParamValue

Delete net::GetHeaderParamValue

This function is a trap.  It's a quick-and-dirty parser that has many nutty
quirks.  There's only one caller left, and that callers should really be using
a Content-Type-specific parser anyway.

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

[asanka, 2012-01-27 16:18:58]

http://codereview.chromium.org/9296005/diff/1/net/http/http_util.cc
File net/http/http_util.cc (right):

http://codereview.chromium.org/9296005/diff/1/net/http/http_util.cc#newcode136
net/http/http_util.cc:136: static const char boundary_str[] = "boundary=";
Isn't *LWSP-char allowed around "="?

GetHeaderParamValue() allowed LWSP-char around symbols.  I wonder if we would
break things that expect this to be the case.

http://codereview.chromium.org/9296005/diff/1/net/http/http_util.h
File net/http/http_util.h (right):

http://codereview.chromium.org/9296005/diff/1/net/http/http_util.h#newcode46
net/http/http_util.h:46: // type and charset value, respectively.
Can you add a comment explaining the |boundary| parameter?

[abarth-chromium, 2012-01-27 18:49:27]

On 2012/01/27 16:18:58, asanka wrote:
> GetHeaderParamValue() allowed LWSP-char around symbols.  I wonder if we would
> break things that expect this to be the case.

Yeah, I wondered about that too.  Should we allow LWSP around "charset" too? 
This function is kind of ugly, which tempts me to clean it up a bit...

> Can you add a comment explaining the |boundary| parameter?

Will do.

[asanka, 2012-01-27 18:58:50]

On 2012/01/27 18:49:27, abarth wrote:
> On 2012/01/27 16:18:58, asanka wrote:
> > GetHeaderParamValue() allowed LWSP-char around symbols.  I wonder if we
would
> > break things that expect this to be the case.
> 
> Yeah, I wondered about that too.  Should we allow LWSP around "charset" too? 
> This function is kind of ugly, which tempts me to clean it up a bit...

Yeah. I think allowing LWSP would be the correct thing to do.

[abarth-chromium, 2012-01-27 22:12:05]

Fixenated.  PTAL.

[asanka, 2012-01-30 22:13:00]

LGTM.

http://codereview.chromium.org/9296005/diff/3004/net/http/http_util.cc
File net/http/http_util.cc (right):

http://codereview.chromium.org/9296005/diff/3004/net/http/http_util.cc#newcode95
net/http/http_util.cc:95: bool *had_charset,
Nit (from lint): bool* had_charset.

[abarth-chromium, 2012-01-30 22:15:52]

Thanks!

[commit-bot: I haz the power, 2012-01-30 22:20:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/9296005/10001

[commit-bot: I haz the power, 2012-01-30 22:20:06]

Presubmit check for 9296005-10001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/glue/weburlloader_impl.cc,webkit/glue/multipart_response_delegate.cc

Presubmit checks took 1.6s to calculate.

[abarth-chromium, 2012-01-30 22:39:13]

@darin: I can haz OWNERS review for the webkit/glue changes?  Thanks!

[darin (slow to review), 2012-01-30 22:53:38]

LGTM

http://codereview.chromium.org/9296005/diff/10001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/9296005/diff/10001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:569:
net::HttpUtil::ParseContentType(content_type, &mime_type, &charset,
did you consider making ParseContentType support NULL out-params, and then just
have the function skip some work?

[abarth-chromium, 2012-01-30 22:57:11]

> did you consider making ParseContentType support NULL out-params, and then
just
> have the function skip some work?

I did, but it's slightly complicated because the handling of the charset
paramater is coupled to the handling of the mime_type.

What I'll probably end up doing is creating a net::HttpContentType class similar
to the net::HttpContentDisposition class to make it easier for folks to use this
algorithm.

[commit-bot: I haz the power, 2012-01-30 22:57:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/9296005/10001

[commit-bot: I haz the power, 2012-01-31 01:00:52]

Change committed as 119790