Share link header parsing code between blink and content.

Share link header parsing code between blink and content.

Introduces a new components/link_header_util with the shared parsing
logic. Moves blinks LinkHeader code from core/loader to platform/network
to be able to depend on this and use std::string.

BUG=605258
Committed: https://crrev.com/9b28f42062acbf9241543d48b9479b449c95fc7b
Cr-Commit-Position: refs/heads/master@{#393650}

[Marijn Kruisselbrink, 2016-03-25 22:10:47]

mek@chromium.org changed reviewers:
+ yoav@yoav.ws

[Marijn Kruisselbrink, 2016-03-25 22:10:47]

Not actually entirely happy with this yet (and it depends on base::optional
which hasn't landed yet), but this is my current attempt at sharing link header
parsing code between content and blink.

As you can see in the changes in LinkHeaderTest, this mostly behaves the same as
the old code, with a few exceptions: 
- invalid urls (in particular urls with embedded whitespace) are treated
differently
- unbalanced quotes are not considered failures by net::HttpUtil, which is now
used
  for parsing.

I'm not sure if you have strong feelings about either of these? For the invalid
URL
difference it seems a bit odd how the existing LinkHeader code just truncated
the URL but
doesn't mark the value as invalid. I'm less sure about the unbalanced quote
change. It 
shouldn't be too hard to add a "strict quotes" mode to
net::HttpUtil::NameValuePairsIterator
that would at least catch some cases with mismatched quotes, but I'm also not
sure if it
is worth the effort.

I'm also still a bit on the edge if trying to share this code is really worth it
in the
first place, since the code isn't that complicated, but saving a few hundred
lines of code
is always nice I suppose...

[Yoav Weiss, 2016-03-29 07:16:40]

On 2016/03/25 22:10:47, Marijn Kruisselbrink wrote:
> Not actually entirely happy with this yet (and it depends on base::optional
> which hasn't landed yet), but this is my current attempt at sharing link
header
> parsing code between content and blink.
> 
> As you can see in the changes in LinkHeaderTest, this mostly behaves the same
as
> the old code, with a few exceptions: 
> - invalid urls (in particular urls with embedded whitespace) are treated
> differently
> - unbalanced quotes are not considered failures by net::HttpUtil, which is now
> used
>   for parsing.
> 
> I'm not sure if you have strong feelings about either of these? For the
invalid
> URL
> difference it seems a bit odd how the existing LinkHeader code just truncated
> the URL but
> doesn't mark the value as invalid. I'm less sure about the unbalanced quote
> change. It 
> shouldn't be too hard to add a "strict quotes" mode to
> net::HttpUtil::NameValuePairsIterator
> that would at least catch some cases with mismatched quotes, but I'm also not
> sure if it
> is worth the effort.
> 
> I'm also still a bit on the edge if trying to share this code is really worth
it
> in the
> first place, since the code isn't that complicated, but saving a few hundred
> lines of code
> is always nice I suppose...

Sorry for the delayed response (travel followed by long weekend). I'll review
this today.

[Yoav Weiss, 2016-03-29 22:05:45]

Initial nits. I'll continue reviewing tomorrow

https://codereview.chromium.org/1811163002/diff/20001/base/optional.h
File base/optional.h (right):

https://codereview.chromium.org/1811163002/diff/20001/base/optional.h#newcode29
base/optional.h:29: // const nullopt_t nullopt(0);
should be deleted?

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
File components/link_header_util/link_header_util.cc (right):

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:111: for (auto it = begin; it !=
end; ++it) {
Can we change that so that we'd iterate over the parameter only once? (maybe a
net::HttpUtil::IsParmname())

[Yoav Weiss, 2016-03-30 07:38:52]

Thanks for working on this! All in all, looks good :) Left a bunch of comments
as I think things can be improved a bit before landing

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
File components/link_header_util/link_header_util.cc (right):

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:21: // in blink::LinkHeader.
That TODO is no longer relevant, right?

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:22: class ValueTokenizer {
Ideally, I'd love to see this goes back to StringTokenizer and HttpUtil. Maybe
add the concept of "non identical quote pairs" to StringTokenizer, and then
inherit from ValuesIterator to set "<>" as a quote pair for this case?

I'm fine with it happening at a separate cleanup CL though.

Otherwise the name ValueTokenizer is not clear in my view, as each token AIUI is
a full Link header. Could you rename?

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:124: values.push_back(
s/values/linkHeaders/ (or something similar)? I think it would make it clearer

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:148: // Extract the URL part
(everything between '<' and first '>' character).
Could you perhaps split the URL parsing part into its own helper function?

https://codereview.chromium.org/1811163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/LinkHeader.cpp (right):

https://codereview.chromium.org/1811163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:43: // FIXME: Add
support for more header parameters as neccessary.
I think we can remove that FIXME

https://codereview.chromium.org/1811163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/LinkHeaderTest.cpp (right):

https://codereview.chromium.org/1811163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeaderTest.cpp:38:
{"</images/cat.jpg wutwut  \t >   ;   rel=prefetch", true, "/images/cat.jpg
wutwut", "prefetch", "", ""},
Looking at https://tools.ietf.org/html/rfc3986 that change seems fine

https://codereview.chromium.org/1811163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeaderTest.cpp:73:
{"<simple.css>; rel=stylesheet; title=\"title with a DQUOTE \\\" and backslash:
\\\"", true, "simple.css", "stylesheet", ""},
That doesn't seem correct according to the definition of quoted-string in
https://tools.ietf.org/html/rfc2616

[Marijn Kruisselbrink, 2016-04-20 00:47:35]

Patchset #3 (id:40001) has been deleted

[Marijn Kruisselbrink, 2016-04-20 00:56:54]

Description was changed from

==========
WIP attempt to share link header parsing code between blink and content

BUG=
==========

to

==========
Share link header parsing code between blink and content.

Introduces a new components/link_header_util with the shared parsing
logic. Moves blinks LinkHeader code from core/loader to platform/network
to be able to depend on this and use std::string.

BUG=
==========

[Marijn Kruisselbrink, 2016-04-20 01:45:04]

I think I addressed most of your comments.

https://codereview.chromium.org/1811163002/diff/20001/base/optional.h
File base/optional.h (right):

https://codereview.chromium.org/1811163002/diff/20001/base/optional.h#newcode29
base/optional.h:29: // const nullopt_t nullopt(0);
On 2016/03/29 at 22:05:45, Yoav Weiss wrote:
> should be deleted?

This was an artifact of me basing this CL on a version of the base::optional
patch that didn't actually compile. Now with base::optional landed this is
indeed no longer needed.

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
File components/link_header_util/link_header_util.cc (right):

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:21: // in blink::LinkHeader.
On 2016/03/30 at 07:38:52, Yoav Weiss wrote:
> That TODO is no longer relevant, right?

Good point, removed.

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:22: class ValueTokenizer {
On 2016/03/30 at 07:38:52, Yoav Weiss wrote:
> Ideally, I'd love to see this goes back to StringTokenizer and HttpUtil. Maybe
add the concept of "non identical quote pairs" to StringTokenizer, and then
inherit from ValuesIterator to set "<>" as a quote pair for this case?

I initially started out by trying to add this to StringTokenizer
(https://codereview.chromium.org/1728833002), but then I realized that it isn't
quite that simple. The URL inside <> does not support \ to escape any characters
(okay, technically the URL is not allowed to contain any \ characters, so this
only matters when parsing invalid Link headers, but it still seemed better to
parse "<foo\>; rel=bar" by not considering the \> an attempt at escaping the >.)


> I'm fine with it happening at a separate cleanup CL though.
> 
> Otherwise the name ValueTokenizer is not clear in my view, as each token AIUI
is a full Link header. Could you rename?

I named it ValueTokenizer since it's most similar to net::ValuesIterator.
Furthermore what it does is split a string into the things that should be parsed
according to the "link-value" rule in in RFC 5988 (i.e. what you call a "full
Link header" is called a "link value" by RFC 5988).

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:111: for (auto it = begin; it !=
end; ++it) {
On 2016/03/29 at 22:05:45, Yoav Weiss wrote:
> Can we change that so that we'd iterate over the parameter only once? (maybe a
net::HttpUtil::IsParmname())

I agree that that would be ideal (that's also why I had the TODO comment above).
Since this CL will need //net OWNERS review anyway, let's see what they say. If
they're happy with having this function in net::HttpUtil I'll gladly move it
there. Otherwise adding a net::HttpUtil::IsTokenChar(char) might be a decent
middle ground.

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:124: values.push_back(
On 2016/03/30 at 07:38:52, Yoav Weiss wrote:
> s/values/linkHeaders/ (or something similar)? I think it would make it clearer

I used values because that's the terminology the spec uses. Also "headers"
wouldn't really be more correct. One header can contain arbitrary many links.
That's the whole reason we need to do this splitting.

https://codereview.chromium.org/1811163002/diff/20001/components/link_header_...
components/link_header_util/link_header_util.cc:148: // Extract the URL part
(everything between '<' and first '>' character).
On 2016/03/30 at 07:38:52, Yoav Weiss wrote:
> Could you perhaps split the URL parsing part into its own helper function?

Done

https://codereview.chromium.org/1811163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/LinkHeader.cpp (right):

https://codereview.chromium.org/1811163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:43: // FIXME: Add
support for more header parameters as neccessary.
On 2016/03/30 at 07:38:52, Yoav Weiss wrote:
> I think we can remove that FIXME

Done

https://codereview.chromium.org/1811163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/LinkHeaderTest.cpp (right):

https://codereview.chromium.org/1811163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeaderTest.cpp:73:
{"<simple.css>; rel=stylesheet; title=\"title with a DQUOTE \\\" and backslash:
\\\"", true, "simple.css", "stylesheet", ""},
On 2016/03/30 at 07:38:52, Yoav Weiss wrote:
> That doesn't seem correct according to the definition of quoted-string in
https://tools.ietf.org/html/rfc2616

Yeah, net::HttpUtil::NameValuePairsIterator deliberately accepts misquoted
strings ("Due to buggy implementations found in some embedded devices, we also
accept values with missing close quotemark (http://crbug.com/39836):").
But I agree that we shouldn't encourage buggy implementations for new features,
so it probably makes sense to be more strict here. I added a "strict quotes"
flag to NameValuePairsIterator, assuming //net OWNERS are happy with such a
thing...

[kinuko, 2016-04-20 02:57:47]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-04-20 02:57:48]

Huh neat. A few drive-by nit-picking comments..

https://codereview.chromium.org/1811163002/diff/80001/components/link_header_...
File components/link_header_util/link_header_util_unittest.cc (right):

https://codereview.chromium.org/1811163002/diff/80001/components/link_header_...
components/link_header_util/link_header_util_unittest.cc:110: // Test data
mostly copied from blink::LinkHeaderTest. Expectations for some
now that we're merging these this comment looks stale / confusing?

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/LinkHeader.cpp (right):

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:65: if (m_isValid) {
nit: prefer early return

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:85: }
nit: prefer no { } for single-line body in blink

[kinuko, 2016-04-20 02:58:04]

Description was changed from

==========
Share link header parsing code between blink and content.

Introduces a new components/link_header_util with the shared parsing
logic. Moves blinks LinkHeader code from core/loader to platform/network
to be able to depend on this and use std::string.

BUG=
==========

to

==========
Share link header parsing code between blink and content.

Introduces a new components/link_header_util with the shared parsing
logic. Moves blinks LinkHeader code from core/loader to platform/network
to be able to depend on this and use std::string.

BUG=
==========

[kinuko, 2016-04-20 02:58:04]

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

[dcheng, 2016-04-20 04:41:06]

Drive-by

https://codereview.chromium.org/1811163002/diff/80001/components/link_header_...
File components/link_header_util/link_header_util.cc (right):

https://codereview.chromium.org/1811163002/diff/80001/components/link_header_...
components/link_header_util/link_header_util.cc:201:
params->insert(std::make_pair(name, base::nullopt_t(0)));
Just write base::nullopt here.

[Yoav Weiss, 2016-04-20 09:42:05]

core/ LGTM % a few nits

The "value" terminology seems fine if it matches the spec. Could you perhaps add
a comment stating that and linking to the spec?

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/LinkHeader.cpp (right):

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:1: // Copyright 2015
The Chromium Authors. All rights reserved.
s/2015/2016/

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:18: static
LinkHeader::LinkParameterName parameterNameFromString(String name)
s/String/const String&/ ?

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:41: void
LinkHeader::setValue(LinkParameterName name, String value)
s/String/const String&/ ?

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:81:
ASSERT(header.is8Bit());
I believe we now deprecated ASSERT in favor of DCHECK

[Marijn Kruisselbrink, 2016-04-20 17:58:05]

Patchset #5 (id:100001) has been deleted

[Marijn Kruisselbrink, 2016-04-20 20:50:34]

Description was changed from

==========
Share link header parsing code between blink and content.

Introduces a new components/link_header_util with the shared parsing
logic. Moves blinks LinkHeader code from core/loader to platform/network
to be able to depend on this and use std::string.

BUG=
==========

to

==========
Share link header parsing code between blink and content.

Introduces a new components/link_header_util with the shared parsing
logic. Moves blinks LinkHeader code from core/loader to platform/network
to be able to depend on this and use std::string.

BUG=605258
==========

[Marijn Kruisselbrink, 2016-04-20 21:07:48]

https://codereview.chromium.org/1811163002/diff/80001/components/link_header_...
File components/link_header_util/link_header_util.cc (right):

https://codereview.chromium.org/1811163002/diff/80001/components/link_header_...
components/link_header_util/link_header_util.cc:201:
params->insert(std::make_pair(name, base::nullopt_t(0)));
On 2016/04/20 at 04:41:06, dcheng wrote:
> Just write base::nullopt here.

Done. I initially wrote this against an older version of the base::optional
patch where base::nullopt didn't compile. But now that works of course.

https://codereview.chromium.org/1811163002/diff/80001/components/link_header_...
File components/link_header_util/link_header_util_unittest.cc (right):

https://codereview.chromium.org/1811163002/diff/80001/components/link_header_...
components/link_header_util/link_header_util_unittest.cc:110: // Test data
mostly copied from blink::LinkHeaderTest. Expectations for some
On 2016/04/20 at 02:57:48, kinuko wrote:
> now that we're merging these this comment looks stale / confusing?

Good point. Updated the comment to actually match the new version of the code.

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/LinkHeader.cpp (right):

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2016/04/20 at 09:42:04, Yoav Weiss wrote:
> s/2015/2016/

I guess it's arguable if this is a new file or just moving the existing
LinkHeader.cpp and removing most of the code... But since at least half of the
new LinkHeader.cpp is just code from the old LinkHeader.cpp leaving the
copyright year alone probably makes the most sense.

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:18: static
LinkHeader::LinkParameterName parameterNameFromString(String name)
On 2016/04/20 at 09:42:04, Yoav Weiss wrote:
> s/String/const String&/ ?

Done

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:41: void
LinkHeader::setValue(LinkParameterName name, String value)
On 2016/04/20 at 09:42:05, Yoav Weiss wrote:
> s/String/const String&/ ?

Done

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:65: if (m_isValid) {
On 2016/04/20 at 02:57:48, kinuko wrote:
> nit: prefer early return

Done

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:81:
ASSERT(header.is8Bit());
On 2016/04/20 at 09:42:04, Yoav Weiss wrote:
> I believe we now deprecated ASSERT in favor of DCHECK

Done

https://codereview.chromium.org/1811163002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:85: }
On 2016/04/20 at 02:57:48, kinuko wrote:
> nit: prefer no { } for single-line body in blink

Done

[Marijn Kruisselbrink, 2016-04-21 23:07:50]

mek@chromium.org changed reviewers:
+ jochen@chromium.org, mmenke@chromium.org

[Marijn Kruisselbrink, 2016-04-21 23:07:50]

+jochen for components/ and third_party/WebKit/Source/platform and content/
OWNERS

+mmenke for net/ OWNERS (and to answer if adding a method to check if a string
matches the "parmname" rule from
https://tools.ietf.org/html/rfc5987#section-3.2.1 to net/http/http_util.h seems
sensible? Or alternately having an IsToken overload (or IsTokenChar method) to
check if a char rather than a string is valid in a token).

[mmenke, 2016-04-22 15:28:41]

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc#...
net/http/http_util.cc:539: return false;
In this case we've partially populated out, which seems like a bad idea.  To be
consistent, I think we should go with one of two options on failure:

1)  Don't touch out on failure.  So have a temporary string, and then swap it
with out on success.
2)  Guarantee out is cleared (So call out->clear() here, and move the call at
line 531 to the start of this method), and document this behavior in the API
docs.

Given the strictness guarantee, I think people who use this method should be
checking the return value, so I'd recommend we go with 1), and add
WARN_UNUSED_RESULT to the function declarations.

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc#...
net/http/http_util.cc:540: }
nit:  Don't use braces for two line if (1 line condition, 1 line body).

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc#...
net/http/http_util.cc:1035: if (*value_begin_ != *(value_end_ - 1) ||
value_begin_ + 1 == value_end_) {
My main concern with doing this is that now we have 3 different unquoting
behaviors.  We have Unquote, we have NameValuePairsIterator::GetNext() in
no-strict mode, which is like Quote but also allows unmatched quotes, and we
have HttpUtil::StrictUnquote, which is much more strict, and we fairly
arbitrarily choose between the last two.

We should definitely be sharing parsing logic, but with this change, I'm not
sure we really are.

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc#...
net/http/http_util.cc:1044: if (strict_quotes_)
Can we just move this outside both the statements?

if (strict_quotes_) {
  if (!HttpUtil::StrictUnquote(...))
    return valid_ = false;
  return true;
}

if (value_begin_ != value_end_ && HttpUtil::IsQuote(*value_begin_))
  // Trim surrounding quotemarks off the value

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.h#n...
net/http/http_util.h:356: enum StrictQuotes { STRICT_QUOTES, NON_STRICT_QUOTES
};
Please make this an enum class (And the other one, too, if things compile with
it)

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.h#n...
net/http/http_util.h:362: StrictQuotes strict_quotes = NON_STRICT_QUOTES);
There's a general style preference in net not to use default values (Notice that
the below method could be expressed as this one, with an extra argument). 
Suggest making the extra argument mandatory.  There aren't a huge number of
places this constructor (4, 1 outside of net?), so shouldn't be a huge deal.

[jochen (gone - plz use gerrit), 2016-04-22 15:29:19]

jochen@chromium.org changed reviewers:
+ esprehn@chromium.org

[jochen (gone - plz use gerrit), 2016-04-22 15:29:20]

+esprehn for the platform/ stuff

[Marijn Kruisselbrink, 2016-04-25 19:52:11]

mmenke: hopefully addressed your comments; and I'm still wondering about your
opinion about  adding a method to check if a string matches the "parmname" rule
from https://tools.ietf.org/html/rfc5987#section-3.2.1 to net/http/http_util.h
seems sensible? Or alternately having an IsToken overload (or IsTokenChar
method) to check if a char rather than a string is valid in a token, to make an
implementation of such a method outside of HttpUtil nicer.

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc#...
net/http/http_util.cc:539: return false;
On 2016/04/22 at 15:28:40, mmenke wrote:
> In this case we've partially populated out, which seems like a bad idea.  To
be consistent, I think we should go with one of two options on failure:
> 
> 1)  Don't touch out on failure.  So have a temporary string, and then swap it
with out on success.
> 2)  Guarantee out is cleared (So call out->clear() here, and move the call at
line 531 to the start of this method), and document this behavior in the API
docs.
> 
> Given the strictness guarantee, I think people who use this method should be
checking the return value, so I'd recommend we go with 1), and add
WARN_UNUSED_RESULT to the function declarations.

Done

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc#...
net/http/http_util.cc:540: }
On 2016/04/22 at 15:28:41, mmenke wrote:
> nit:  Don't use braces for two line if (1 line condition, 1 line body).

Done

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc#...
net/http/http_util.cc:1035: if (*value_begin_ != *(value_end_ - 1) ||
value_begin_ + 1 == value_end_) {
On 2016/04/22 at 15:28:41, mmenke wrote:
> My main concern with doing this is that now we have 3 different unquoting
behaviors.  We have Unquote, we have NameValuePairsIterator::GetNext() in
no-strict mode, which is like Quote but also allows unmatched quotes, and we
have HttpUtil::StrictUnquote, which is much more strict, and we fairly
arbitrarily choose between the last two.

Yeah, the existing behavior around how malformed quoted strings are treated is a
bit odd/inconsistent (as the comment here mentions). I don't think I'm making
anything worse by introducing a more spec-compliant/consistent mode to parse
name/value pairs though. And I would say that for any new header we want to
parse we should use strict quotes mode, rather than encouraging non-compliant
implementations of new features.

> We should definitely be sharing parsing logic, but with this change, I'm not
sure we really are.

We're definitely still sharing parsing logic for link headers between blink and
content, which was the main goal of this CL. So I'm not sure why you think we're
not sharing parsing logic anymore? Arguably some of the existing uses of
NameValuePairsIterator should probably have used a strict quotes mode as well,
to not encourage broken implementations of new features (it's okay to be more
lenient with parsing headers for existing features if that increases
compatibility, but trying to parse malformed headers for new features only risks
decreasing compatibility by encouraging broken implementations that might or
might not work in other browsers).

And unfortunately due to peculiarities of link headers the link header parsing
code did end up with its own modified version of ValuesIterator (it did not seem
desirably to extend ValuesIterator and/or base::StringTokenizer to be able to
deal with these peculiarities).

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc#...
net/http/http_util.cc:1044: if (strict_quotes_)
On 2016/04/22 at 15:28:41, mmenke wrote:
> Can we just move this outside both the statements?
> 
> if (strict_quotes_) {
>   if (!HttpUtil::StrictUnquote(...))
>     return valid_ = false;
>   return true;
> }
> 
> if (value_begin_ != value_end_ && HttpUtil::IsQuote(*value_begin_))
>   // Trim surrounding quotemarks off the value

Moving the strict_quotes_ code to before the HttpUtil::IsQuote check would be
wrong, as even in strict quotes mode we still want to support unquoted values as
well. But separating the strict quotes logic from the "graceful" non-strict
unquoting logic does make sense, so done that.

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.h#n...
net/http/http_util.h:356: enum StrictQuotes { STRICT_QUOTES, NON_STRICT_QUOTES
};
On 2016/04/22 at 15:28:41, mmenke wrote:
> Please make this an enum class (And the other one, too, if things compile with
it)

Done (to make the other an enum class I of course needed to update all the
callsites, but then to make the quotes argument not optional I already needed to
do that anyway).

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.h#n...
net/http/http_util.h:362: StrictQuotes strict_quotes = NON_STRICT_QUOTES);
On 2016/04/22 at 15:28:41, mmenke wrote:
> There's a general style preference in net not to use default values (Notice
that the below method could be expressed as this one, with an extra argument). 
Suggest making the extra argument mandatory.  There aren't a huge number of
places this constructor (4, 1 outside of net?), so shouldn't be a huge deal.

Okay, made the new argument mandatory.

[mmenke, 2016-04-26 17:27:15]

On 2016/04/25 19:52:11, Marijn Kruisselbrink wrote:
> mmenke: hopefully addressed your comments; and I'm still wondering about your
> opinion about  adding a method to check if a string matches the "parmname"
rule
> from https://tools.ietf.org/html/rfc5987#section-3.2.1 to net/http/http_util.h
> seems sensible? Or alternately having an IsToken overload (or IsTokenChar
> method) to check if a char rather than a string is valid in a token, to make
an
> implementation of such a method outside of HttpUtil nicer.

I'm fine with adding an IsParmname method (IsParmName?).  Think it makes more
sense to put the method in net, rather than make a consumer use an IsTokenChar
method to implement the logic itself.  I'm also fine with adding IsTokenChar,
just think having the paramname logic alongside everything else makes the most
sense.

[mmenke, 2016-04-26 18:05:58]

Sorry for slowness, review overload last week.  Should be more responsive now.

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1811163002/diff/140001/net/http/http_util.cc#...
net/http/http_util.cc:1035: if (*value_begin_ != *(value_end_ - 1) ||
value_begin_ + 1 == value_end_) {
On 2016/04/25 19:52:10, Marijn Kruisselbrink wrote:
> On 2016/04/22 at 15:28:41, mmenke wrote:
> > My main concern with doing this is that now we have 3 different unquoting
> behaviors.  We have Unquote, we have NameValuePairsIterator::GetNext() in
> no-strict mode, which is like Quote but also allows unmatched quotes, and we
> have HttpUtil::StrictUnquote, which is much more strict, and we fairly
> arbitrarily choose between the last two.
> 
> Yeah, the existing behavior around how malformed quoted strings are treated is
a
> bit odd/inconsistent (as the comment here mentions). I don't think I'm making
> anything worse by introducing a more spec-compliant/consistent mode to parse
> name/value pairs though. And I would say that for any new header we want to
> parse we should use strict quotes mode, rather than encouraging non-compliant
> implementations of new features.
> 
> > We should definitely be sharing parsing logic, but with this change, I'm not
> sure we really are.
> 
> We're definitely still sharing parsing logic for link headers between blink
and
> content, which was the main goal of this CL. So I'm not sure why you think
we're
> not sharing parsing logic anymore? Arguably some of the existing uses of
> NameValuePairsIterator should probably have used a strict quotes mode as well,
> to not encourage broken implementations of new features (it's okay to be more
> lenient with parsing headers for existing features if that increases
> compatibility, but trying to parse malformed headers for new features only
risks
> decreasing compatibility by encouraging broken implementations that might or
> might not work in other browsers).
> 
> And unfortunately due to peculiarities of link headers the link header parsing
> code did end up with its own modified version of ValuesIterator (it did not
seem
> desirably to extend ValuesIterator and/or base::StringTokenizer to be able to
> deal with these peculiarities).

My point with this comment is that half the parsing code here is different, so
it may be the same method call, but it's bifurcated code in net/, so still
completely different codepaths.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc#...
net/http/http_util.cc:504: bool
HttpUtil::StrictUnquote(std::string::const_iterator begin,
Hrm...Should this and Unquote share an implementation?  The only differences are
the c == quote check, and the terminal escape characted check, right?

I'd suggest a shared implementation in an anonymous namespace in this file, and
just have these two methods call it.  It makes the differences between them
clearer, and gets us a bit better test coverage of the code.  Also makes
accidental future divergence of the two methods less likely.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc#...
net/http/http_util.cc:526: if (begin != end && *(end - 1) == '\\')
BUG:  "\\" is valid.  Instead, should have a "prev_unescaped == false" check on
the end.  Add a test for this.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc#...
net/http/http_util.cc:1037: if (value_begin_ != value_end_ &&
HttpUtil::IsQuote(*value_begin_)) {
So strict mode fails on foo='bar but succeeds on foo=bar'?  IsToken even returns
true on the latter (Though if it were ", it would return false).  Is this
expected behavior?

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.h#n...
net/http/http_util.h:138: // unescaped actually is a valid quoted string.
Returns false for an empty
Think we should mention this still allows single quotes, contrary to RFC 2616.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.h#n...
net/http/http_util.h:143: std::string* out) WARN_UNUSED_RESULT;
nit:  Include "base/macros.h" for WARN_UNUSED_RESULT.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util_uni...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util_uni...
net/http/http_util_unittest.cc:282: EXPECT_TRUE(HttpUtil::StrictUnquote("'x\"'",
&out));
Suggest duplicating this test,but with single and double quotes switched.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util_uni...
net/http/http_util_unittest.cc:284: }
Some more other suggested tests:

Minimal quoted string with escaped final quote ("\").  Know you have a test for
this case, just want a smaller string to make sure there are no bounds check
issues (There aren't, but I'm paranoid).

Empty string.

Quoted empty string.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1217: std::string data = "foo=\"bar\";
name='value";
Could you also test a case with a missing end quote (Codepaths are basically the
same now, but were different in an earlier iteration of this CL, so seems worth
still checking), and an unquoted string?

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1217: std::string data = "foo=\"bar\";
name='value";
Also, shouldn't use mismatched quotes in this test - HttpUtil::Unquote also
doesn't allow mismatched quotes, so I think this would fail in non-string mode,
too, right?  So do something that only fails in strict mode (\ before final
quote, or an extra quote in the middle of the value).

[Marijn Kruisselbrink, 2016-04-26 22:51:47]

Patchset #8 (id:180001) has been deleted

[Marijn Kruisselbrink, 2016-04-27 01:49:50]

Still working on figuring out what to do about the windows compile errors (I
believe windows.h #define's OPTIONAL. So I'll probably need to come up with a
different name for Values::OPTIONAL)

yoav: see my question below about why LinkHeader in blink decided to allow
single quotes for quoted strings despite the spec allowing single quotes to be
part of unquoted values.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc#...
net/http/http_util.cc:504: bool
HttpUtil::StrictUnquote(std::string::const_iterator begin,
On 2016/04/26 at 18:05:57, mmenke wrote:
> Hrm...Should this and Unquote share an implementation?  The only differences
are the c == quote check, and the terminal escape characted check, right?
> 
> I'd suggest a shared implementation in an anonymous namespace in this file,
and just have these two methods call it.  It makes the differences between them
clearer, and gets us a bit better test coverage of the code.  Also makes
accidental future divergence of the two methods less likely.

Good idea. Done.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc#...
net/http/http_util.cc:526: if (begin != end && *(end - 1) == '\\')
On 2016/04/26 at 18:05:57, mmenke wrote:
> BUG:  "\\" is valid.  Instead, should have a "prev_unescaped == false" check
on the end.  Add a test for this.

Yeah, just caught that myself too. Added a test and fixed the code.

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc#...
net/http/http_util.cc:1037: if (value_begin_ != value_end_ &&
HttpUtil::IsQuote(*value_begin_)) {
On 2016/04/26 at 18:05:57, mmenke wrote:
> So strict mode fails on foo='bar but succeeds on foo=bar'?  IsToken even
returns true on the latter (Though if it were ", it would return false).  Is
this expected behavior?

Argh, excellent question... And not really something I have an answer to. Maybe
it makes most sense for strict-mode to disallow single quotes entirely. Also
because treating single quotes as quoted strings is actually possibly
spec-incompliant (at least in the Link header spec foo='bar is perfectly legal,
assigning "'bar" to the foo extended parameter. Similarly foo='a\b' should
result in "'a\b'" as value, without any unescaping/unquoting).

Yoav: any particular reason you wrote LinkHeader.cpp to (incorrectly) unquote
strings in single quotes?

Of course as you point out the situation is also weird for foo=bar". At least
code consuming NameValuePairsIterator could detect that via
(!it.value_is_quoted() && !HttpUtil::IsToken(it.value_begin(), it.value_end())).
I don't think it makes sense to make NameValuePairsIterator even more strict by
implying this IsToken check as part of strict quotes (for example the Link
header spec actually uses a |ptoken| rule as valid unquoted values, which allows
for more characters than a plain |token|...)

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.h#n...
net/http/http_util.h:138: // unescaped actually is a valid quoted string.
Returns false for an empty
On 2016/04/26 at 18:05:58, mmenke wrote:
> Think we should mention this still allows single quotes, contrary to RFC 2616.

Done

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.h#n...
net/http/http_util.h:143: std::string* out) WARN_UNUSED_RESULT;
On 2016/04/26 at 18:05:58, mmenke wrote:
> nit:  Include "base/macros.h" for WARN_UNUSED_RESULT.

Done

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util_uni...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1217: std::string data = "foo=\"bar\";
name='value";
On 2016/04/26 at 18:05:58, mmenke wrote:
> Also, shouldn't use mismatched quotes in this test - HttpUtil::Unquote also
doesn't allow mismatched quotes, so I think this would fail in non-string mode,
too, right?  So do something that only fails in strict mode (\ before final
quote, or an extra quote in the middle of the value).

This does in fact only fail in strict mode (the existing test right above it
verifies that non-strict mode doesn't fail with missing end quotes).


On 2016/04/26 at 18:05:58, mmenke wrote:
> Could you also test a case with a missing end quote (Codepaths are basically
the same now, but were different in an earlier iteration of this CL, so seems
worth still checking), and an unquoted string?

So the test case here was exactly testing for that missing end quote case. Now
added a few other test cases for other situations as well to better cover the
code.

[esprehn, 2016-04-27 04:00:01]

Can we use kFoo named constants? That's what we plan to do in blink.

-- 
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.

[esprehn, 2016-04-27 04:00:02]

Can we use kFoo named constants? That's what we plan to do in blink.

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

[mmenke, 2016-04-27 04:02:23]

On 2016/04/27 04:00:02, esprehn wrote:
> Can we use kFoo named constants? That's what we plan to do in blink.
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

All caps enums are the prevailing style in net/, so I think we should just stick
with it.

[mmenke, 2016-04-27 04:02:40]

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util_uni...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1217: std::string data = "foo=\"bar\";
name='value";
On 2016/04/27 01:49:50, Marijn Kruisselbrink wrote:
> On 2016/04/26 at 18:05:58, mmenke wrote:
> > Also, shouldn't use mismatched quotes in this test - HttpUtil::Unquote also
> doesn't allow mismatched quotes, so I think this would fail in non-string
mode,
> too, right?  So do something that only fails in strict mode (\ before final
> quote, or an extra quote in the middle of the value).
> 
> This does in fact only fail in strict mode (the existing test right above it
> verifies that non-strict mode doesn't fail with missing end quotes).
> 
> 
> On 2016/04/26 at 18:05:58, mmenke wrote:
> > Could you also test a case with a missing end quote (Codepaths are basically
> the same now, but were different in an earlier iteration of this CL, so seems
> worth still checking), and an unquoted string?
> 
> So the test case here was exactly testing for that missing end quote case. Now
> added a few other test cases for other situations as well to better cover the
> code.

Oops, sorry, misread the test - was thinking the final quote was part of the
quoted string.

[esprehn, 2016-04-27 08:10:18]

https://codereview.chromium.org/1811163002/diff/160001/components/link_header...
File components/link_header_util/DEPS (right):

https://codereview.chromium.org/1811163002/diff/160001/components/link_header...
components/link_header_util/DEPS:4: "-content",
also block third_party/WebKit, this code needs to be fully isolated

https://codereview.chromium.org/1811163002/diff/160001/components/link_header...
File components/link_header_util/link_header_util.h (right):

https://codereview.chromium.org/1811163002/diff/160001/components/link_header...
components/link_header_util/link_header_util.h:23: std::string::const_iterator
end,
why doesn't this take a const std::string& instead and deal with the iterators
internally?

https://codereview.chromium.org/1811163002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/LinkHeader.cpp (right):

https://codereview.chromium.org/1811163002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:18: static
LinkHeader::LinkParameterName parameterNameFromString(const String& name)
We could also take a base::StringPiece and use whatever base would use to do the
comparison below.

https://codereview.chromium.org/1811163002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:20: if
(equalIgnoringCase(name, "rel"))
This is unicode aware, the other code uses ASCII compares, I think you want the
same here?

https://codereview.chromium.org/1811163002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:57: template <typename
Iterator>
why do we need the template?

https://codereview.chromium.org/1811163002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:67: m_url =
String(&url[0], url.length());
It's always latin1?

[jochen (gone - plz use gerrit), 2016-04-27 13:14:33]

lgtm

https://codereview.chromium.org/1811163002/diff/200001/components/link_header...
File components/link_header_util/link_header_util.cc (right):

https://codereview.chromium.org/1811163002/diff/200001/components/link_header...
components/link_header_util/link_header_util.cc:100: };
disallow copy/assign

[Yoav Weiss, 2016-04-27 13:33:18]

On 2016/04/27 01:49:50, Marijn Kruisselbrink wrote:
> Still working on figuring out what to do about the windows compile errors (I
> believe windows.h #define's OPTIONAL. So I'll probably need to come up with a
> different name for Values::OPTIONAL)
> 
> yoav: see my question below about why LinkHeader in blink decided to allow
> single quotes for quoted strings despite the spec allowing single quotes to be
> part of unquoted values.
> 
> https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc
> File net/http/http_util.cc (right):
> 
>
https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc#...
> net/http/http_util.cc:504: bool
> HttpUtil::StrictUnquote(std::string::const_iterator begin,
> On 2016/04/26 at 18:05:57, mmenke wrote:
> > Hrm...Should this and Unquote share an implementation?  The only differences
> are the c == quote check, and the terminal escape characted check, right?
> > 
> > I'd suggest a shared implementation in an anonymous namespace in this file,
> and just have these two methods call it.  It makes the differences between
them
> clearer, and gets us a bit better test coverage of the code.  Also makes
> accidental future divergence of the two methods less likely.
> 
> Good idea. Done.
> 
>
https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc#...
> net/http/http_util.cc:526: if (begin != end && *(end - 1) == '\\')
> On 2016/04/26 at 18:05:57, mmenke wrote:
> > BUG:  "\\" is valid.  Instead, should have a "prev_unescaped == false" check
> on the end.  Add a test for this.
> 
> Yeah, just caught that myself too. Added a test and fixed the code.
> 
>
https://codereview.chromium.org/1811163002/diff/160001/net/http/http_util.cc#...
> net/http/http_util.cc:1037: if (value_begin_ != value_end_ &&
> HttpUtil::IsQuote(*value_begin_)) {
> On 2016/04/26 at 18:05:57, mmenke wrote:
> > So strict mode fails on foo='bar but succeeds on foo=bar'?  IsToken even
> returns true on the latter (Though if it were ", it would return false).  Is
> this expected behavior?
> 
> Argh, excellent question... And not really something I have an answer to.
Maybe
> it makes most sense for strict-mode to disallow single quotes entirely. Also
> because treating single quotes as quoted strings is actually possibly
> spec-incompliant (at least in the Link header spec foo='bar is perfectly
legal,
> assigning "'bar" to the foo extended parameter. Similarly foo='a\b' should
> result in "'a\b'" as value, without any unescaping/unquoting).
> 
> Yoav: any particular reason you wrote LinkHeader.cpp to (incorrectly) unquote
> strings in single quotes?
> 

No reason in particular. Dropping it and aligning to the spec sgtm.

[mmenke, 2016-05-02 19:13:23]

Marijn:  Are you waiting on me, or just haven't had a chance to respond to
everyone else's comments?  I've been waiting for you to respond, but it occurs
to me that you may be waiting on me.  Not trying to rush you, just want to make
sure we agree on whose court this is in.

[Marijn Kruisselbrink, 2016-05-02 19:59:26]

On 2016/05/02 at 19:13:23, mmenke wrote:
> Marijn:  Are you waiting on me, or just haven't had a chance to respond to
everyone else's comments?  I've been waiting for you to respond, but it occurs
to me that you may be waiting on me.  Not trying to rush you, just want to make
sure we agree on whose court this is in.

Just haven't had time yet to further respond. This being merely a code cleanup
it isn't terribly high on my list of things to work on...

[Marijn Kruisselbrink, 2016-05-11 00:36:56]

On 2016/04/27 at 04:02:23, mmenke wrote:
> All caps enums are the prevailing style in net/, so I think we should just
stick with it.

Changed it from OPTIONAL/NOT_OPTIONAL to NOT_REQUIRED/REQUIRED to stick with all
caps,
while avoiding annoying windows.h defines.



On 2016/04/27 at 13:33:18, yoav wrote:
> On 2016/04/27 01:49:50, Marijn Kruisselbrink wrote:
> >
> > Yoav: any particular reason you wrote LinkHeader.cpp to (incorrectly)
unquote
> > strings in single quotes?
> > 
> 
> No reason in particular. Dropping it and aligning to the spec sgtm.

Okay, changed the strict quotes mode in net::HttpUtils to not consider single
quotes for quoted strings. It's a bit icky the way I've done it now. I wonder if
maybe it would make sense to just pass a Quotes::STRICT/Quotes::NOT_STRICT flag
to every method dealing with quotes (Unquote, IsQuote, ValuesIterator,
NameValuePairsIterator), rather than the somewhat ad-hoc way things are done
now.

https://codereview.chromium.org/1811163002/diff/160001/components/link_header...
File components/link_header_util/DEPS (right):

https://codereview.chromium.org/1811163002/diff/160001/components/link_header...
components/link_header_util/DEPS:4: "-content",
On 2016/04/27 at 08:10:17, esprehn wrote:
> also block third_party/WebKit, this code needs to be fully isolated

Done

https://codereview.chromium.org/1811163002/diff/160001/components/link_header...
File components/link_header_util/link_header_util.h (right):

https://codereview.chromium.org/1811163002/diff/160001/components/link_header...
components/link_header_util/link_header_util.h:23: std::string::const_iterator
end,
On 2016/04/27 at 08:10:17, esprehn wrote:
> why doesn't this take a const std::string& instead and deal with the iterators
internally?

It takes the output from SplitLinkHeader, and parses that. I considered using
const std::string&, base::StringPiece and iterator pairs, and ended up using
iterator pairs because that's what the rest of net::HttpUtil does. Using
StringPiece would require changing the rest of net::HttpUtil to have overloads
for StringPiece (which might be a good idea, but beyond the scope of this
patch). Using std::string would introduce unnecessary copies (which isn't the
end of the world, but there are already enough string copies being made, so no
reason to make it worse). So using pairs of iterators is both consistent with
similar code in net::HttpUtil and seems to me to be the right balance between
efficient code and usable API.

https://codereview.chromium.org/1811163002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/LinkHeader.cpp (right):

https://codereview.chromium.org/1811163002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:18: static
LinkHeader::LinkParameterName parameterNameFromString(const String& name)
On 2016/04/27 at 08:10:17, esprehn wrote:
> We could also take a base::StringPiece and use whatever base would use to do
the comparison below.

Good point. Changed it to base::StringPiece and
base::EqualsCaseInsensitiveASCII.

https://codereview.chromium.org/1811163002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:57: template <typename
Iterator>
On 2016/04/27 at 08:10:17, esprehn wrote:
> why do we need the template?

Without a template the header file would need to include <string> to be able to
explicitly use std::string::const_iterator. It seemed preferred to not leak
std::string implementation details passed the implementation. But I agree it's a
bit weird to use a template when only one type can ever be used as template
parameter.

https://codereview.chromium.org/1811163002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/LinkHeader.cpp:67: m_url =
String(&url[0], url.length());
On 2016/04/27 at 08:10:17, esprehn wrote:
> It's always latin1?

LinkHeaderSet bellow DCHECKs to make sure it's always 8 bit (and in the latest
PS I made the LinkHeader constructor private with LinkHeaderSet as friend), and
beyond that it's pretty much guaranteed that headers are always latin1 (or at
least sequences of octets), yes. Certainly by the way content passes headers to
blink, and all the other places where headers are created as blink strings that
I looked at verify that they're latin1.

[mmenke, 2016-05-11 15:00:28]

net/ LGTM, though looks like some of the bots are sad.

https://codereview.chromium.org/1811163002/diff/220001/net/http/http_util_uni...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1811163002/diff/220001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1276: std::string data = "foo=\"bar\";
name='value; ok=it's";
Should the second string end in a single quote, so it's also valid strict
single-quoted string (Other than the fact strict doesn't allow single quoted
strings)?

[Marijn Kruisselbrink, 2016-05-11 18:12:13]

On 2016/05/11 at 15:00:28, mmenke wrote:
> looks like some of the bots are sad.

Hmm, yeah... seems windows doesn't like STRICT as an enum value either...
Changed it to STRICT_QUOTES, even though Quotes::STRICT_QUOTES is a bit
redundant.
And fixed the one link header layout test that was (incorrectly) using single
quotes.

https://codereview.chromium.org/1811163002/diff/220001/net/http/http_util_uni...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1811163002/diff/220001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1276: std::string data = "foo=\"bar\";
name='value; ok=it's";
On 2016/05/11 at 15:00:28, mmenke wrote:
> Should the second string end in a single quote, so it's also valid strict
single-quoted string (Other than the fact strict doesn't allow single quoted
strings)?

Yeah, that's not a bad idea. Done.

[Marijn Kruisselbrink, 2016-05-13 18:41:52]

esprehn: ping?

[esprehn, 2016-05-13 19:36:12]

lgtm

https://codereview.chromium.org/1811163002/diff/240001/components/link_header...
File components/link_header_util/link_header_util.h (right):

https://codereview.chromium.org/1811163002/diff/240001/components/link_header...
components/link_header_util/link_header_util.h:25:
std::vector<StringIteratorPair> SplitLinkHeader(const std::string& header);
this could just be a std::vector of StringPiece too right?

[Marijn Kruisselbrink, 2016-05-13 19:39:38]

https://codereview.chromium.org/1811163002/diff/240001/components/link_header...
File components/link_header_util/link_header_util.h (right):

https://codereview.chromium.org/1811163002/diff/240001/components/link_header...
components/link_header_util/link_header_util.h:25:
std::vector<StringIteratorPair> SplitLinkHeader(const std::string& header);
On 2016/05/13 at 19:36:12, esprehn wrote:
> this could just be a std::vector of StringPiece too right?

It could, but that would require modifying a whole slew of methods in
net::HttpUtil to accept a StringPiece rather than a pair of iterators. That
might be a sensible thing to do (after all a pair of iterators is trivially
convertible to a StringPiece, and the HttpUtil methods don't really do anything
that requires specifically iterators), but that seems out of scope of this CL.

[esprehn, 2016-05-13 19:56:26]

On 2016/05/13 at 19:39:38, mek wrote:
>
https://codereview.chromium.org/1811163002/diff/240001/components/link_header...
> File components/link_header_util/link_header_util.h (right):
> 
>
https://codereview.chromium.org/1811163002/diff/240001/components/link_header...
> components/link_header_util/link_header_util.h:25:
std::vector<StringIteratorPair> SplitLinkHeader(const std::string& header);
> On 2016/05/13 at 19:36:12, esprehn wrote:
> > this could just be a std::vector of StringPiece too right?
> 
> It could, but that would require modifying a whole slew of methods in
net::HttpUtil to accept a StringPiece rather than a pair of iterators. That
might be a sensible thing to do (after all a pair of iterators is trivially
convertible to a StringPiece, and the HttpUtil methods don't really do anything
that requires specifically iterators), but that seems out of scope of this CL.

Yeah definitely not in this CL, but it does seem much cleaner in the code. :)

[Marijn Kruisselbrink, 2016-05-13 20:13:15]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2016-05-13 20:13:17]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws,
jochen@chromium.org, mmenke@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1811163002/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-13 20:14:06]

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

[commit-bot: I haz the power, 2016-05-13 21:46:35]

Committed patchset #11 (id:260001)

[commit-bot: I haz the power, 2016-05-13 21:48:37]

Description was changed from

==========
Share link header parsing code between blink and content.

Introduces a new components/link_header_util with the shared parsing
logic. Moves blinks LinkHeader code from core/loader to platform/network
to be able to depend on this and use std::string.

BUG=605258
==========

to

==========
Share link header parsing code between blink and content.

Introduces a new components/link_header_util with the shared parsing
logic. Moves blinks LinkHeader code from core/loader to platform/network
to be able to depend on this and use std::string.

BUG=605258

Committed: https://crrev.com/9b28f42062acbf9241543d48b9479b449c95fc7b
Cr-Commit-Position: refs/heads/master@{#393650}
==========

[commit-bot: I haz the power, 2016-05-13 21:48:39]

Patchset 11 (id:??) landed as
https://crrev.com/9b28f42062acbf9241543d48b9479b449c95fc7b
Cr-Commit-Position: refs/heads/master@{#393650}