Merge page serializers [7/12]

Move MHTML test, remove old test files

Move WebPageNewSerializeTest.TestMHTMLEncoding (currently as FAILS_*) to
MHTMLTest and fixing it, removing FAILS_ prefix. This is the last
remaining test in WebPageNewSerializeTest so removing the files.

--

This review is part of set aiming to merge the two existing page
serializers, WebPageSerializerImpl and PageSerializer into the latter
one.

In addition to this it moves all the old tests from
WebPageNewSerializerTest and WebPageSerializerTest to the newer
PageSerializerTest structure and splits out a test for MHTML into the
MHTMLTest file.

Note: This is a rebase and split of
https://codereview.chromium.org/68613003 from Opera which was reverted
due to failing tests. In addition to the rebase some smaller fixes was
done to accommodate to changes in the code from when it was first
landed.

BUG=328354
R=hajimehoshi@chromium.org

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

[Tiger (Sony Mobile), 2015-06-10 08:48:52]

gustav.tiger@sonymobile.com changed reviewers:
+ hajimehoshi@chromium.org
- philipj@opera.com

[Tiger (Sony Mobile), 2015-06-10 08:49:17]

PTAL

[hajimehoshi, 2015-06-10 09:02:03]

lgtm

https://codereview.chromium.org/1172993002/diff/1/Source/web/tests/MHTMLTest.cpp
File Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/1172993002/diff/1/Source/web/tests/MHTMLTest....
Source/web/tests/MHTMLTest.cpp:73: } else {
nit: early return is better

https://codereview.chromium.org/1172993002/diff/1/Source/web/tests/MHTMLTest....
Source/web/tests/MHTMLTest.cpp:215: EXPECT_TRUE(line.find(expectedEncoding) !=
std::string::npos);
nit: EXPECT_NE

[Tiger (Sony Mobile), 2015-06-10 10:30:37]

https://codereview.chromium.org/1172993002/diff/1/Source/web/tests/MHTMLTest.cpp
File Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/1172993002/diff/1/Source/web/tests/MHTMLTest....
Source/web/tests/MHTMLTest.cpp:73: } else {
On 2015/06/10 09:02:03, hajimehoshi wrote:
> nit: early return is better

Done.

https://codereview.chromium.org/1172993002/diff/1/Source/web/tests/MHTMLTest....
Source/web/tests/MHTMLTest.cpp:215: EXPECT_TRUE(line.find(expectedEncoding) !=
std::string::npos);
On 2015/06/10 09:02:03, hajimehoshi wrote:
> nit: EXPECT_NE

Done.

[philipj_slow, 2015-06-10 12:10:07]

philipj@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2015-06-10 12:10:07]

LGTM with voluntary nit. hajimehoshi@, do you think it's OK the way it is?

https://codereview.chromium.org/1172993002/diff/20001/Source/web/tests/MHTMLT...
File Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/1172993002/diff/20001/Source/web/tests/MHTMLT...
Source/web/tests/MHTMLTest.cpp:200: if (!line.find("Content-Type:")) {
Because std::string::find returns 0 if the match is at the first position this
is correct, but reading it certainly gives the opposite impression.
line.find("Content-Type") == 0 would be more clear, but since that's frowned
upon maybe writing this with std::string::compare would make it more clear?
Saves the effort of scanning the whole string, too.

https://codereview.chromium.org/1172993002/diff/20001/Source/web/tests/MHTMLT...
Source/web/tests/MHTMLTest.cpp:214: if
(!line.find("Content-Transfer-Encoding:")) {
Ditto

[hajimehoshi, 2015-06-10 13:49:10]

On 2015/06/10 12:10:07, philipj wrote:
> LGTM with voluntary nit. hajimehoshi@, do you think it's OK the way it is?

I missed that point. I agree with you. I prefer 'compare' way.

[Tiger (Sony Mobile), 2015-06-10 13:50:08]

https://codereview.chromium.org/1172993002/diff/20001/Source/web/tests/MHTMLT...
File Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/1172993002/diff/20001/Source/web/tests/MHTMLT...
Source/web/tests/MHTMLTest.cpp:200: if (!line.find("Content-Type:")) {
On 2015/06/10 12:10:07, philipj wrote:
> Because std::string::find returns 0 if the match is at the first position this
> is correct, but reading it certainly gives the opposite impression.
> line.find("Content-Type") == 0 would be more clear, but since that's frowned
> upon maybe writing this with std::string::compare would make it more clear?
> Saves the effort of scanning the whole string, too.

Done.

https://codereview.chromium.org/1172993002/diff/20001/Source/web/tests/MHTMLT...
Source/web/tests/MHTMLTest.cpp:214: if
(!line.find("Content-Transfer-Encoding:")) {
On 2015/06/10 12:10:07, philipj wrote:
> Ditto

Done.

[philipj_slow, 2015-06-10 13:58:39]

https://codereview.chromium.org/1172993002/diff/40001/Source/web/tests/MHTMLT...
File Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/1172993002/diff/40001/Source/web/tests/MHTMLT...
Source/web/tests/MHTMLTest.cpp:200: if (line.compare(0, 13, "Content-Type:") ==
0) {
I'm not sure, but I think line.compare(std::string("Content-Type:")) must use
*some* length and if it's the minimum of the two string it's exactly what you
want and avoid off-by-one (or two) errors in the string length. If that doesn't
work, this is fine too.

[Tiger (Sony Mobile), 2015-06-10 14:16:19]

https://codereview.chromium.org/1172993002/diff/40001/Source/web/tests/MHTMLT...
File Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/1172993002/diff/40001/Source/web/tests/MHTMLT...
Source/web/tests/MHTMLTest.cpp:200: if (line.compare(0, 13, "Content-Type:") ==
0) {
On 2015/06/10 13:58:39, philipj wrote:
> I'm not sure, but I think line.compare(std::string("Content-Type:")) must use
> *some* length and if it's the minimum of the two string it's exactly what you
> want and avoid off-by-one (or two) errors in the string length. If that
doesn't
> work, this is fine too.

It does the comparison on the shortest string but then if everything match it
additionally looks at the length of the strings as well, so it must be written
like this.

[Tiger (Sony Mobile), 2015-06-10 15:15:41]

The CQ bit was checked by gustav.tiger@sonymobile.com

[Tiger (Sony Mobile), 2015-06-10 15:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
hajimehoshi@chromium.org, philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1172993002/#ps40001
(title: "Fixes nits")

[commit-bot: I haz the power, 2015-06-10 15:15:51]

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

[philipj_slow, 2015-06-10 16:42:52]

https://codereview.chromium.org/1172993002/diff/40001/Source/web/tests/MHTMLT...
File Source/web/tests/MHTMLTest.cpp (right):

https://codereview.chromium.org/1172993002/diff/40001/Source/web/tests/MHTMLT...
Source/web/tests/MHTMLTest.cpp:200: if (line.compare(0, 13, "Content-Type:") ==
0) {
On 2015/06/10 14:16:19, Tiger (Sony Mobile) wrote:
> On 2015/06/10 13:58:39, philipj wrote:
> > I'm not sure, but I think line.compare(std::string("Content-Type:")) must
use
> > *some* length and if it's the minimum of the two string it's exactly what
you
> > want and avoid off-by-one (or two) errors in the string length. If that
> doesn't
> > work, this is fine too.
> 
> It does the comparison on the shortest string but then if everything match it
> additionally looks at the length of the strings as well, so it must be written
> like this.

Oh, fair enough :)

[commit-bot: I haz the power, 2015-06-10 18:16:03]

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

[commit-bot: I haz the power, 2015-06-10 18:16:04]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/66061)

[philipj_slow, 2015-06-10 18:45:41]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2015-06-10 18:45:49]

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

[commit-bot: I haz the power, 2015-06-10 19:40:06]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196892