Implement a parser for Content-Disposition header fields

Implement a parser for Content-Disposition header fields

This allows disposition types and parameters to be extracted from
Content-Disposition header fields.

BUG=455103
Review-Url: https://codereview.chromium.org/2880543003
Cr-Commit-Position: refs/heads/master@{#474116}
Committed: https://chromium.googlesource.com/chromium/src/+/2d391fc58e0f24b5d33fadb80527dd2b81193d43

[e_hakkinen, 2017-05-11 10:58:37]

eero.hakkinen@intel.com changed reviewers:
+ haraken@chromium.org

[e_hakkinen, 2017-05-11 10:58:38]

PTAL.

This is needed for Content-Disposition header field parsing for
https://crrev.com/2292763002/

[haraken, 2017-05-11 12:48:39]

I don't have any context for this work and am not sure if I'm a good reviewer
for this. Maybe you can ask yhirano@ and horo@ for review?

[e_hakkinen, 2017-05-12 07:24:27]

eero.hakkinen@intel.com changed reviewers:
+ horo@chromium.org, yhirano@chromium.org

[e_hakkinen, 2017-05-12 07:24:28]

On 2017/05/11 12:48:39, haraken wrote:
> I don't have any context for this work and am not sure if I'm a good reviewer
> for this. Maybe you can ask yhirano@ and horo@ for review?

Sure.

@yhirano, @horo: PTAL.

[yhirano, 2017-05-12 10:59:40]

On 2017/05/12 07:24:28, e_hakkinen wrote:
> On 2017/05/11 12:48:39, haraken wrote:
> > I don't have any context for this work and am not sure if I'm a good
reviewer
> > for this. Maybe you can ask yhirano@ and horo@ for review?
> 
> Sure.
> 
> @yhirano, @horo: PTAL.

Please put more information to the issue description (including BUG=).

[e_hakkinen, 2017-05-12 13:52:32]

Description was changed from

==========
Implement a generalized Content header field parser.
==========

to

==========
Implement a generalized Content header field parser

This allows a content header field parser to be used sensibly also for
content header field other than Content-Type such as Content-Disposition.

BUG=455103
==========

[e_hakkinen, 2017-05-12 13:53:01]

On 2017/05/12 10:59:40, yhirano wrote:
> Please put more information to the issue description (including BUG=).

Done.

[horo, 2017-05-15 05:57:54]

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.cpp
(right):

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.cpp:85:
++index_;
Why you don't use |c|?

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.cpp:156:
ParsedParameterizedContentHeaderField::ParsedParameterizedContentHeaderField(
Who call this?
If you want to use it to parse Content-Disposition, you should add
ParsedContentDisposition class.
And the constructor of ParsedContentDisposition should call 
ParsedContentDisposition::Parse() same as the constructor of ParsedContentType.

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.h
(right):

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.h:89:
String input_;
nit: const String input_;

[yhirano, 2017-05-15 11:14:52]

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/network/ParsedContentType.h (right):

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedContentType.h:47: : public
ParsedParameterizedContentHeaderField {
Please use delegation rather than inheritance.

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.h
(right):

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.h:45:
class PLATFORM_EXPORT ParsedParameterizedContentHeaderField {
Please write tests.

[e_hakkinen, 2017-05-18 09:33:13]

Description was changed from

==========
Implement a generalized Content header field parser

This allows a content header field parser to be used sensibly also for
content header field other than Content-Type such as Content-Disposition.

BUG=455103
==========

to

==========
Implement a parser for Content-Disposition header fields

This allows disposition types and parameters to be extracted from
Content-Disposition header fields.

BUG=455103
==========

[e_hakkinen, 2017-05-18 09:35:49]

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/network/ParsedContentType.h (right):

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedContentType.h:47: : public
ParsedParameterizedContentHeaderField {
On 2017/05/15 11:14:51, yhirano wrote:
> Please use delegation rather than inheritance.

Done.

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.cpp
(right):

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.cpp:85:
++index_;
On 2017/05/15 05:57:54, horo wrote:
> Why you don't use |c|?

Due to too much refactoring. Fixed.

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.cpp:156:
ParsedParameterizedContentHeaderField::ParsedParameterizedContentHeaderField(
On 2017/05/15 05:57:54, horo wrote:
> Who call this?

Anyone wanting to parse any content header field other than Content-Type.

> If you want to use it to parse Content-Disposition, you should add
> ParsedContentDisposition class.
> And the constructor of ParsedContentDisposition should call 
> ParsedContentDisposition::Parse() same as the constructor of
ParsedContentType.

I see the point.

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.h
(right):

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.h:45:
class PLATFORM_EXPORT ParsedParameterizedContentHeaderField {
On 2017/05/15 11:14:51, yhirano wrote:
> Please write tests.

Done.

https://codereview.chromium.org/2880543003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/network/ParsedParameterizedContentHeaderField.h:89:
String input_;
On 2017/05/15 05:57:54, horo wrote:
> nit: const String input_;

Done.

[horo, 2017-05-22 02:31:19]

Thank you.

lgtm with nits.

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

https://codereview.chromium.org/2880543003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.cpp:1: /*
Nit: Please use this license header for new files.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

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

https://codereview.chromium.org/2880543003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.cpp:1:
/*
nit: ditto

[e_hakkinen, 2017-05-22 07:17:09]

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

https://codereview.chromium.org/2880543003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.cpp:1: /*
On 2017/05/22 02:31:19, horo wrote:
> Nit: Please use this license header for new files.
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
> // Copyright 2017 The Chromium Authors. All rights reserved.
> // Use of this source code is governed by a BSD-style license that can be
> // found in the LICENSE file.

Done.

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

https://codereview.chromium.org/2880543003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.cpp:1:
/*
On 2017/05/22 02:31:19, horo wrote:
> nit: ditto

Done.

[e_hakkinen, 2017-05-22 07:21:20]

@haraken, are you willing to give OWNER-lgtm now that @horo reviewed this?

[haraken, 2017-05-22 07:24:36]

yhirano@: Would you mind taking a look at this? I think you should be much more
familiar with network/ than me.

[kinuko, 2017-05-22 07:39:44]

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

[kinuko, 2017-05-22 07:39:45]

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h
(right):

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h:13: class
ContentHeaderFieldInput final {
Please add a class-level comment.

It'd be probably helpful each of these related classes have a lightweight
comment about the relationship

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ParsedContentDisposition.h
(right):

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentDisposition.h:16: //
ParsedContentType parses the constructor argument and stores the result.
This class's not ParsedContentType.

Could you add a little more descriptive comment about what this class is for? 
(The current comment talks about how it works but doesn't tell what it does)

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.h
(right):

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.h:6:
#define ParsedParameterizedContentHeaderField_h
Things look inconsistent across here, class-level comment and class name?

[kinuko, 2017-05-22 07:40:08]

On 2017/05/22 07:21:20, e_hakkinen wrote:
> @haraken, are you willing to give OWNER-lgtm now that @horo reviewed this?

I can do owner review for these, thanks.

[yhirano, 2017-05-22 07:51:20]

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h
(right):

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h:13: class
ContentHeaderFieldInput final {
Please write a class comment.

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h:22: bool
Consume(char);
It would be good to explain what the returned boolean means. Ditto below.

[e_hakkinen, 2017-05-22 08:11:59]

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h
(right):

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h:13: class
ContentHeaderFieldInput final {
On 2017/05/22 07:39:44, kinuko wrote:
> Please add a class-level comment.
> 
> It'd be probably helpful each of these related classes have a lightweight
> comment about the relationship

Done.

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h:22: bool
Consume(char);
On 2017/05/22 07:51:20, yhirano wrote:
> It would be good to explain what the returned boolean means. Ditto below.

Done.

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ParsedContentDisposition.h
(right):

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentDisposition.h:16: //
ParsedContentType parses the constructor argument and stores the result.
On 2017/05/22 07:39:45, kinuko wrote:
> This class's not ParsedContentType.
> 
> Could you add a little more descriptive comment about what this class is for? 
> (The current comment talks about how it works but doesn't tell what it does)

Done.

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.h
(right):

https://codereview.chromium.org/2880543003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.h:6:
#define ParsedParameterizedContentHeaderField_h
On 2017/05/22 07:39:45, kinuko wrote:
> Things look inconsistent across here, class-level comment and class name?

Yep, old class name from before refactoring. Fixed.

[yhirano, 2017-05-23 07:28:27]

lgtm

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.cpp
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.cpp:29:
return parameters_.at(name.DeprecatedLower());
LowerASCII

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.cpp:92:
String key_string = key.ToString().DeprecatedLower();
LowerASCII

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParametersTest.cpp
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParametersTest.cpp:85:
DCHECK_EQ(kelvin.DeprecatedLower(), "k");
LowerUnicode

[kinuko, 2017-05-23 11:27:31]

lgtm

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h:15: //
content header fields into tokens, quoted strings and separators.
Maybe just:

"Parses content header fields into tokens, quoted string and separators.
Commonly used by the ParsedContent* classes."

Should we also mention rfc2045 here?

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h:16: class
ContentHeaderFieldInput final {
Would it make sense to rather call it, say, ContentHeaderFieldTokenizer?

It doesn't feel this class itself should be called 'input', while callers tend
to use this to tokenize input? (Please just let me know if I'm missing
something)

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ParsedContentDisposition.h
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentDisposition.h:16: //
ParsedContentDisposition parses the content of a Content-Disposition header
nit: this can just start with "Parses the..." without repeating the class name.

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.h
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.h:17:
// ParsedContentHeaderFieldParameters parses content header field
ditto

[e_hakkinen, 2017-05-23 20:07:02]

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h:15: //
content header fields into tokens, quoted strings and separators.
On 2017/05/23 11:27:31, kinuko wrote:
> Maybe just:
> 
> "Parses content header fields into tokens, quoted string and separators.
> Commonly used by the ParsedContent* classes."

Done.

> Should we also mention rfc2045 here?

Maybe not. This class is more generic than rfc2045.

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ContentHeaderFieldInput.h:16: class
ContentHeaderFieldInput final {
On 2017/05/23 11:27:31, kinuko wrote:
> Would it make sense to rather call it, say, ContentHeaderFieldTokenizer?
> 
> It doesn't feel this class itself should be called 'input', while callers tend
> to use this to tokenize input? (Please just let me know if I'm missing
> something)

Done.

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ParsedContentDisposition.h
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentDisposition.h:16: //
ParsedContentDisposition parses the content of a Content-Disposition header
On 2017/05/23 11:27:31, kinuko wrote:
> nit: this can just start with "Parses the..." without repeating the class
name.

Done.

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.cpp
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.cpp:29:
return parameters_.at(name.DeprecatedLower());
On 2017/05/23 07:28:27, yhirano wrote:
> LowerASCII

Done.

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.cpp:92:
String key_string = key.ToString().DeprecatedLower();
On 2017/05/23 07:28:27, yhirano wrote:
> LowerASCII

Done.

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.h
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParameters.h:17:
// ParsedContentHeaderFieldParameters parses content header field
On 2017/05/23 11:27:31, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParametersTest.cpp
(right):

https://codereview.chromium.org/2880543003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentHeaderFieldParametersTest.cpp:85:
DCHECK_EQ(kelvin.DeprecatedLower(), "k");
On 2017/05/23 07:28:27, yhirano wrote:
> LowerUnicode

Done.

[e_hakkinen, 2017-05-23 20:20:18]

The CQ bit was checked by eero.hakkinen@intel.com

[e_hakkinen, 2017-05-23 20:20:18]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org,
kinuko@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2880543003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-23 20:21:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 00:11:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495570818500030,
"parent_rev": "a009c5fb061f47e6f6e4c7b486ab217c6ed6e5f9", "commit_rev":
"2d391fc58e0f24b5d33fadb80527dd2b81193d43"}

[commit-bot: I haz the power, 2017-05-24 00:11:54]

Description was changed from

==========
Implement a parser for Content-Disposition header fields

This allows disposition types and parameters to be extracted from
Content-Disposition header fields.

BUG=455103
==========

to

==========
Implement a parser for Content-Disposition header fields

This allows disposition types and parameters to be extracted from
Content-Disposition header fields.

BUG=455103

Review-Url: https://codereview.chromium.org/2880543003
Cr-Commit-Position: refs/heads/master@{#474116}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d391fc58e0f24b5d33fadb80527...
==========

[commit-bot: I haz the power, 2017-05-24 00:11:56]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2d391fc58e0f24b5d33fadb80527...