Show deprecation warnings for header values in XHR according to RFC 7230

Show deprecation warnings for header values in XHR
according to RFC 7230.

In this CL:
- We don't show messages for Fetch API.
- We don't actually update header checks.
Also, this CL is preparation for actually updating
header value checks to RFC 7230.

These issue is a merge from:
https://bugs.webkit.org/show_bug.cgi?id=128593.

BUG=455099
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201795

[shiva.jm, 2015-03-18 08:45:23]

shiva.jm@samsung.com changed reviewers:
+ a.cavalcanti@samsung.com, pdr@chromium.org, tkent@chromium.org

[shiva.jm, 2015-03-18 08:45:24]

Please have a look.

[tkent, 2015-03-18 08:53:46]

Is the new behavior compatible with IE and Firefox?

[shiva.jm, 2015-03-19 11:09:55]

On 2015/03/18 08:53:46, tkent wrote:
> Is the new behavior compatible with IE and Firefox?

New behavior is not compatible with IE and Firefox yet.

We can see comment in Firefox that "Header values MUST NOT contain line-breaks. 

RFC 2616 technically permits CTL characters, including CR and LF, 
in header values provided they are quoted.  However, this can lead to problems
if servers do not interpret quoted strings properly.  Disallowing CR and LF 
here seems reasonable and keeps things simple.  We also disallow a null byte."

[tkent, 2015-03-19 22:59:48]

tkent@chromium.org changed reviewers:
+ tyoshino@chromium.org, yhirano@chromium.org

[tkent, 2015-03-19 22:59:48]

Add XHR people.

[tkent, 2015-03-19 23:04:09]

https://codereview.chromium.org/1018903002/diff/1/Source/platform/network/HTT...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/1/Source/platform/network/HTT...
Source/platform/network/HTTPParsers.cpp:113: UChar c = value[0];
Looks this has an out-of-bound access issue

nit: c -> firstCharacter

https://codereview.chromium.org/1018903002/diff/1/Source/platform/network/HTT...
Source/platform/network/HTTPParsers.cpp:117: c = value[value.length() - 1];
c -> char lastCharacter

https://codereview.chromium.org/1018903002/diff/1/Source/platform/network/HTT...
Source/platform/network/HTTPParsers.cpp:124: return false;
wrong indentation

[yhirano, 2015-03-20 01:15:15]

yhirano@chromium.org changed reviewers:
+ hiroshige@chromium.org

[yhirano, 2015-03-20 01:15:15]

+hiroshige

[yhirano, 2015-03-20 01:21:04]

On 2015/03/20 01:15:15, yhirano wrote:
> +hiroshige

https://groups.google.com/a/chromium.org/forum/#!msg/net-dev/PBQ6Y_ai0jg/P1ED...
https://docs.google.com/document/d/1eH4aQNaYT6PDFugUXfAcgK4J1aU7m-iu9x_YVGsNe...

[hiroshige, 2015-03-20 05:24:38]

On 2015/03/20 01:21:04, yhirano wrote:
> On 2015/03/20 01:15:15, yhirano wrote:
> > +hiroshige
> 
>
https://groups.google.com/a/chromium.org/forum/#!msg/net-dev/PBQ6Y_ai0jg/P1ED...
>
https://docs.google.com/document/d/1eH4aQNaYT6PDFugUXfAcgK4J1aU7m-iu9x_YVGsNe...

[hiroshige, 2015-03-20 05:37:43]

(Sorry for sending a premature message.)

Chromium-side code also contains header value checking, e.g.
HttpUtil::IsValidHeaderValue().
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_util...
Do we have to update the header value checks in the Chromium side to keep
Chromium and Blink consistent?

[shiva.jm, 2015-03-20 06:23:52]

Please have a look, update the patch to fix review comments

https://codereview.chromium.org/1018903002/diff/1/Source/platform/network/HTT...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/1/Source/platform/network/HTT...
Source/platform/network/HTTPParsers.cpp:113: UChar c = value[0];
On 2015/03/19 23:04:08, tkent wrote:
> Looks this has an out-of-bound access issue
> 
> nit: c -> firstCharacter

These looks ok, if we just have value.length(), it will try to get character at
the end of string.

https://codereview.chromium.org/1018903002/diff/1/Source/platform/network/HTT...
Source/platform/network/HTTPParsers.cpp:124: return false;
On 2015/03/19 23:04:08, tkent wrote:
> wrong indentation

Done.

[shiva.jm, 2015-03-20 06:27:17]

On 2015/03/20 05:37:43, hiroshige wrote:
> (Sorry for sending a premature message.)
> 
> Chromium-side code also contains header value checking, e.g.
> HttpUtil::IsValidHeaderValue().
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_util...
> Do we have to update the header value checks in the Chromium side to keep
> Chromium and Blink consistent?

We need to updated HttpUtil::IsValidHeaderValue() also, shall we do it in
another patch, after these patch got merged?

[hiroshige, 2015-03-20 08:53:33]

> We need to updated HttpUtil::IsValidHeaderValue() also, shall we do it in
> another patch, after these patch got merged?
Then, this series of CLs will affect the header value checking in
1. Fetch API
2. XHRs
3. AssociateURLLoader (?)
4. and all Chromium
(1.-3. are affected by this Blink-side CL, and 4. would be affected if
HttpUtil::IsValidHeaderValue() is modified)

As for 1. Fetch API, I proposed a similar thing, as yhirano mentioned:
[1]
https://groups.google.com/a/chromium.org/forum/#!msg/net-dev/PBQ6Y_ai0jg/P1ED...
[2]
https://docs.google.com/document/d/1eH4aQNaYT6PDFugUXfAcgK4J1aU7m-iu9x_YVGsNe...
So this CL is fine with me in terms of Fetch API.

As for 2., 3., and 4., we might break existing code or tests (e.g. some
servers/applications that uses characters that are rejected by this and related
CLs).
Ryan Sleevi suggested me in [1]:
> Alternatively, you should consider evaluating the feasability of 
> putting these checks at the //net stack, so that we're uniformly 
> consistent in behaviour.
> The first step would be recording how often these violations happen.
This would be a good step before landing this and related Chromium-side CLs.
My activities are currently suspended, but we can follow this step and restart
the discussion.

[hiroshige, 2015-03-20 08:54:52]

https://codereview.chromium.org/1018903002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html (right):

https://codereview.chromium.org/1018903002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html:26:
try_value("�ｽ�", true);
This test fails on trybots (i.e. no exception is thrown. probably because the
value is percent-encoded?).

[shiva.jm, 2015-03-23 05:20:42]

On 2015/03/20 08:53:33, hiroshige wrote:
> > We need to updated HttpUtil::IsValidHeaderValue() also, shall we do it in
> > another patch, after these patch got merged?
> Then, this series of CLs will affect the header value checking in
> 1. Fetch API
> 2. XHRs
> 3. AssociateURLLoader (?)
> 4. and all Chromium
> (1.-3. are affected by this Blink-side CL, and 4. would be affected if
> HttpUtil::IsValidHeaderValue() is modified)
> 
> As for 1. Fetch API, I proposed a similar thing, as yhirano mentioned:
> [1]
>
https://groups.google.com/a/chromium.org/forum/#!msg/net-dev/PBQ6Y_ai0jg/P1ED...
> [2]
>
https://docs.google.com/document/d/1eH4aQNaYT6PDFugUXfAcgK4J1aU7m-iu9x_YVGsNe...
> So this CL is fine with me in terms of Fetch API.
> 
> As for 2., 3., and 4., we might break existing code or tests (e.g. some
> servers/applications that uses characters that are rejected by this and
related
> CLs).
> Ryan Sleevi suggested me in [1]:
> > Alternatively, you should consider evaluating the feasability of 
> > putting these checks at the //net stack, so that we're uniformly 
> > consistent in behaviour.
> > The first step would be recording how often these violations happen.
> This would be a good step before landing this and related Chromium-side CLs.
> My activities are currently suspended, but we can follow this step and restart
> the discussion.

Thanks for the detailed inputs, so we should first get the use cases or
violations happened and then we can discuss more on considering these patch
right?

[shiva.jm, 2015-03-23 05:21:24]

https://codereview.chromium.org/1018903002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html (right):

https://codereview.chromium.org/1018903002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html:26:
try_value("�ｽ�", true);
On 2015/03/20 08:54:52, hiroshige wrote:
> This test fails on trybots (i.e. no exception is thrown. probably because the
> value is percent-encoded?).

Yes right, in my local linux machine, these test passed.

[tyoshino (SeeGerritForStatus), 2015-03-24 11:43:12]

https://codereview.chromium.org/1018903002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html (right):

https://codereview.chromium.org/1018903002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html:18:
assert_throws("SyntaxError", function() { client.setRequestHeader("x-test",
value) }, ' given value ' + value+', ');
I know that this was just copied from W3C, but let's fix it.

The description argument will be put between a string "assert_throws:" and the
error detail such as "function "function () {}" did not throw". It should
describe what's being tested. E.g.

'setRequestHeader() method throws for value "' + value + '"'

[hiroshige, 2015-03-26 09:57:17]

> so we should first get the use cases or
> violations happened and then we can discuss more on considering these patch
Perhaps. I'll restart the discussion on net-dev, and I think it is good to
clarify [A] the range of update to RFC 7230 and [B] our
motivation/justification.

I think there will be two (or three) options:
1. We aim to update header value checks of the whole Chromium/Blink to RFC 7230,
and collect statistics of all HTTP header values in //net/ in Chromium.
2. We aim to update header value checks of only XHRs and Fetch API to RFC 7230,
and collect statistics of HTTP header values set in XHR's
setRequestHeader()/Fetch API's Headers-related interfaces in Blink. Statistics
of Fetch API might be unnecessary because it is a new spec/implementation.
(3. I originally aimed to update header value checks of only Fetch API to RFC
7230 without collecting statistics on Fetch API)

shiva.jm@, [A] in which range do you update the header value check and [B] what
is your motivation for that?
It would be helpful.

(I [A] wanted to update Fetch API (but no strong motivation of updating others)
[B] for simpler implementation and potentially better security, as listed in
https://docs.google.com/document/d/1eH4aQNaYT6PDFugUXfAcgK4J1aU7m-iu9x_YVGsNe...)

[shiva.jm, 2015-03-27 09:01:30]

On 2015/03/26 09:57:17, hiroshige wrote:
> > so we should first get the use cases or
> > violations happened and then we can discuss more on considering these patch
> Perhaps. I'll restart the discussion on net-dev, and I think it is good to
> clarify [A] the range of update to RFC 7230 and [B] our
> motivation/justification.
> 
> I think there will be two (or three) options:
> 1. We aim to update header value checks of the whole Chromium/Blink to RFC
7230,
> and collect statistics of all HTTP header values in //net/ in Chromium.
> 2. We aim to update header value checks of only XHRs and Fetch API to RFC
7230,
> and collect statistics of HTTP header values set in XHR's
> setRequestHeader()/Fetch API's Headers-related interfaces in Blink. Statistics
> of Fetch API might be unnecessary because it is a new spec/implementation.
> (3. I originally aimed to update header value checks of only Fetch API to RFC
> 7230 without collecting statistics on Fetch API)
> 
> shiva.jm@, [A] in which range do you update the header value check and [B]
what
> is your motivation for that?
> It would be helpful.
> 
> (I [A] wanted to update Fetch API (but no strong motivation of updating
others)
> [B] for simpler implementation and potentially better security, as listed in
>
https://docs.google.com/document/d/1eH4aQNaYT6PDFugUXfAcgK4J1aU7m-iu9x_YVGsNe...)


Thanks for the detailed inputs, I agree with your range for header value checks
and motivation, after going through web, Firefox and others, we can consider
option 3, i.e updating header value checks to only Fetch API, since its new
spec/implementation.
So if we want to make change only in Fetch, then we need to add new separate
function for header value check as per RFC 7230. old function
(isValidHTTPHeaderValue()) can be retained. is that OK?. 
Please share your inputs.

[shiva.jm, 2015-04-06 10:42:07]

Patchset #3 (id:40001) has been deleted

[shiva.jm, 2015-04-14 04:55:42]

Please have a look, updated the header value checks for only Fetch API to RFC
7230.

https://codereview.chromium.org/1018903002/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html (right):

https://codereview.chromium.org/1018903002/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/xmlhttprequest/set-bad-headervalue.html:18:
assert_throws("SyntaxError", function() { client.setRequestHeader("x-test",
value) }, ' given value ' + value+', ');
On 2015/03/24 11:43:12, tyoshino wrote:
> I know that this was just copied from W3C, but let's fix it.
> 
> The description argument will be put between a string "assert_throws:" and the
> error detail such as "function "function () {}" did not throw". It should
> describe what's being tested. E.g.
> 
> 'setRequestHeader() method throws for value "' + value + '"'

Done.

[shiva.jm, 2015-04-14 05:04:39]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-14 05:04:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018903002/60001

[commit-bot: I haz the power, 2015-04-14 05:05:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-14 05:05:03]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[yhirano, 2015-04-17 03:50:04]

By the way, the fetch spec still uses RFC2616.
Hiroshige, we once talked about confirming Anne's update plan. Have you
confirmed?

[hiroshige, 2015-04-22 16:47:11]

Sorry for my late response.

Anne (Fetch API spec writer) wrote in [1]:
> Hmm, I'm not sure that's desirable since that would mean XMLHttpRequest is
more powerful than fetch() for this feature. If anything fetch() needs to be
more powerful in the end.
[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27797

Also I noticed XHR spec e.g. setRequestHeader()
https://xhr.spec.whatwg.org/#dom-xmlhttprequest-setrequestheader
refers to Fetch API's spec's concept-header-value
https://fetch.spec.whatwg.org/#concept-header-value
that refers to RFC 2616.
So from the spec's perspective, I think changing the Fetch API's
concept-header-value definition to RFC 7230 will also affect XHR's
setRequestHeader and whatever refers to Fetch API's concept-header-value.
This seems to lead to updating whole Chromium //net stack, i.e. as I wrote in
comment #20:
> 1. We aim to update header value checks of the whole Chromium/Blink to RFC
7230,
> and collect statistics of all HTTP header values in //net in Chromium.

Does anyone have suggestions to proceed to Fetch API-only approach?
Or to proceed to update whole //net?

[shiva.jm, 2015-08-13 05:58:03]

On 2015/04/22 16:47:11, hiroshige wrote:
> Sorry for my late response.
> 
> Anne (Fetch API spec writer) wrote in [1]:
> > Hmm, I'm not sure that's desirable since that would mean XMLHttpRequest is
> more powerful than fetch() for this feature. If anything fetch() needs to be
> more powerful in the end.
> [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27797
> 
> Also I noticed XHR spec e.g. setRequestHeader()
> https://xhr.spec.whatwg.org/#dom-xmlhttprequest-setrequestheader
> refers to Fetch API's spec's concept-header-value
> https://fetch.spec.whatwg.org/#concept-header-value
> that refers to RFC 2616.
> So from the spec's perspective, I think changing the Fetch API's
> concept-header-value definition to RFC 7230 will also affect XHR's
> setRequestHeader and whatever refers to Fetch API's concept-header-value.
> This seems to lead to updating whole Chromium //net stack, i.e. as I wrote in
> comment #20:
> > 1. We aim to update header value checks of the whole Chromium/Blink to RFC
> 7230,
> > and collect statistics of all HTTP header values in //net in Chromium.
> 
> Does anyone have suggestions to proceed to Fetch API-only approach?
> Or to proceed to update whole //net?

Dear All,

     The Fetch spec is now updated with RFC:7230 as in links:
https://github.com/whatwg/fetch/commit/6c00fe28e7a361d2b7e0dda776ebccfaa4c439a5,

https://github.com/whatwg/fetch/issues/99 and
https://github.com/whatwg/fetch/issues/100. so should we consider these issue ?.
also now we need to Normalize header value in append() i.e ( To normalize a
value, remove any leading and trailing HTTP whitespace bytes from it). so we
need to remove leading and trailing HTTP whitespace as well now. shall we
consider these change or just remove any leading and trailing HTTP whitespace
from header value for Fetch now ?.

Thanks

[hiroshige, 2015-08-17 18:10:52]

On 2015/08/13 05:58:03, shiva.jm wrote:
> On 2015/04/22 16:47:11, hiroshige wrote:
> > Sorry for my late response.
> > 
> > Anne (Fetch API spec writer) wrote in [1]:
> > > Hmm, I'm not sure that's desirable since that would mean XMLHttpRequest is
> > more powerful than fetch() for this feature. If anything fetch() needs to be
> > more powerful in the end.
> > [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27797
> > 
> > Also I noticed XHR spec e.g. setRequestHeader()
> > https://xhr.spec.whatwg.org/#dom-xmlhttprequest-setrequestheader
> > refers to Fetch API's spec's concept-header-value
> > https://fetch.spec.whatwg.org/#concept-header-value
> > that refers to RFC 2616.
> > So from the spec's perspective, I think changing the Fetch API's
> > concept-header-value definition to RFC 7230 will also affect XHR's
> > setRequestHeader and whatever refers to Fetch API's concept-header-value.
> > This seems to lead to updating whole Chromium //net stack, i.e. as I wrote
in
> > comment #20:
> > > 1. We aim to update header value checks of the whole Chromium/Blink to RFC
> > 7230,
> > > and collect statistics of all HTTP header values in //net in Chromium.
> > 
> > Does anyone have suggestions to proceed to Fetch API-only approach?
> > Or to proceed to update whole //net?
> 
> Dear All,
> 
>      The Fetch spec is now updated with RFC:7230 as in links:
>
https://github.com/whatwg/fetch/commit/6c00fe28e7a361d2b7e0dda776ebccfaa4c439a5,
> 
> https://github.com/whatwg/fetch/issues/99 and
> https://github.com/whatwg/fetch/issues/100. so should we consider these issue
?.
> also now we need to Normalize header value in append() i.e ( To normalize a
> value, remove any leading and trailing HTTP whitespace bytes from it). so we
> need to remove leading and trailing HTTP whitespace as well now. shall we
> consider these change or just remove any leading and trailing HTTP whitespace
> from header value for Fetch now ?.
> 
> Thanks

Great!

The WebKit fix (https://bugs.webkit.org/show_bug.cgi?id=128593) modifies
isValidHTTPHeaderValue(), so affects only JavaScript interfaces:
XHR's setRequestHeader() and Fetch API's Headers.

So, in Blink, how about:
Step 1 (in this CL):
- Implement isValidHTTPHeaderValueForFetch(), and
- Show deprecation warnings and count occurrences of such deprecated header
  values by adding entries to UseCounter.h/cpp and
  calling UseCounter::countDeprecation() in
  XHR's setRequestHeader() and Fetch API's Headers.
  i.e. in all call sites of isValidHTTPHeaderValue(), add code like:
    if(isValidHTTPHeaderValueForFetch(...))
      UseCounter::countDeprecation(...);
  isValidHTTPHeaderValueForFetch() is used only for showing warnings, and
  accepting/rejecting values itself is done by (original)
  isValidHTTPHeaderValue().

Step 2 (in a separate CL; M-47/M-48? if the count is sufficiently small?):
- Normalize header values and actually reject deprecated header values in
  XHR's setRequestHeader() and Fetch API's Headers.

[shiva.jm, 2015-08-19 10:33:52]

On 2015/08/17 18:10:52, hiroshige (ooo zombie) wrote:
> On 2015/08/13 05:58:03, shiva.jm wrote:
> > On 2015/04/22 16:47:11, hiroshige wrote:
> > > Sorry for my late response.
> > > 
> > > Anne (Fetch API spec writer) wrote in [1]:
> > > > Hmm, I'm not sure that's desirable since that would mean XMLHttpRequest
is
> > > more powerful than fetch() for this feature. If anything fetch() needs to
be
> > > more powerful in the end.
> > > [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27797
> > > 
> > > Also I noticed XHR spec e.g. setRequestHeader()
> > > https://xhr.spec.whatwg.org/#dom-xmlhttprequest-setrequestheader
> > > refers to Fetch API's spec's concept-header-value
> > > https://fetch.spec.whatwg.org/#concept-header-value
> > > that refers to RFC 2616.
> > > So from the spec's perspective, I think changing the Fetch API's
> > > concept-header-value definition to RFC 7230 will also affect XHR's
> > > setRequestHeader and whatever refers to Fetch API's concept-header-value.
> > > This seems to lead to updating whole Chromium //net stack, i.e. as I wrote
> in
> > > comment #20:
> > > > 1. We aim to update header value checks of the whole Chromium/Blink to
RFC
> > > 7230,
> > > > and collect statistics of all HTTP header values in //net in Chromium.
> > > 
> > > Does anyone have suggestions to proceed to Fetch API-only approach?
> > > Or to proceed to update whole //net?
> > 
> > Dear All,
> > 
> >      The Fetch spec is now updated with RFC:7230 as in links:
> >
>
https://github.com/whatwg/fetch/commit/6c00fe28e7a361d2b7e0dda776ebccfaa4c439a5,
> > 
> > https://github.com/whatwg/fetch/issues/99 and
> > https://github.com/whatwg/fetch/issues/100. so should we consider these
issue
> ?.
> > also now we need to Normalize header value in append() i.e ( To normalize a
> > value, remove any leading and trailing HTTP whitespace bytes from it). so we
> > need to remove leading and trailing HTTP whitespace as well now. shall we
> > consider these change or just remove any leading and trailing HTTP
whitespace
> > from header value for Fetch now ?.
> > 
> > Thanks
> 
> Great!
> 
> The WebKit fix (https://bugs.webkit.org/show_bug.cgi?id=128593) modifies
> isValidHTTPHeaderValue(), so affects only JavaScript interfaces:
> XHR's setRequestHeader() and Fetch API's Headers.
> 
> So, in Blink, how about:
> Step 1 (in this CL):
> - Implement isValidHTTPHeaderValueForFetch(), and
> - Show deprecation warnings and count occurrences of such deprecated header
>   values by adding entries to UseCounter.h/cpp and
>   calling UseCounter::countDeprecation() in
>   XHR's setRequestHeader() and Fetch API's Headers.
>   i.e. in all call sites of isValidHTTPHeaderValue(), add code like:
>     if(isValidHTTPHeaderValueForFetch(...))
>       UseCounter::countDeprecation(...);
>   isValidHTTPHeaderValueForFetch() is used only for showing warnings, and
>   accepting/rejecting values itself is done by (original)
>   isValidHTTPHeaderValue().
> 
> Step 2 (in a separate CL; M-47/M-48? if the count is sufficiently small?):
> - Normalize header values and actually reject deprecated header values in
>   XHR's setRequestHeader() and Fetch API's Headers.

Ok, actually we have 2 parts now:
1) Normalize header values first (i.e To normalize a value, remove any 
leading and trailing HTTP whitespace bytes from header value), these has to be
done
before, we call isValidHeaderValue() api), trying in patch
https://codereview.chromium.org/1288263003,
have some failures, still investigating.

2) Implement isValidHTTPHeaderValueForFetch() as in these CL, as per your above
2 steps inputs.

So in your Step 2, if the count is sufficiently small, means we should remove
old isValidHTTPHeaderValue()
and use new isValidHTTPHeaderValueForFetch() ?.

[hiroshige, 2015-08-19 12:43:02]

> So in your Step 2, if the count is sufficiently small, means we should remove
> old isValidHTTPHeaderValue()
> and use new isValidHTTPHeaderValueForFetch() ?.
Yes.

> 1) Normalize header values first
I think this should be done in Step 2 (i.e. after collecting counts), because
normalization itself might break users' programs (impact will be smaller
though).

If the non-normalized value is a valid field-content (i.e. passes
isValidHTTPHeaderValueForFetch()), neither normalization nor upgrading the value
check to RFC 7230 affects that value, so we can safely do both.
If isValidHTTPHeaderValueForFetch() fails (and we'll show deprecation messages),
normalization and/or upgrading to RFC 7230 affects the program, and I hope and I
want to confirm this case is rare.

[pdr., 2015-08-19 17:04:22]

pdr@chromium.org changed reviewers:
- pdr@chromium.org

[shiva.jm, 2015-08-20 16:56:41]

added deprecation warnings and counter, pls have a look.
Should we add CONSOLE Warning in all XHR and Fetch tests?.

[shiva.jm, 2015-08-21 05:51:56]

On 2015/08/20 16:56:41, shiva.jm wrote:
> added deprecation warnings and counter, pls have a look.
> Should we add CONSOLE Warning in all XHR and Fetch tests?.

Using UseCounter::countDeprecation() in Fetch is failing some tests,
from FetchHeaderList.cpp and AssociatedURLLoader.cpp, getting context/
document is not available, so need to use UseCounter::deprecationMessage().

[shiva.jm, 2015-08-21 11:54:28]

pls have a look, will updated the failing tests in patch7 soon.

[shiva.jm, 2015-08-24 03:04:13]

updated with tests, pls have a look.

[hiroshige, 2015-08-24 06:07:48]

Sorry for delay, I'm currently sick and unavailable for a few more days. I'll
respond when I'm back.

[shiva.jm, 2015-08-26 10:53:59]

shiva.jm@samsung.com changed reviewers:
- a.cavalcanti@samsung.com

[shiva.jm, 2015-08-26 10:53:59]

updated the tests, but test
'http/tests/inspector/network/network-disable-cache-cors.html' behaves strange
in mac, windows and linux builds, even if make it correct, but fails again.

[hiroshige, 2015-08-27 11:38:08]

1.
As mentioned in individual comments, I think we want to show warnings if
 (!isValidHTTPHeaderValueForFetch(value) && isValidHTTPHeaderValue(value)), not
if (isValidHTTPHeaderValueForFetch(value)).
After fixing this, I expect much less Layout Test expectation changes.

2. CL description:
How about: "Show deprecation warnings Update header values check according to
RFC 7230 in XHR and Fetch API"?

I reused existing crbug Issue 455099 to tracking related issues. Could you refer
BUG=455099?

3. How about adding a test in
LayoutTests/http/tests/fetch/chromium/error-messages.html to confirm deprecation
message is actually displayed?
(A single test would be sufficient; we can add more tests to
INVALID_HEADER_VALUES when we actually reject deprecated header values later)

https://codereview.chromium.org/1018903002/diff/260001/Source/core/frame/UseC...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/core/frame/UseC...
Source/core/frame/UseCounter.cpp:975: return "isValidHTTPHeaderValue() will be
deprecated and will be removed in future.";
How about "Header values not matching RFC 7230 will be deprecated (see:
https://www.chromestatus.com/feature/6457425448140800)"?
This message is for XHR/Fetch API users so should be a warning about header
values.
Also I created the chromestatus entry.

https://codereview.chromium.org/1018903002/diff/260001/Source/core/frame/UseC...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/core/frame/UseC...
Source/core/frame/UseCounter.h:835: FetchHeaderValue = 940,
FetchHeaderValue looks too general.
How about something like "HeaderValueNotMatchingRFC7230"?

https://codereview.chromium.org/1018903002/diff/260001/Source/core/xmlhttpreq...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/core/xmlhttpreq...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:1182: if
(isValidHTTPHeaderValueForFetch(value))
How about replacing this with "if (!isValidHTTPHeaderValueForFetch(value))" and
placing this after the "if (!isValidHTTPHeaderValue(value)) {...}" block?

i.e. we want to warn header values that pass
isValidHTTPHeaderValue() but don't pass
isValidHTTPHeaderValueForFetch().

https://codereview.chromium.org/1018903002/diff/260001/Source/modules/fetch/F...
File Source/modules/fetch/FetchHeaderList.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchHeaderList.cpp:159: if
(isValidHTTPHeaderValueForFetch(value))
This should be "if (!isValidHTTPHeaderValueForFetch(value) &&
isValidHTTPHeaderValue(value))".

https://codereview.chromium.org/1018903002/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchHeaderList.cpp:160:
UseCounter::deprecationMessage(UseCounter::FetchHeaderValue);
Calling UseCounter::deprecationMessage() does nothing here (no warning, no
counter update).

An alternative is to call UseCounter::countDeprecation(ExecutionContext*, ...)
here and to add ExecutionContext* as a parameter of isValidHeaderValue().
This adds ExecutionContext* to many caller methods though.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:110: // See RFC 2616, Section 4.2.
Removing this comment might be better, because isValidHTTPHeaderValue()'s
implementation does not match with the RFC, as mentioned in the FIXME in
L113-114.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:119: // See RFC 7230, Section 3.2.3.
Please add a comment that this function checks whether |value| matches
field-content in RFC 7230.
Should we refer to Section 3.2 where |field-content| is defined?

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:121: {
Please add:
if (value.isEmpty())
    return false;
to return false for the empty string (currently returns true?) and
to avoid accessing value[0] and value[-1] for the empty string.
(Accessing value[0] and value[-1] will be blocked by WTF::String impl but such
code is quite hard to verify.)

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:122: UChar c = value[0];
c -> firstCharacter, as tkent@ commented.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:126: c = value[value.length() - 1];
c -> UChar lastCharacter, as tkent@ commented.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:131: c = value[i];
c -> UChar c.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:140: bool isValidHTTPToken(const String&
value)
Please retain the name of |characters| instead of changing to |value| to avoid
changing unrelated code and make diff smaller.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
File Source/platform/network/HTTPParsers.h (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.h:95: PLATFORM_EXPORT bool
isValidHTTPHeaderValueForFetch(const String&);
How about isValidHTTPFieldContentRFC7230()?
(This is used not only in Fetch API but also in XHR, and this corresponds
strictly to field-content in RFC 7230)

https://codereview.chromium.org/1018903002/diff/260001/Source/web/AssociatedU...
File Source/web/AssociatedURLLoader.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/web/AssociatedU...
Source/web/AssociatedURLLoader.cpp:75:
UseCounter::deprecationMessage(UseCounter::FetchHeaderValue);
I think deprecation message here is not needed (or at least out of scope of this
CL).
Although isValidHTTPHeaderValue() is called, this is not directly coupled with
XHR or Fetch API.

[shiva.jm, 2015-09-01 08:26:52]

Updated tests and code, pls have a look.
but could not call UseCounter::countDeprecation from
FetchHeaderList::isValidHeaderValue(), since from Headers class only Exception
state can be passed, we cannot get Execution context. Also tried to use as done
in CSSStyleSheet.cpp, to use
callingExecutionContext(V8PerIsolateData::mainThreadIsolate()),but many other
tests are failing.

https://codereview.chromium.org/1018903002/diff/260001/Source/core/frame/UseC...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/core/frame/UseC...
Source/core/frame/UseCounter.cpp:975: return "isValidHTTPHeaderValue() will be
deprecated and will be removed in future.";
On 2015/08/27 11:38:07, hiroshige (slow) wrote:
> How about "Header values not matching RFC 7230 will be deprecated (see:
> https://www.chromestatus.com/feature/6457425448140800)%22
> This message is for XHR/Fetch API users so should be a warning about header
> values.
> Also I created the chromestatus entry.

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/core/frame/UseC...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/core/frame/UseC...
Source/core/frame/UseCounter.h:835: FetchHeaderValue = 940,
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> FetchHeaderValue looks too general.
> How about something like "HeaderValueNotMatchingRFC7230"?

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/core/xmlhttpreq...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/core/xmlhttpreq...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:1182: if
(isValidHTTPHeaderValueForFetch(value))
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> How about replacing this with "if (!isValidHTTPHeaderValueForFetch(value))"
and
> placing this after the "if (!isValidHTTPHeaderValue(value)) {...}" block?
> 
> i.e. we want to warn header values that pass
> isValidHTTPHeaderValue() but don't pass
> isValidHTTPHeaderValueForFetch().

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/modules/fetch/F...
File Source/modules/fetch/FetchHeaderList.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchHeaderList.cpp:159: if
(isValidHTTPHeaderValueForFetch(value))
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> This should be "if (!isValidHTTPHeaderValueForFetch(value) &&
> isValidHTTPHeaderValue(value))".

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/modules/fetch/F...
Source/modules/fetch/FetchHeaderList.cpp:159: if
(isValidHTTPHeaderValueForFetch(value))
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> This should be "if (!isValidHTTPHeaderValueForFetch(value) &&
> isValidHTTPHeaderValue(value))".

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:110: // See RFC 2616, Section 4.2.
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> Removing this comment might be better, because isValidHTTPHeaderValue()'s
> implementation does not match with the RFC, as mentioned in the FIXME in
> L113-114.

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:119: // See RFC 7230, Section 3.2.3.
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> Please add a comment that this function checks whether |value| matches
> field-content in RFC 7230.
> Should we refer to Section 3.2 where |field-content| is defined?

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:121: {
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> Please add:
> if (value.isEmpty())
>     return false;
> to return false for the empty string (currently returns true?) and
> to avoid accessing value[0] and value[-1] for the empty string.
> (Accessing value[0] and value[-1] will be blocked by WTF::String impl but such
> code is quite hard to verify.)

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:122: UChar c = value[0];
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> c -> firstCharacter, as tkent@ commented.

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:126: c = value[value.length() - 1];
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> c -> UChar lastCharacter, as tkent@ commented.

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:131: c = value[i];
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> c -> UChar c.

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:140: bool isValidHTTPToken(const String&
value)
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> Please retain the name of |characters| instead of changing to |value| to avoid
> changing unrelated code and make diff smaller.

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
File Source/platform/network/HTTPParsers.h (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/platform/networ...
Source/platform/network/HTTPParsers.h:95: PLATFORM_EXPORT bool
isValidHTTPHeaderValueForFetch(const String&);
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> How about isValidHTTPFieldContentRFC7230()?
> (This is used not only in Fetch API but also in XHR, and this corresponds
> strictly to field-content in RFC 7230)

Done.

https://codereview.chromium.org/1018903002/diff/260001/Source/web/AssociatedU...
File Source/web/AssociatedURLLoader.cpp (right):

https://codereview.chromium.org/1018903002/diff/260001/Source/web/AssociatedU...
Source/web/AssociatedURLLoader.cpp:75:
UseCounter::deprecationMessage(UseCounter::FetchHeaderValue);
On 2015/08/27 11:38:08, hiroshige (slow) wrote:
> I think deprecation message here is not needed (or at least out of scope of
this
> CL).
> Although isValidHTTPHeaderValue() is called, this is not directly coupled with
> XHR or Fetch API.

Done.

[tkent, 2015-09-01 08:27:29]

tkent@chromium.org changed reviewers:
- tkent@chromium.org

[hiroshige, 2015-09-01 11:14:55]

> but could not call UseCounter::countDeprecation from
> FetchHeaderList::isValidHeaderValue(), since from Headers class only Exception
> state can be passed, we cannot get Execution context. Also tried to use as
done
> in CSSStyleSheet.cpp, to use
> callingExecutionContext(V8PerIsolateData::mainThreadIsolate()),but many other
> tests are failing.
Hmm. Then, we can omit deprecation messages for Fetch API, because:
- Fetch API is relative new API so the impact of updating the header value check
would be smaller.
- Implementation is somehow costly; we have to supply ExecutionContext* from the
callers, and callers of callers ... of FetchHeaderList::isValidHeaderValue().
- Deprecation warnings by UseCounter are not counted in UMA in serviceworker,
but serviceworker is the main location where Fetch API is used.
So we can revert the changes in:
- LayoutTests/http/tests/fetch/chromium/error-messages.html
- LayoutTests/http/tests/fetch/chromium/error-messages-expected.txt
- Source/modules/fetch/FetchHeaderList.cpp

[hiroshige, 2015-09-01 11:21:18]

https://codereview.chromium.org/1018903002/diff/400001/Source/core/xmlhttpreq...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1018903002/diff/400001/Source/core/xmlhttpreq...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:1186: // Show deprecation warnings
and count occurrences of such deprecated header values
nit: append '.' at the end of the statement.

https://codereview.chromium.org/1018903002/diff/400001/Source/core/xmlhttpreq...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:1187: if
(!isValidHTTPFieldContentRFC7230(value))
After I saw the diff of xmlhttprequest-setrequestheader-no-value-expected.txt, I
found the empty string is expected to be a valid header value, and I filed
https://github.com/whatwg/fetch/issues/115.
So I expect this will be:
if (!value.isEmpty() && !isValidHTTPFieldContentRFC7230(value))
If this change is done, I expect the warning diff in
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-setrequestheader-no-value-expected.txt
can be removed.

https://codereview.chromium.org/1018903002/diff/400001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/400001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:126: UChar firstCharacter= value[0];
A space before '='.

https://codereview.chromium.org/1018903002/diff/400001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:127: if (firstCharacter== ' ' ||
firstCharacter== '\t')
A Space before each of '=='s.

[shiva.jm, 2015-09-01 14:29:10]

shiva.jm@samsung.com changed reviewers:
+ vollick@chromium.org

[shiva.jm, 2015-09-01 14:29:11]

updated tests and code, pls have a look.

https://codereview.chromium.org/1018903002/diff/400001/Source/core/xmlhttpreq...
File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/1018903002/diff/400001/Source/core/xmlhttpreq...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:1186: // Show deprecation warnings
and count occurrences of such deprecated header values
On 2015/09/01 11:21:18, hiroshige (slow) wrote:
> nit: append '.' at the end of the statement.

Done.

https://codereview.chromium.org/1018903002/diff/400001/Source/core/xmlhttpreq...
Source/core/xmlhttprequest/XMLHttpRequest.cpp:1187: if
(!isValidHTTPFieldContentRFC7230(value))
On 2015/09/01 11:21:18, hiroshige (slow) wrote:
> After I saw the diff of xmlhttprequest-setrequestheader-no-value-expected.txt,
I
> found the empty string is expected to be a valid header value, and I filed
> https://github.com/whatwg/fetch/issues/115.
> So I expect this will be:
> if (!value.isEmpty() && !isValidHTTPFieldContentRFC7230(value))
> If this change is done, I expect the warning diff in
>
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-setrequestheader-no-value-expected.txt
> can be removed.

Done.

https://codereview.chromium.org/1018903002/diff/400001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/400001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:126: UChar firstCharacter= value[0];
On 2015/09/01 11:21:18, hiroshige (slow) wrote:
> A space before '='.

Done.

https://codereview.chromium.org/1018903002/diff/400001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:127: if (firstCharacter== ' ' ||
firstCharacter== '\t')
On 2015/09/01 11:21:18, hiroshige (slow) wrote:
> A Space before each of '=='s.

Done.

[hiroshige, 2015-09-01 15:07:42]

(non-owner) lgtm with comments/nits.

CL Description:
> Show deprecation warnings and Update header values check according to RFC 7230
in XHR and Fetch API
How about "Show deprecation warnings for header values in XHR according to RFC
7230." or so?
In this CL,
- We don't show messages for Fetch API.
- We don't actually update header checks.
Also, mentioning that "This CL is preparation for actually updating header value
checks to RFC 7230" might be good for providing the context.

Thanks for taking this issue!

https://codereview.chromium.org/1018903002/diff/430001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/430001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:118: // See RFC 7230, Section 3.2.3.
nit: Section 3.2? (3.2.3 refers to Whitespace)

https://codereview.chromium.org/1018903002/diff/430001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:119: // checks whether |value| matches
field-content in RFC 7230.
nit: s/checks/Checks/

[shiva.jm, 2015-09-01 15:18:47]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-01 15:18:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018903002/430001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1018903002/430001

[commit-bot: I haz the power, 2015-09-01 16:30:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-01 16:30:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shiva.jm, 2015-09-02 03:47:44]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-02 03:47:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018903002/450001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1018903002/450001

[shiva.jm, 2015-09-02 03:49:39]

shiva.jm@samsung.com changed reviewers:
+ dglazkov@chromium.org

[shiva.jm, 2015-09-02 03:49:41]

updated patch tests and description, pls have a look.

https://codereview.chromium.org/1018903002/diff/430001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/430001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:118: // See RFC 7230, Section 3.2.3.
On 2015/09/01 15:07:42, hiroshige (slow) wrote:
> nit: Section 3.2? (3.2.3 refers to Whitespace)

Done.

https://codereview.chromium.org/1018903002/diff/430001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:119: // checks whether |value| matches
field-content in RFC 7230.
On 2015/09/01 15:07:42, hiroshige (slow) wrote:
> nit: s/checks/Checks/

Done.

[commit-bot: I haz the power, 2015-09-02 04:55:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-02 04:55:02]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-09-02 05:46:27]

hiroshige@chromium.org changed reviewers:
+ mkwst@chromium.org

[hiroshige, 2015-09-02 05:46:27]

+mkwst@, could you take a look?

[Mike West, 2015-09-02 06:14:02]

On 2015/09/02 at 05:46:27, hiroshige wrote:
> +mkwst@, could you take a look?

A few comments inline. Please also add a unit test for the new function you've
added to verify that it works the way you expect.

[Mike West, 2015-09-02 06:59:14]

*ahem* A few comments inline. :)

https://codereview.chromium.org/1018903002/diff/450001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/450001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:132: return false;
Why not just strip leading and trailing whitespace rather than yelling at
developers about them? :)

https://codereview.chromium.org/1018903002/diff/450001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:134: for (unsigned i = 0; i <
value.length(); ++i) {
Can you use the hot new C++11 loop syntax here? Not sure it works with strings,
so this is an honest question. :)

https://codereview.chromium.org/1018903002/diff/450001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:136: if (c == 0x7F || c > 0xFF || (c <
0x20 && c != '\t'))
This seems like it ought to already exist somewhere. Can you do a quick `git gs
'0x20'` to see where else we do this check? I suspect we can extract a character
class out of this.

[shiva.jm, 2015-09-02 09:36:51]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-02 09:36:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018903002/470001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1018903002/470001

[shiva.jm, 2015-09-02 09:37:12]

updated with tests, pls have a look.

https://codereview.chromium.org/1018903002/diff/450001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/450001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:132: return false;
On 2015/09/02 06:59:14, Mike West (buried) wrote:
> Why not just strip leading and trailing whitespace rather than yelling at
> developers about them? :)

striping of leading and trailing whitespace will be applied in another separate
CL after we measure the usage form these patch, as discussed in
https://codereview.chromium.org/1018903002/#msg31 and 
https://codereview.chromium.org/1018903002/#msg33

https://codereview.chromium.org/1018903002/diff/450001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:134: for (unsigned i = 0; i <
value.length(); ++i) {
On 2015/09/02 06:59:14, Mike West (buried) wrote:
> Can you use the hot new C++11 loop syntax here? Not sure it works with
strings,
> so this is an honest question. :)

tried using for ( UChar c : value), but got error:
invalid range expression of type 'const WTF::String'; no viable 'begin' function
available

https://codereview.chromium.org/1018903002/diff/450001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:136: if (c == 0x7F || c > 0xFF || (c <
0x20 && c != '\t'))
On 2015/09/02 06:59:14, Mike West (buried) wrote:
> This seems like it ought to already exist somewhere. Can you do a quick `git
gs
> '0x20'` to see where else we do this check? I suspect we can extract a
character
> class out of this.

In these files same kind of check is done, WTFString.cpp GlyphPageTreeNode.cpp
Response.cpp Document.cpp 
CSSMarkup.cpp CSSPropertyParser.cpp

[hiroshige, 2015-09-02 09:53:48]

https://codereview.chromium.org/1018903002/diff/470001/Source/platform/networ...
File Source/platform/network/HTTPParsersTest.cpp (right):

https://codereview.chromium.org/1018903002/diff/470001/Source/platform/networ...
Source/platform/network/HTTPParsersTest.cpp:253:
EXPECT_FALSE(blink::isValidHTTPFieldContentRFC7230("t\vt"));
Could you add
- a test for null bytes, e.g.
EXPECT_FALSE(blink::isValidHTTPFieldContentRFC7230(String("t\0t", 3))); and
- a test for non-ASCII UTF-16 string (EXPECT_FALSE(...)).

isValidHTTPHeaderValue() rejects \0, \r, \n, and non-latin1 characters, which
should be also rejected by isValidHTTPFieldContentRFC7230().
\r and \n are tested, but \0 and non-latin1 are not.

[Mike West, 2015-09-02 10:01:10]

On 2015/09/02 at 09:37:12, shiva.jm wrote:
> striping of leading and trailing whitespace will be applied in another
separate CL after we measure the usage form these patch, as discussed in
https://codereview.chromium.org/1018903002/#msg31 and 
> https://codereview.chromium.org/1018903002/#msg33

I'm not sure it's valuable to wait, as stripping leading and trailing whitespace
seems like a fairly safe operation. Moreover, the metric you've added doesn't
distinguish between character class failures and whitespace failures. How will
you decide based on the number you're getting whether or not it's safe to strip
the characters?

I'd just send an Intent to Ship, and strip them. :)

> In these files same kind of check is done, WTFString.cpp GlyphPageTreeNode.cpp
Response.cpp Document.cpp 
> CSSMarkup.cpp CSSPropertyParser.cpp

Interesting. Ok, please add `// TODO(mkwst): Extract this character class to a
central location. https://crbug.com/527324`.

-mike

[Mike West, 2015-09-02 10:02:01]

Otherwise LGTM once hiroshige@ is happy with the tests.

[commit-bot: I haz the power, 2015-09-02 10:52:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-02 10:52:54]

Dry run: This issue passed the CQ dry run.

[shiva.jm, 2015-09-03 06:28:05]

updated patch with new tests.

[shiva.jm, 2015-09-03 06:28:30]

pls have a look.

[shiva.jm, 2015-09-03 06:28:40]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-03 06:28:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018903002/490001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1018903002/490001

[commit-bot: I haz the power, 2015-09-03 07:09:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-03 07:09:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shiva.jm, 2015-09-03 10:55:50]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-03 10:55:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018903002/530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1018903002/530001

[shiva.jm, 2015-09-03 10:58:28]

updated patch with test, but for inputs like:'\u007F', '\u2603' and '\u0218',
had build issue like:
a universal-character-name specifies an invalid character, even after tried
using String/CString also.

[commit-bot: I haz the power, 2015-09-03 12:00:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-03 12:00:34]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-09-04 05:23:42]

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:126: if (!value.containsOnlyLatin1())
I think this block is not needed (because this condition is covered by L140: c >
0xFF).

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
File Source/platform/network/HTTPParsersTest.cpp (right):

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
Source/platform/network/HTTPParsersTest.cpp:247:
EXPECT_TRUE(blink::isValidHTTPFieldContentRFC7230(String("77?")));
Is this trying to test non-latin1 cases?
If so, isValidHTTPFieldContentRFC7230() should return false. How about code
like:

const UChar hiraganaA[2] = { 0x3042, 0 };
EXPECT_FALSE(blink::isValidHTTPFieldContentRFC7230(String(hiraganaA)));

(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
Source/platform/network/HTTPParsersTest.cpp:259:
EXPECT_FALSE(blink::isValidHTTPFieldContentRFC7230(String("test \0 data")));
L259-L262 don't test null bytes, e.g. String("test \0 data") is equal to
String("test ").
How about testing:
  String("\0", 1)
  String("test \0", 6)
and removing "test \0 data" (a "t\0t" case is sufficient).

[shiva.jm, 2015-09-04 11:03:09]

updated test and code, pls have a look.

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
File Source/platform/network/HTTPParsers.cpp (right):

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
Source/platform/network/HTTPParsers.cpp:126: if (!value.containsOnlyLatin1())
On 2015/09/04 05:23:42, hiroshige (slow) wrote:
> I think this block is not needed (because this condition is covered by L140: c
>
> 0xFF).

Done.

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
File Source/platform/network/HTTPParsersTest.cpp (right):

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
Source/platform/network/HTTPParsersTest.cpp:247:
EXPECT_TRUE(blink::isValidHTTPFieldContentRFC7230(String("77?")));
On 2015/09/04 05:23:42, hiroshige (slow) wrote:
> Is this trying to test non-latin1 cases?
> If so, isValidHTTPFieldContentRFC7230() should return false. How about code
> like:
> 
> const UChar hiraganaA[2] = { 0x3042, 0 };
> EXPECT_FALSE(blink::isValidHTTPFieldContentRFC7230(String(hiraganaA)));
> 
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)

Done.

https://codereview.chromium.org/1018903002/diff/530001/Source/platform/networ...
Source/platform/network/HTTPParsersTest.cpp:259:
EXPECT_FALSE(blink::isValidHTTPFieldContentRFC7230(String("test \0 data")));
On 2015/09/04 05:23:42, hiroshige (slow) wrote:
> L259-L262 don't test null bytes, e.g. String("test \0 data") is equal to
> String("test ").
> How about testing:
>   String("\0", 1)
>   String("test \0", 6)
> and removing "test \0 data" (a "t\0t" case is sufficient).

Done.

[shiva.jm, 2015-09-04 11:03:45]

The CQ bit was checked by shiva.jm@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-04 11:03:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018903002/550001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1018903002/550001

[commit-bot: I haz the power, 2015-09-04 12:21:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-04 12:21:15]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-09-04 13:16:42]

On 2015/09/04 12:21:15, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

lgtm. Thanks!

[hiroshige, 2015-09-04 13:18:20]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2015-09-04 13:18:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1018903002/#ps550001
(title: " ")

[commit-bot: I haz the power, 2015-09-04 13:18:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018903002/550001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1018903002/550001

[commit-bot: I haz the power, 2015-09-04 13:23:10]

Committed patchset #28 (id:550001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201795

[yuvaa.it, 2016-01-25 17:32:49]

Hi Shiva,
Is it implemented already?
In safari if i am sending a param and value which has trailing spaces 
parameter itself got removed instead of trimming trailing spaces, where as 
in chrome we are getting deprecated warning symbol in console but it is 
allowing trailing space in header.
in chrome console i got redirected 
to https://www.chromestatus.com/feature/6457425448140800

could you please clear overview?

On Friday, August 21, 2015 at 7:51:58 AM UTC+2, shiv...@samsung.com wrote:
>
> On 2015/08/20 16:56:41, shiva.jm wrote: 
> > added deprecation warnings and counter, pls have a look. 
> > Should we add CONSOLE Warning in all XHR and Fetch tests?. 
>
> Using UseCounter::countDeprecation() in Fetch is failing some tests, 
> from FetchHeaderList.cpp and AssociatedURLLoader.cpp, getting context/ 
> document is not available, so need to use 
> UseCounter::deprecationMessage(). 
>
> https://codereview.chromium.org/1018903002/ 
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[shiva.jm, 2016-01-27 06:10:23]

On 2016/01/25 17:32:49, yuvaa.it_gmail.com wrote:
> Hi Shiva,
> Is it implemented already?
> In safari if i am sending a param and value which has trailing spaces 
> parameter itself got removed instead of trimming trailing spaces, where as 
> in chrome we are getting deprecated warning symbol in console but it is 
> allowing trailing space in header.
> in chrome console i got redirected 
> to https://www.chromestatus.com/feature/6457425448140800
> 
> could you please clear overview?
> 
> On Friday, August 21, 2015 at 7:51:58 AM UTC+2, mailto:shiv...@samsung.com
wrote:
> >
> > On 2015/08/20 16:56:41, shiva.jm wrote: 
> > > added deprecation warnings and counter, pls have a look. 
> > > Should we add CONSOLE Warning in all XHR and Fetch tests?. 
> >
> > Using UseCounter::countDeprecation() in Fetch is failing some tests, 
> > from FetchHeaderList.cpp and AssociatedURLLoader.cpp, getting context/ 
> > document is not available, so need to use 
> > UseCounter::deprecationMessage(). 
> >
> > https://codereview.chromium.org/1018903002/ 
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Blink
> Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.


It's not implemented yet, we just show deprecated warning as of now.
As per plan, we added deprecated warnings in XHR/Fetch and UMA for XHR and Net
as of now, based on these measure next plan was to update the 
code as per RFC 7230.
Related CLs and intent link:  https://codereview.chromium.org/1358203003/, 
https://codereview.chromium.org/1018903002/, 
https://codereview.chromium.org/1374883002/,
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/K5jd8Y5l6pI

[hiroshige, 2016-01-27 06:56:56]

On 2016/01/27 06:10:23, shiva.jm wrote:
> On 2016/01/25 17:32:49, http://yuvaa.it_gmail.com wrote:
> > Hi Shiva,
> > Is it implemented already?
> > In safari if i am sending a param and value which has trailing spaces 
> > parameter itself got removed instead of trimming trailing spaces, where as 
> > in chrome we are getting deprecated warning symbol in console but it is 
> > allowing trailing space in header.
> > in chrome console i got redirected 
> > to https://www.chromestatus.com/feature/6457425448140800
> > 
> > could you please clear overview?
> > 
> > On Friday, August 21, 2015 at 7:51:58 AM UTC+2, mailto:shiv...@samsung.com
> wrote:
> > >
> > > On 2015/08/20 16:56:41, shiva.jm wrote: 
> > > > added deprecation warnings and counter, pls have a look. 
> > > > Should we add CONSOLE Warning in all XHR and Fetch tests?. 
> > >
> > > Using UseCounter::countDeprecation() in Fetch is failing some tests, 
> > > from FetchHeaderList.cpp and AssociatedURLLoader.cpp, getting context/ 
> > > document is not available, so need to use 
> > > UseCounter::deprecationMessage(). 
> > >
> > > https://codereview.chromium.org/1018903002/ 
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> "Blink
> > Reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> 
> It's not implemented yet, we just show deprecated warning as of now.
> As per plan, we added deprecated warnings in XHR/Fetch and UMA for XHR and Net
> as of now, based on these measure next plan was to update the 
> code as per RFC 7230.
> Related CLs and intent link:  https://codereview.chromium.org/1358203003/, 
> https://codereview.chromium.org/1018903002/, 
> https://codereview.chromium.org/1374883002/,
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/K5jd8Y5l6pI

Hi,

I'll restart the discussion at the Intent with the UMA numbers.

[yuvaa.it, 2016-01-27 08:55:26]

On 2016/01/27 06:56:56, hiroshige wrote:
> On 2016/01/27 06:10:23, shiva.jm wrote:
> > On 2016/01/25 17:32:49, http://yuvaa.it_gmail.com wrote:
> > > Hi Shiva,
> > > Is it implemented already?
> > > In safari if i am sending a param and value which has trailing spaces 
> > > parameter itself got removed instead of trimming trailing spaces, where as

> > > in chrome we are getting deprecated warning symbol in console but it is 
> > > allowing trailing space in header.
> > > in chrome console i got redirected 
> > > to https://www.chromestatus.com/feature/6457425448140800
> > > 
> > > could you please clear overview?
> > > 
> > > On Friday, August 21, 2015 at 7:51:58 AM UTC+2, mailto:shiv...@samsung.com
> > wrote:
> > > >
> > > > On 2015/08/20 16:56:41, shiva.jm wrote: 
> > > > > added deprecation warnings and counter, pls have a look. 
> > > > > Should we add CONSOLE Warning in all XHR and Fetch tests?. 
> > > >
> > > > Using UseCounter::countDeprecation() in Fetch is failing some tests, 
> > > > from FetchHeaderList.cpp and AssociatedURLLoader.cpp, getting context/ 
> > > > document is not available, so need to use 
> > > > UseCounter::deprecationMessage(). 
> > > >
> > > > https://codereview.chromium.org/1018903002/ 
> > > >
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > "Blink
> > > Reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:blink-reviews+unsubscribe@chromium.org.
> > 
> > 
> > It's not implemented yet, we just show deprecated warning as of now.
> > As per plan, we added deprecated warnings in XHR/Fetch and UMA for XHR and
Net
> > as of now, based on these measure next plan was to update the 
> > code as per RFC 7230.
> > Related CLs and intent link:  https://codereview.chromium.org/1358203003/, 
> > https://codereview.chromium.org/1018903002/, 
> > https://codereview.chromium.org/1374883002/,
> > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/K5jd8Y5l6pI
> 
> Hi,
> 
> I'll restart the discussion at the Intent with the UMA numbers.

Hi,
Yes, Indeed i am getting warninig message as it is deprecated but if it is
implemented in near future the implementation will remove the first and trailing
spaces of the parameter value or it will remove the parameter itself.
eg:
{"headParam":"Testing "} will be implemented as {"headParam":"Testing"} or
"headParam" will be removed from header?