Reland "UTF-8 detector for pages missing encoding info"

Reland "UTF-8 detector for pages missing encoding info"

TextResourceDecoder is designed (or used) in such a way that the text
encoding of a document gets resolved from the first chunk (as big as
4096 bytes) of the text received from network - by BOM, meta tag, or
auto encoding detection (if enabled).

The newly introduced UTF-8 encoding detector crrev.com/1721373002 was
reverted (crbug.com/603558) because it attempted to work in a bit
different way - it examined all the subsequent chunks as well in search
of non-ASCII-UTF-8-encoded char sequence. This means it is possible for
TextResourceDecoder to start with a codec for, say, windows-1252, and
then later switch to one for UTF-8. Theoretically this should still work
but doesn't in practice (maybe hasn't been used/tested in that
way). This is what happened with failed perf tests - one of the js files
was big (13K), of pure ASCII except one tiny char sequence \xc2\xa7
almost at the end.

The CL was updated so the UTF-8 encoding detection also works against
the first chunk only like other methods, to avoid potential codec
switching in the middle.

BUG=583549, 603558
Committed: https://crrev.com/57139d64c5b98142ca9305792f39ae23a4950375
Cr-Commit-Position: refs/heads/master@{#388927}

[Jinsuk Kim, 2016-04-15 12:37:26]

The ideal fix would be to allow TextResourceDecoder to switch codecs but first,
this is a quick fix. This is at least no worse than how it was before. I'm
thinking of looking into codec switching approach in a follow up CL, plus tests.
Feel free to let me know what you think.

[jungshik at Google, 2016-04-15 19:10:36]

On 2016/04/15 12:37:26, Jinsuk wrote:
> The ideal fix would be to allow TextResourceDecoder to switch codecs but
first,
> this is a quick fix. This is at least no worse than how it was before. I'm
> thinking of looking into codec switching approach in a follow up CL, plus
tests.
> Feel free to let me know what you think.

Could you point me where this CL is different from what you landed (and got
reverted) previously? I just had a cursory look (I'm not supposed to work today
;-)) and fail to find a difference.

Thanks

[Jinsuk Kim, 2016-04-15 19:20:40]

On 2016/04/15 19:10:36, jshin (ooo Fri aka jungshik) wrote:
> On 2016/04/15 12:37:26, Jinsuk wrote:
> > The ideal fix would be to allow TextResourceDecoder to switch codecs but
> first,
> > this is a quick fix. This is at least no worse than how it was before. I'm
> > thinking of looking into codec switching approach in a follow up CL, plus
> tests.
> > Feel free to let me know what you think.
> 
> Could you point me where this CL is different from what you landed (and got
> reverted) previously? I just had a cursory look (I'm not supposed to work
today
> ;-)) and fail to find a difference.
> 
> Thanks

The difference is in detectTextEncoding().

Previously:

 411 void TextResourceDecoder::detectTextEncoding(const char* data, size_t len)
 412 {
 413     if (!shouldDetectEncoding())
 414         return;
 415 
 416     if (WTF::Unicode::isUTF8andNotASCII(data, len)) {
 417         setEncoding(UTF8Encoding(), EncodingFromContentSniffing);
 418         return;
 419     }
 420     if (m_encodingDetectionOption == UseAllAutoDetection) {
 421         WTF::TextEncoding detectedEncoding;
 422         if (detectTextEncodingUniversal(data, len, m_hintEncoding,
&detectedEncoding))
 423             setEncoding(detectedEncoding, EncodingFromContentSniffing);
 424     }
 425 }
 426 

Now:

 411 //   We also check if the text is encoded in UTF-8 in case the encoding has
not
 412 //   been determined by auto encoding detector (optional). Then |m_source|
needs
 413 //   to be set to anything but DefaultEncoding to avoid further detection
 414 //   attempts.
 415 void TextResourceDecoder::detectTextEncoding(const char* data, size_t len)
 416 {
 417     if (!shouldDetectEncoding())
 418         return;
 419 
 420     if (m_encodingDetectionOption == UseAllAutoDetection) {
 421         WTF::TextEncoding detectedEncoding;
 422         if (detectTextEncodingUniversal(data, len, m_hintEncoding,
&detectedEncoding)) {
 423             setEncoding(detectedEncoding, EncodingFromContentSniffing);
 424             return;
 425         }
 426     }
 427 
 428     if (WTF::Unicode::isUTF8andNotASCII(data, len))
 429         setEncoding(UTF8Encoding(), EncodingFromContentSniffing);
 430     else
 431         m_source = EncodingFromContentSniffing;
 432 }


Thanks for taking a look while OOO...

[Jinsuk Kim, 2016-04-15 19:31:05]

Description was changed from

==========
Reland "UTF-8 detector for pages missing encoding info"

TextResourceDecoder is designed (or used) in such a way that the text
encoding of a document gets resolved from the first chunk (as big as
4096 bytes) of the text received from network - by BOM, meta tag, or
auto encoding detection (if enabled).

The newly introduced UTF-8 encoding detector crrev.com/1721373002 was
reverted (crbug.com/603558) because it attempted to work in a bit
different way - it examines all the subsequent chunks as well in search
of non-ASCII-UTF-8-encoded char sequence. This means it is possible for
TextResourceDecoder to start with a codec for, say, windows-1252, and
then later switch to one for UTF-8. Theoretically this should still work
but doesn't in practice (maybe hasn't been used/tested in that
way). This is what happened with failed perf tests - one of the js files
was big (13K), of pure ASCII except one tiny char sequence \xc2\xa7
almost at the end.

The CL was updatd for the UTF-8 encoding detection also works against
the first chunk only like other methods, to avoid potential codec
switching in the middle.

TBR=tkent@chromium.org,aelias@chromium.org,jshin@chromium.org
BUG=583549,603558
==========

to

==========
Reland "UTF-8 detector for pages missing encoding info"

TextResourceDecoder is designed (or used) in such a way that the text
encoding of a document gets resolved from the first chunk (as big as
4096 bytes) of the text received from network - by BOM, meta tag, or
auto encoding detection (if enabled).

The newly introduced UTF-8 encoding detector crrev.com/1721373002 was
reverted (crbug.com/603558) because it attempted to work in a bit
different way - it examined all the subsequent chunks as well in search
of non-ASCII-UTF-8-encoded char sequence. This means it is possible for
TextResourceDecoder to start with a codec for, say, windows-1252, and
then later switch to one for UTF-8. Theoretically this should still work
but doesn't in practice (maybe hasn't been used/tested in that
way). This is what happened with failed perf tests - one of the js files
was big (13K), of pure ASCII except one tiny char sequence \xc2\xa7
almost at the end.

The CL was updated so the UTF-8 encoding detection also works against
the first chunk only like other methods, to avoid potential codec
switching in the middle.

TBR=tkent@chromium.org,aelias@chromium.org,jshin@chromium.org
BUG=583549,603558
==========

[Jinsuk Kim, 2016-04-18 23:39:45]

Description was changed from

==========
Reland "UTF-8 detector for pages missing encoding info"

TextResourceDecoder is designed (or used) in such a way that the text
encoding of a document gets resolved from the first chunk (as big as
4096 bytes) of the text received from network - by BOM, meta tag, or
auto encoding detection (if enabled).

The newly introduced UTF-8 encoding detector crrev.com/1721373002 was
reverted (crbug.com/603558) because it attempted to work in a bit
different way - it examined all the subsequent chunks as well in search
of non-ASCII-UTF-8-encoded char sequence. This means it is possible for
TextResourceDecoder to start with a codec for, say, windows-1252, and
then later switch to one for UTF-8. Theoretically this should still work
but doesn't in practice (maybe hasn't been used/tested in that
way). This is what happened with failed perf tests - one of the js files
was big (13K), of pure ASCII except one tiny char sequence \xc2\xa7
almost at the end.

The CL was updated so the UTF-8 encoding detection also works against
the first chunk only like other methods, to avoid potential codec
switching in the middle.

TBR=tkent@chromium.org,aelias@chromium.org,jshin@chromium.org
BUG=583549,603558
==========

to

==========
Reland "UTF-8 detector for pages missing encoding info"

TextResourceDecoder is designed (or used) in such a way that the text
encoding of a document gets resolved from the first chunk (as big as
4096 bytes) of the text received from network - by BOM, meta tag, or
auto encoding detection (if enabled).

The newly introduced UTF-8 encoding detector crrev.com/1721373002 was
reverted (crbug.com/603558) because it attempted to work in a bit
different way - it examined all the subsequent chunks as well in search
of non-ASCII-UTF-8-encoded char sequence. This means it is possible for
TextResourceDecoder to start with a codec for, say, windows-1252, and
then later switch to one for UTF-8. Theoretically this should still work
but doesn't in practice (maybe hasn't been used/tested in that
way). This is what happened with failed perf tests - one of the js files
was big (13K), of pure ASCII except one tiny char sequence \xc2\xa7
almost at the end.

The CL was updated so the UTF-8 encoding detection also works against
the first chunk only like other methods, to avoid potential codec
switching in the middle.

BUG=583549,603558
==========

[Jinsuk Kim, 2016-04-19 22:59:28]

jshin@, tkent@: could you a look? This is a relanding CL that was reverted.  And
tkent@, I need your owner's approval.

Thanks,

[tkent, 2016-04-20 00:07:49]

This CL should have a test for crbug.com/603558.

[Jinsuk Kim, 2016-04-20 02:08:39]

On 2016/04/20 00:07:49, tkent wrote:
> This CL should have a test for crbug.com/603558.

Done. I meant to add tests in a follow up CL for the new approach. Please note
that the new test will replace the one added here, verifying the codec switching
works as expected.

[tkent, 2016-04-20 06:37:47]

lgtm

https://codereview.chromium.org/1890103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/TextResourceDecoderTest.cpp
(right):

https://codereview.chromium.org/1890103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/TextResourceDecoderTest.cpp:24:
ASSERT_EQ(UTF8Encoding(), decoder->encoding());
probably this should be EXPECT_EQ because the test can continue even if this
fails.

https://codereview.chromium.org/1890103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/TextResourceDecoderTest.cpp:36:
ASSERT_EQ(Latin1Encoding(), decoder->encoding());
Ditto.

[Jinsuk Kim, 2016-04-20 07:13:55]

Thanks again Kent for the review.

https://codereview.chromium.org/1890103002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/TextResourceDecoderTest.cpp
(right):

https://codereview.chromium.org/1890103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/TextResourceDecoderTest.cpp:24:
ASSERT_EQ(UTF8Encoding(), decoder->encoding());
On 2016/04/20 06:37:46, tkent wrote:
> probably this should be EXPECT_EQ because the test can continue even if this
> fails.

Done.

https://codereview.chromium.org/1890103002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/TextResourceDecoderTest.cpp:36:
ASSERT_EQ(Latin1Encoding(), decoder->encoding());
On 2016/04/20 06:37:46, tkent wrote:
> Ditto.

Done.

[jungshik at Google, 2016-04-21 18:31:49]

LGTM ! Thank you for your patience.

[Jinsuk Kim, 2016-04-21 20:57:49]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-04-21 20:57:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1890103002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-04-21 20:58:30]

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

[commit-bot: I haz the power, 2016-04-21 22:33:34]

Description was changed from

==========
Reland "UTF-8 detector for pages missing encoding info"

TextResourceDecoder is designed (or used) in such a way that the text
encoding of a document gets resolved from the first chunk (as big as
4096 bytes) of the text received from network - by BOM, meta tag, or
auto encoding detection (if enabled).

The newly introduced UTF-8 encoding detector crrev.com/1721373002 was
reverted (crbug.com/603558) because it attempted to work in a bit
different way - it examined all the subsequent chunks as well in search
of non-ASCII-UTF-8-encoded char sequence. This means it is possible for
TextResourceDecoder to start with a codec for, say, windows-1252, and
then later switch to one for UTF-8. Theoretically this should still work
but doesn't in practice (maybe hasn't been used/tested in that
way). This is what happened with failed perf tests - one of the js files
was big (13K), of pure ASCII except one tiny char sequence \xc2\xa7
almost at the end.

The CL was updated so the UTF-8 encoding detection also works against
the first chunk only like other methods, to avoid potential codec
switching in the middle.

BUG=583549,603558
==========

to

==========
Reland "UTF-8 detector for pages missing encoding info"

TextResourceDecoder is designed (or used) in such a way that the text
encoding of a document gets resolved from the first chunk (as big as
4096 bytes) of the text received from network - by BOM, meta tag, or
auto encoding detection (if enabled).

The newly introduced UTF-8 encoding detector crrev.com/1721373002 was
reverted (crbug.com/603558) because it attempted to work in a bit
different way - it examined all the subsequent chunks as well in search
of non-ASCII-UTF-8-encoded char sequence. This means it is possible for
TextResourceDecoder to start with a codec for, say, windows-1252, and
then later switch to one for UTF-8. Theoretically this should still work
but doesn't in practice (maybe hasn't been used/tested in that
way). This is what happened with failed perf tests - one of the js files
was big (13K), of pure ASCII except one tiny char sequence \xc2\xa7
almost at the end.

The CL was updated so the UTF-8 encoding detection also works against
the first chunk only like other methods, to avoid potential codec
switching in the middle.

BUG=583549,603558
==========

[commit-bot: I haz the power, 2016-04-21 22:33:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:40:37]

Description was changed from

==========
Reland "UTF-8 detector for pages missing encoding info"

TextResourceDecoder is designed (or used) in such a way that the text
encoding of a document gets resolved from the first chunk (as big as
4096 bytes) of the text received from network - by BOM, meta tag, or
auto encoding detection (if enabled).

The newly introduced UTF-8 encoding detector crrev.com/1721373002 was
reverted (crbug.com/603558) because it attempted to work in a bit
different way - it examined all the subsequent chunks as well in search
of non-ASCII-UTF-8-encoded char sequence. This means it is possible for
TextResourceDecoder to start with a codec for, say, windows-1252, and
then later switch to one for UTF-8. Theoretically this should still work
but doesn't in practice (maybe hasn't been used/tested in that
way). This is what happened with failed perf tests - one of the js files
was big (13K), of pure ASCII except one tiny char sequence \xc2\xa7
almost at the end.

The CL was updated so the UTF-8 encoding detection also works against
the first chunk only like other methods, to avoid potential codec
switching in the middle.

BUG=583549,603558
==========

to

==========
Reland "UTF-8 detector for pages missing encoding info"

TextResourceDecoder is designed (or used) in such a way that the text
encoding of a document gets resolved from the first chunk (as big as
4096 bytes) of the text received from network - by BOM, meta tag, or
auto encoding detection (if enabled).

The newly introduced UTF-8 encoding detector crrev.com/1721373002 was
reverted (crbug.com/603558) because it attempted to work in a bit
different way - it examined all the subsequent chunks as well in search
of non-ASCII-UTF-8-encoded char sequence. This means it is possible for
TextResourceDecoder to start with a codec for, say, windows-1252, and
then later switch to one for UTF-8. Theoretically this should still work
but doesn't in practice (maybe hasn't been used/tested in that
way). This is what happened with failed perf tests - one of the js files
was big (13K), of pure ASCII except one tiny char sequence \xc2\xa7
almost at the end.

The CL was updated so the UTF-8 encoding detection also works against
the first chunk only like other methods, to avoid potential codec
switching in the middle.

BUG=583549,603558

Committed: https://crrev.com/57139d64c5b98142ca9305792f39ae23a4950375
Cr-Commit-Position: refs/heads/master@{#388927}
==========

[commit-bot: I haz the power, 2016-04-22 19:40:38]

Patchset 3 (id:??) landed as
https://crrev.com/57139d64c5b98142ca9305792f39ae23a4950375
Cr-Commit-Position: refs/heads/master@{#388927}