Add a comma delimited syntax parser and use it for Accept-CH

Add a comma delimited syntax parser and use it for Accept-CH

The Accept-CH syntax needed a proper parser.
Since the syntax is a simple comma delimited one, I've created a generic
parser that can also be used elsewhere.

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

[Yoav Weiss, 2015-02-26 15:44:14]

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

[Yoav Weiss, 2015-02-26 15:44:15]

As discussed, here's a generic comma delimited parser, used to fix the lack of
Accept-CH parser.

[Mike West, 2015-02-27 08:53:41]

https://codereview.chromium.org/961773002/diff/20001/Source/platform/CommaDel...
File Source/platform/CommaDelimitedParser.cpp (right):

https://codereview.chromium.org/961773002/diff/20001/Source/platform/CommaDel...
Source/platform/CommaDelimitedParser.cpp:10: static bool isWhitespace(UChar chr)
Can you use isASCIIWhitespace instead (in string_util.h)?

https://codereview.chromium.org/961773002/diff/40001/Source/platform/CommaDel...
File Source/platform/CommaDelimitedParser.h (right):

https://codereview.chromium.org/961773002/diff/40001/Source/platform/CommaDel...
Source/platform/CommaDelimitedParser.h:14: class CommaDelimitedParser {
I'm not sure this is a good name. It does parse a string, but you're treating it
like a Set, not like a parser.

How about 'CommaDelimitedHeaderSet'?

Really, this isn't much more than a typedef around a HashSet, and a parsing
function. It might make more sense to implement it that way as part of
`platform/network/HTTPParsers.h`. Something like:

```
using CommaDelimitedHeaderSet = HashSet<String, CaseFoldingHash>;
PLATFORM_EXPORT bool parseCommaDelimitedHeaderValue(const String& header,
CommaDelimitedHeaderSet& result);
```

WDYT?

https://codereview.chromium.org/961773002/diff/40001/Source/platform/CommaDel...
Source/platform/CommaDelimitedParser.h:21: HashMap<String, bool,
CaseFoldingHash> m_map;
I'd suggest a HashSet rather than a map containing a bool. :)

[Yoav Weiss, 2015-02-27 10:01:44]

On 2015/02/27 08:53:41, Mike West wrote:
>
https://codereview.chromium.org/961773002/diff/20001/Source/platform/CommaDel...
> File Source/platform/CommaDelimitedParser.cpp (right):
> 
>
https://codereview.chromium.org/961773002/diff/20001/Source/platform/CommaDel...
> Source/platform/CommaDelimitedParser.cpp:10: static bool isWhitespace(UChar
chr)
> Can you use isASCIIWhitespace instead (in string_util.h)?
> 
>
https://codereview.chromium.org/961773002/diff/40001/Source/platform/CommaDel...
> File Source/platform/CommaDelimitedParser.h (right):
> 
>
https://codereview.chromium.org/961773002/diff/40001/Source/platform/CommaDel...
> Source/platform/CommaDelimitedParser.h:14: class CommaDelimitedParser {
> I'm not sure this is a good name. It does parse a string, but you're treating
it
> like a Set, not like a parser.
> 
> How about 'CommaDelimitedHeaderSet'?
> 
> Really, this isn't much more than a typedef around a HashSet, and a parsing
> function. It might make more sense to implement it that way as part of
> `platform/network/HTTPParsers.h`. Something like:
> 
> ```
> using CommaDelimitedHeaderSet = HashSet<String, CaseFoldingHash>;
> PLATFORM_EXPORT bool parseCommaDelimitedHeaderValue(const String& header,
> CommaDelimitedHeaderSet& result);
> ```
> 
> WDYT?
> 
>
https://codereview.chromium.org/961773002/diff/40001/Source/platform/CommaDel...
> Source/platform/CommaDelimitedParser.h:21: HashMap<String, bool,
> CaseFoldingHash> m_map;
> I'd suggest a HashSet rather than a map containing a bool. :)

Makes sense. Pretty much copied your code :) Let me know what you think

[Mike West, 2015-02-27 10:07:55]

LGTM, if you extend the unit tests you added in the previous patch to include
some of the edge cases that previously failed, but now succeed. `dprw`, and any
of the other interesting bits from the Set-specific tests you're adding here.
Just want to make sure we don't regress the behavior if we end up changing the
implementation at some point in the future. :)

[Yoav Weiss, 2015-02-27 10:22:26]

On 2015/02/27 10:07:55, Mike West wrote:
> LGTM, if you extend the unit tests you added in the previous patch to include
> some of the edge cases that previously failed, but now succeed. `dprw`, and
any
> of the other interesting bits from the Set-specific tests you're adding here.
> Just want to make sure we don't regress the behavior if we end up changing the
> implementation at some point in the future. :)

You're right. Added a layout test to make sure "dprw" fails.

[Mike West, 2015-02-27 10:27:53]

Beautiful. Land it!

[Yoav Weiss, 2015-02-27 10:31:38]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-02-27 10:31:38]

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/961773002/#ps100001
(title: "Added layout test")

[commit-bot: I haz the power, 2015-02-27 10:32:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/961773002/100001

[commit-bot: I haz the power, 2015-02-27 11:46:23]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190987