Add 8bit Content-Transfer-Encoding support for reading MHTML files

Add 8bit Content-Transfer-Encoding support for reading MHTML files

R=abarth@chromium.org
BUG=274232
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156720

[abarth-chromium, 2013-08-07 18:12:27]

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

https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
Source/web/tests/MHTMLTest.cpp:51: 
We usually skip this blank line

https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
Source/web/tests/MHTMLTest.cpp:61: static PassRefPtr<SharedBuffer>
readFile(const char* fileName)
This function looks copy/pasted from readFile in WebImageTest.  Instead of
copy/pasting the code, can we move the other implementation to a place where we
can share the code?

https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
Source/web/tests/MHTMLTest.cpp:78: 
You've got an extra blank line here.

https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
Source/web/tests/MHTMLTest.cpp:82: if (data.get()) {
There's no need to call .get() here.  RefPtr has a boolean operator.

https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
Source/web/tests/MHTMLTest.cpp:93: RefPtr<MHTMLArchive> archive =
loadMHTMLArchive("8bit-transfer-encoding.mhtml");
Rather than writing a unit test for this change, you should write a LayoutTest. 
The rest of the tests for loading MHTML archives are LayoutTests:

https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/mhtml/

[Tiger, 2013-08-08 09:45:28]

On 2013/08/07 18:12:27, abarth wrote:
> https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cpp
> File Source/web/tests/MHTMLTest.cpp (right):
> 
>
https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
> Source/web/tests/MHTMLTest.cpp:51: 
> We usually skip this blank line
> 
>
https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
> Source/web/tests/MHTMLTest.cpp:61: static PassRefPtr<SharedBuffer>
> readFile(const char* fileName)
> This function looks copy/pasted from readFile in WebImageTest.  Instead of
> copy/pasting the code, can we move the other implementation to a place where
we
> can share the code?
> 
>
https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
> Source/web/tests/MHTMLTest.cpp:78: 
> You've got an extra blank line here.
> 
>
https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
> Source/web/tests/MHTMLTest.cpp:82: if (data.get()) {
> There's no need to call .get() here.  RefPtr has a boolean operator.
> 
>
https://codereview.chromium.org/22292008/diff/1/Source/web/tests/MHTMLTest.cp...
> Source/web/tests/MHTMLTest.cpp:93: RefPtr<MHTMLArchive> archive =
> loadMHTMLArchive("8bit-transfer-encoding.mhtml");
> Rather than writing a unit test for this change, you should write a
LayoutTest. 
> The rest of the tests for loading MHTML archives are LayoutTests:
> 
> https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/mhtml/

Ah, layout tests. That was certainly a bit easier. Thanks! Uploaded a new patch
set that uses layout test instead. Also added the mhtml generator I'm using. Not
sure if that is worth keeping it in the blink repo though, but it might be
useful to someone else.

[abarth-chromium, 2013-08-08 17:48:36]

LGTM

It's a bit odd that this folder uses _ in the file names rather than -, but I
don't think it's super important.

Thanks for the generator script.  That looks useful.  I might have put it in the
resources directory so that folks know it's not meant to be a test.

[commit-bot: I haz the power, 2013-08-08 17:48:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/22292008/6001

[commit-bot: I haz the power, 2013-08-08 21:16:26]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[commit-bot: I haz the power, 2013-08-12 12:13:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/22292008/28001

[commit-bot: I haz the power, 2013-08-12 13:32:15]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-08-12 14:17:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/22292008/28001

[commit-bot: I haz the power, 2013-08-12 15:36:10]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-08-13 08:56:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/22292008/28001

[commit-bot: I haz the power, 2013-08-13 10:18:17]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-08-13 14:42:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/22292008/52001

[commit-bot: I haz the power, 2013-08-13 16:19:02]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Tiger, 2013-08-14 08:19:33]

@abarth, any idea why these tests should crash on the build bots? I'm unable to
reproduce this locally.

[abarth-chromium, 2013-08-14 08:24:55]

Nope...  That doesn't make a lot of sense.  It seems to work on Windows.  Maybe
Linux and Mac are different somehow?

[Tiger, 2013-08-14 09:34:15]

On 2013/08/14 08:24:55, abarth wrote:
> Nope...  That doesn't make a lot of sense.  It seems to work on Windows. 
Maybe
> Linux and Mac are different somehow?

Don't know. I'm testing on linux here locally, and that work. Can the trybots
give some more debug info somehow? Or build a debug build?

[commit-bot: I haz the power, 2013-08-14 13:23:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/22292008/67001

[Tiger, 2013-08-14 13:28:59]

On 2013/08/14 08:24:55, abarth wrote:
> Nope...  That doesn't make a lot of sense.  It seems to work on Windows. 
Maybe
> Linux and Mac are different somehow?

My current theory is that windows normalized the line endings of the new files
to CRLF and to LF on linux and mac, and the mhtml files of course need to have
CRLF line endings in the header. Perhaps I forgot to set some flags somewhere
else too, but I'm trying to set the -crlf flag in .gitattributes for mhtml/*.mht
first.

[Tiger, 2013-08-14 14:39:43]

On 2013/08/14 13:28:59, Tiger wrote:
> On 2013/08/14 08:24:55, abarth wrote:
> > Nope...  That doesn't make a lot of sense.  It seems to work on Windows. 
> Maybe
> > Linux and Mac are different somehow?
> 
> My current theory is that windows normalized the line endings of the new files
> to CRLF and to LF on linux and mac, and the mhtml files of course need to have
> CRLF line endings in the header. Perhaps I forgot to set some flags somewhere
> else too, but I'm trying to set the -crlf flag in .gitattributes for
mhtml/*.mht
> first.

Indeed, doing git cl patch 22292008 on a clean tree gives me the mht files with
the wrong line endings. Trying to fix it and the doing git cl upload results in
a patch set that does nothing. So somewhere along the way, something eats the
line endings.

[commit-bot: I haz the power, 2013-08-14 15:42:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tiger@opera.com/22292008/96001

[abarth-chromium, 2013-08-14 17:45:17]

Why do bad line ends result in a crash?

[commit-bot: I haz the power, 2013-08-14 18:01:03]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Tiger, 2013-08-14 23:53:12]

On 2013/08/14 17:45:17, abarth wrote:
> Why do bad line ends result in a crash?

I don't remember the exact details now but if the MIME message, and especially
the headers uses LF instead instead of CRLF (as the standard specifies) it
crashes either in the parsing of the MIME message/header parser or in the MHTML
specific parts. One could argue that it should fail more gracefully with an
invalid document, but fixing that issue (a bit unrelated to this task) will
still not make these layout tests work. I can look this up if you want the
details.

The MHTML files should be treated as binary files by the systems, and I've tried
to mark them as such here in both by attaching svn props when uploading with git
cl upload and by setting the .gitattribues, but alas, they still get corrupted
somewhere from the uploads scripts to the review system. I guess I will run into
this problem again with a couple of other review (23043002, for example) so it
would be good to get this sorted out. Who can know more about this?

[abarth-chromium, 2013-08-14 23:56:11]

It's fine to fix that in a separate CL, but no inputs should cause the browser
to crash.

[Tiger, 2013-08-15 11:04:00]

Perhaps this is just an effect of
<https://code.google.com/p/chromium/issues/detail?id=23608>, and the binary
files have to be committed manually?

[Tiger, 2013-08-15 14:28:45]

On 2013/08/15 11:04:00, Tiger wrote:
> Perhaps this is just an effect of
> <https://code.google.com/p/chromium/issues/detail?id=23608>, and the binary
> files have to be committed manually?

Yeah, my blink-dev post seem to confirm this, as well as the try server page
<https://sites.google.com/a/chromium.org/dev/developers/testing/try-server-usage>.
So this CL (among other I suppose) have to be committed without tests, and
adding the tests manually later.

[abarth-chromium, 2013-08-26 19:51:25]

Committed patchset #14 manually as r156720 (presubmit successful).