Add unittests to ParsedContentType

Add unittests to ParsedContentType

This CL doesn't include a behavioral change.

BUG=689731
Review-Url: https://codereview.chromium.org/2704143002
Cr-Commit-Position: refs/heads/master@{#451697}
Committed: https://chromium.googlesource.com/chromium/src/+/d133322623bc4dca3cfcf69e1e83d979cf2dfd9c

[yhirano, 2017-02-20 06:49:12]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 06:49:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-02-20 07:20:11]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 07:20:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-02-20 07:37:04]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 07:37:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-02-20 07:37:45]

Description was changed from

==========
Add unittests to ParsedContentType

This CL ass unittests for ParsedContentType. This CL includes a comment update
and minor style fix-ups. This CL doesn't change the behavior.

BUG=689731
==========

to

==========
Add unittests to ParsedContentType

This CL doesn't include a behavioral change.

BUG=689731
==========

[yhirano, 2017-02-20 07:37:45]

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

[kinuko, 2017-02-20 08:27:56]

Looks good.  Um... not directly related to this CL but do you think we could
deprecate mime/ContentType (no tests, buggy) and merge it into this one?  Also
the similar code in HTTPParsers.cpp?

[yhirano, 2017-02-20 08:48:19]

On 2017/02/20 08:27:56, kinuko wrote:
> Looks good.  Um... not directly related to this CL but do you think we could
> deprecate mime/ContentType (no tests, buggy) and merge it into this one?  Also
> the similar code in HTTPParsers.cpp?

Sorry, what do you mean by "mime/ContentType (no tests, buggy)"?

net::HttpUtil::ParseContentType also provides a similar function, but I don't
like it either because

 - It parses a fixed set of parameters ([1] needs "type" parameter which this
function doesn't provide), and
 - It doesn't reject invalid input strings (apparently we need isValid()).


1:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mhtml...

[kinuko, 2017-02-20 09:14:13]

On 2017/02/20 08:48:19, yhirano wrote:
> On 2017/02/20 08:27:56, kinuko wrote:
> > Looks good.  Um... not directly related to this CL but do you think we could
> > deprecate mime/ContentType (no tests, buggy) and merge it into this one? 
Also
> > the similar code in HTTPParsers.cpp?
> 
> Sorry, what do you mean by "mime/ContentType (no tests, buggy)"?

I meant platform/network/mime/ContentType.{h,cpp}

> net::HttpUtil::ParseContentType also provides a similar function, but I don't
> like it either because
> 
>  - It parses a fixed set of parameters ([1] needs "type" parameter which this
> function doesn't provide), and
>  - It doesn't reject invalid input strings (apparently we need isValid()).

Yep I don't think this can work for us for now either.

[kinuko, 2017-02-20 10:04:32]

The test code itself lgtm, btw

https://codereview.chromium.org/2704143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ParsedContentTypeTest.cpp
(right):

https://codereview.chromium.org/2704143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentTypeTest.cpp:46:
EXPECT_EQ(String(), t.charset());
Do you prefer having these distinct tests over setting up some test cases +
expected results in an array and loop over them?  (Either is fine with me)

[commit-bot: I haz the power, 2017-02-20 10:07:10]

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

[commit-bot: I haz the power, 2017-02-20 10:07:11]

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

[yhirano, 2017-02-20 10:22:24]

https://codereview.chromium.org/2704143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/network/ParsedContentTypeTest.cpp
(right):

https://codereview.chromium.org/2704143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/network/ParsedContentTypeTest.cpp:46:
EXPECT_EQ(String(), t.charset());
On 2017/02/20 10:04:32, kinuko wrote:
> Do you prefer having these distinct tests over setting up some test cases +
> expected results in an array and loop over them?  (Either is fine with me)

As long as I can give a meaningful name, I prefer having separated ones. (I
cannot do so for each case in Validity and ParameterName).

[yhirano, 2017-02-21 03:13:21]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2017-02-21 03:13:35]

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

[yhirano, 2017-02-21 04:02:58]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 04:03:09]

Dry run: 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-02-21 04:03:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487646801950220,
"parent_rev": "853a4cb2161bbd4f16e6aff541bb5912ac83d850", "commit_rev":
"d133322623bc4dca3cfcf69e1e83d979cf2dfd9c"}

[commit-bot: I haz the power, 2017-02-21 04:04:50]

Description was changed from

==========
Add unittests to ParsedContentType

This CL doesn't include a behavioral change.

BUG=689731
==========

to

==========
Add unittests to ParsedContentType

This CL doesn't include a behavioral change.

BUG=689731

Review-Url: https://codereview.chromium.org/2704143002
Cr-Commit-Position: refs/heads/master@{#451697}
Committed:
https://chromium.googlesource.com/chromium/src/+/d133322623bc4dca3cfcf69e1e83...
==========

[commit-bot: I haz the power, 2017-02-21 04:04:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d133322623bc4dca3cfcf69e1e83...

[yhirano, 2017-02-21 04:05:30]

Patchset #4 (id:60001) has been deleted