Parsing routines for X-Force-TLS header....

Parsing routines for X-Force-TLS header.

R=darin
BUG=http://crbug.com/12190
TEST=ForceTLSStateTest.BogusHeaders, ForceTLSStateTest.ValidHeaders


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

[darin (slow to review), 2009-05-31 15:05:45]

Looks good, however...

http://codereview.chromium.org/118049/diff/6/8
File net/base/force_tls_state.cc (right):

http://codereview.chromium.org/118049/diff/6/8#newcode106
Line 106: static const std::string kIncludeSubdomains = "includesubdomains";
nit: might be nice to avoid a static reference to a heap object (the
std::string's internal buffer).  creating a new temporary heap object each time
this method is called would be better.

http://codereview.chromium.org/118049/diff/6/8#newcode124
Line 124: bool ForceTLSState::ParseHeader(const std::string& value,
nit: list methods in the same order as they appear in the .h file?

http://codereview.chromium.org/118049/diff/6/8#newcode130
Line 130: // Nomalize header for easier processing.
had you considered using StringTokenizer?  it is what we use in newer code that
parses HTTP headers.  it might be a better fit here.

http://codereview.chromium.org/118049/diff/6/10
File net/base/force_tls_state_unittest.cc (right):

http://codereview.chromium.org/118049/diff/6/10#newcode14
Line 14: int max_age = 42;
nit: const int kMaxAge = 42;

http://codereview.chromium.org/118049/diff/6/10#newcode77
Line 77: int max_age = 42;
ditto

[abarth-chromium, 2009-05-31 21:07:10]

http://codereview.chromium.org/118049/diff/6/8
File net/base/force_tls_state.cc (right):

http://codereview.chromium.org/118049/diff/6/8#newcode130
Line 130: // Nomalize header for easier processing.
On 2009/05/31 15:05:45, darin wrote:
> had you considered using StringTokenizer?

Nifty.  I've uploaded a new version based on StringTokenizer.

http://codereview.chromium.org/118049/diff/6/10
File net/base/force_tls_state_unittest.cc (right):

http://codereview.chromium.org/118049/diff/6/10#newcode14
Line 14: int max_age = 42;
On 2009/05/31 15:05:45, darin wrote:
> nit: const int kMaxAge = 42;

This can't be const because we supply it as the max_age out param below.

http://codereview.chromium.org/118049/diff/6/10#newcode77
Line 77: int max_age = 42;
On 2009/05/31 15:05:45, darin wrote:
> ditto

ditto

[darin (slow to review), 2009-06-01 14:02:48]

LGTM

Glad that StringTokenizer worked out for you.

[wtc, 2009-06-01 17:59:18]

I only read the .h file to understand the API.  A few nits
below.  Thanks!

http://codereview.chromium.org/118049/diff/16/18
File net/base/force_tls_state.cc (right):

http://codereview.chromium.org/118049/diff/16/18#newcode36
Line 36: // "X-Force-TLS" ":" "max-age" "=" delta-seconds *1INCLUDESUBDOMAINS
OK, so you have the header's definition here.  Would be nice
to move it (perhaps a version paraphrased in English) to the
header so that users of this class can understand this method
without reading the .cc file.

http://codereview.chromium.org/118049/diff/16/19
File net/base/force_tls_state.h (right):

http://codereview.chromium.org/118049/diff/16/19#newcode22
Line 22: // fatal, and refuses to load any mixed content.
Nit: refuses => refuse

http://codereview.chromium.org/118049/diff/16/19#newcode38
Line 38: // Returns |true| if value parses as a valid X-Force-TLS header value.
Nit: the first "value" should be quoted with ||.

http://codereview.chromium.org/118049/diff/16/19#newcode39
Line 39: // The value of max-age and and includeSubDomains are returned in
|max_age|
Nit: value => values

Could you include the definition of the X-Force-TLS header
in this comment, or cite a reference that defines the header?

[abarth-chromium, 2009-06-02 02:24:05]

http://codereview.chromium.org/118049/diff/16/18
File net/base/force_tls_state.cc (right):

http://codereview.chromium.org/118049/diff/16/18#newcode36
Line 36: // "X-Force-TLS" ":" "max-age" "=" delta-seconds *1INCLUDESUBDOMAINS
On 2009/06/01 17:59:19, wtc wrote:
> OK, so you have the header's definition here.  Would be nice
> to move it (perhaps a version paraphrased in English) to the
> header so that users of this class can understand this method
> without reading the .cc file.

Ok......  The only reason this method is public is to make it unit testable. 
I'd rather wait until this settles down a bit before putting a lot of effort
into documenting it.

http://codereview.chromium.org/118049/diff/16/19
File net/base/force_tls_state.h (right):

http://codereview.chromium.org/118049/diff/16/19#newcode22
Line 22: // fatal, and refuses to load any mixed content.
On 2009/06/01 17:59:19, wtc wrote:
> Nit: refuses => refuse

Fixed.

http://codereview.chromium.org/118049/diff/16/19#newcode38
Line 38: // Returns |true| if value parses as a valid X-Force-TLS header value.
On 2009/06/01 17:59:19, wtc wrote:
> Nit: the first "value" should be quoted with ||.

Fixed.

http://codereview.chromium.org/118049/diff/16/19#newcode39
Line 39: // The value of max-age and and includeSubDomains are returned in
|max_age|
On 2009/06/01 17:59:19, wtc wrote:
> Nit: value => values

Fixed.

> Could you include the definition of the X-Force-TLS header
> in this comment, or cite a reference that defines the header?

The spec is still under development.  This is an experimental implemenation
(behind a command line flag) that's meant to help the spec writers figure out
how the spec should work.  I'll add a link once there is something published to
refer to.