[Fetch] Always use utf-8 for decoding in text()

[Fetch] Always use utf-8 for decoding in text()

In the spec, Response.text() should run utf-8 decode [1][2], i.e. always use
utf-8. But in the implementation, TextResourceDecoder used by Response.text()
decodes the input as utf-16/32 if it contains utf-16/32 BOMs.

This CL
- Adds TextResourceDecoder::createAlwaysUseUTF8ForText() and
  AlwaysUseUTF8ForText that corresponds to utf-8 decode in the spec [2].
- Fixes Response.text() by using CheckForOnlyUTF8BOM in FetchDataLoaderAsString.
- Adds layout tests for Fetch API and XHRs
  (but doesn't change the behavior of XHRs).

[1] https://fetch.spec.whatwg.org/#concept-body-package-data
[2] https://encoding.spec.whatwg.org/#utf-8-decode

BUG=557510
Committed: https://crrev.com/2198aba1db661a195eb6f191274c1595c3bc6867
Cr-Commit-Position: refs/heads/master@{#364978}

[hiroshige, 2015-11-23 11:25:33]

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

[commit-bot: I haz the power, 2015-11-23 11:25:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470893002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470893002/1

[commit-bot: I haz the power, 2015-11-23 12:25:34]

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

[commit-bot: I haz the power, 2015-11-23 12:25:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2015-11-23 12:42:15]

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

[commit-bot: I haz the power, 2015-11-23 12:42:27]

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

[hiroshige, 2015-11-23 13:19:28]

Description was changed from

==========
[Fetch] Always use utf-8 for decoding in text()

BUG=557510
==========

to

==========
[Fetch] Always use utf-8 for decoding in text()

In the spec, Response.text() should run utf-8 decode [1][2], i.e. always use
utf-8. But in the implementation, TextResourceDecoder used by Response.text()
decodes the input as utf-16/32 if it contains utf-16/32 BOMs.

This CL
- Adds TextResourceDecoder::CheckForOnlyUTF8BOM option, which does not
  check for utf-16/32 BOMs but checks utf-8 BOMs, so that
    TextResourceDecoder::create("text/plain", UTF8Encoding(), false,
                                CheckForOnlyUTF8BOM)
  corresponds to utf-8 decode in the spec [2],
- Fixes Response.text() by using CheckForOnlyUTF8BOM in FetchDataLoaderAsString.
- Adds layout tests for Fetch API and XHRs
  (but doesn't change the behavior of XHRs).

[1] https://fetch.spec.whatwg.org/#concept-body-package-data
[2] https://encoding.spec.whatwg.org/#utf-8-decode

BUG=557510
==========

[hiroshige, 2015-11-23 13:19:43]

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

[commit-bot: I haz the power, 2015-11-23 13:19:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470893002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470893002/60001

[hiroshige, 2015-11-23 13:29:34]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2015-11-23 14:31:36]

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

[commit-bot: I haz the power, 2015-11-23 14:32:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470893002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470893002/100001

[hiroshige, 2015-11-23 14:34:03]

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

[commit-bot: I haz the power, 2015-11-23 14:34:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470893002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470893002/120001

[commit-bot: I haz the power, 2015-11-23 15:40:54]

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

[commit-bot: I haz the power, 2015-11-23 15:40:54]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-11-23 18:33:39]

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

[hiroshige, 2015-11-23 18:33:39]

PTAL.

[yhirano, 2015-11-24 02:06:40]

https://codereview.chromium.org/1470893002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp (right):

https://codereview.chromium.org/1470893002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp:197: // We
let it override even a user-chosen encoding.
This is the problem, right?
Rather than introducing a new parameter, I think it is good to expand
|usesEncodingDetector| constructor parameter so that a user can choose one of

 - UseAutoDetection (<= usesEncodingDetector == true)
 - UseBOMBasedDetectionOnly  (<= usesEncodingDetector == false, default value)
 - DoNotUseAutoDetection (<= Response.text())

. What do you think?

[hiroshige, 2015-11-30 08:50:12]

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

[commit-bot: I haz the power, 2015-11-30 08:50:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470893002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470893002/130001

[hiroshige, 2015-11-30 09:50:01]

https://codereview.chromium.org/1470893002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp (right):

https://codereview.chromium.org/1470893002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp:197: // We
let it override even a user-chosen encoding.
How about Patch Set 8:
  |usesEncodingDetector| can be:
  - UseAllAutoDetection (<= usesEncodingDetector == true)
  - UseContentAndBOMBasedDetection  (<= usesEncodingDetector == false, default
value)
  - AlwaysUseUTF8ForText (<= Response.text())
  and when it is AlwaysUseUTF8ForText, |defaultEncoding| should be utf-8, and
|mimeType| should be "text/plain".
This is not orthogonal, but implementing DoNotUseAutoDetection for all encodings
and non-text contents complicates the code:
- Even when we don't detect encoding, we should handle utf-8 BOMs in
AlwaysUseUTF8ForText, i.e. we should skip utf-8 BOMs (but not other BOMs such as
utf-16be BOMs) when we decode the content as utf-8.
  Supporting this BOM skipping for utf-16/32 makes code more complicated.
- In non-text contents (i.e. CSS/XML/HTML), TextResourceDecoder handles
encodings explicitly specified in those files (e.g. <meta> tag) and its
detection logic is built into other logics in the decoder.
  Disabling such logic makes code and state management of TextResourceDecoder
more complicated.
- And anyway we don't have such use cases.

[commit-bot: I haz the power, 2015-11-30 10:04:22]

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

[commit-bot: I haz the power, 2015-11-30 10:04:23]

Dry run: This issue passed the CQ dry run.

[yhirano, 2015-12-02 08:28:30]

lgtm

https://codereview.chromium.org/1470893002/diff/130001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp (right):

https://codereview.chromium.org/1470893002/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp:214: if
(m_encodingDetectionOption <= UseContentAndBOMBasedDetection && c1 == 0xFF && c2
== 0xFE) {
[optional] I'm not a fan of numerical enum value comparison though I cannot find
a rule forbidding it.

[hiroshige, 2015-12-03 07:24:44]

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

[commit-bot: I haz the power, 2015-12-03 07:25:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470893002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470893002/150001

[commit-bot: I haz the power, 2015-12-03 08:36:48]

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

[commit-bot: I haz the power, 2015-12-03 08:36:49]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-12-09 21:33:30]

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

[hiroshige, 2015-12-09 21:36:29]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2015-12-10 06:19:47]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470893002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470893002/150001

[commit-bot: I haz the power, 2015-12-10 08:38:21]

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

[commit-bot: I haz the power, 2015-12-10 08:38:22]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2015-12-11 04:57:11]

hiroshige@chromium.org changed reviewers:
+ kouhei@chromium.org

[hiroshige, 2015-12-11 04:57:11]

kouhei@, could you take a look as a core/html/parser OWNER?

[kouhei (in TOK), 2015-12-14 02:00:22]

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp (right):

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp:128:
ASSERT(!(m_encodingDetectionOption == AlwaysUseUTF8ForText && (m_contentType !=
PlainTextContent || m_encoding != UTF8Encoding())));
Optional nit: Would you split this ASSERT stmt?
if (m_encodingDetectionOption == AlwaysUseUTF8ForText)
    ASSERT(m_contentType != PlainTextContent || m_encoding != UTF8Encoding());

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp:214: if
(m_encodingDetectionOption != AlwaysUseUTF8ForText && c1 == 0xFF && c2 == 0xFE)
{
Can we reorder the if stmts here?

if (c1 == 0xEF && c2 == 0xBB && c3 == 0xBF) {

} else if (m_encodingDetectionOption != AlwaysUseUTF8ForText) {
  if (c1 == 0xFF && c2 == 0xFE) {

  } else if (c1 == 0xFE && c2 == 0xFF) {

  } else if (...) {

  }
}

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.h (right):

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/TextResourceDecoder.h:49: static
PassOwnPtr<TextResourceDecoder> create(const String& mimeType, const
WTF::TextEncoding& defaultEncoding = WTF::TextEncoding(), bool
usesEncodingDetector = false)
Most of the create() methods use exactly same params as c-tors.
Would you change "bool usesEncodingDetector" to EncodingDecodingOption?

[hiroshige, 2015-12-14 05:38:44]

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

[commit-bot: I haz the power, 2015-12-14 05:38:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1470893002/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1470893002/170001

[hiroshige, 2015-12-14 05:43:37]

Description was changed from

==========
[Fetch] Always use utf-8 for decoding in text()

In the spec, Response.text() should run utf-8 decode [1][2], i.e. always use
utf-8. But in the implementation, TextResourceDecoder used by Response.text()
decodes the input as utf-16/32 if it contains utf-16/32 BOMs.

This CL
- Adds TextResourceDecoder::CheckForOnlyUTF8BOM option, which does not
  check for utf-16/32 BOMs but checks utf-8 BOMs, so that
    TextResourceDecoder::create("text/plain", UTF8Encoding(), false,
                                CheckForOnlyUTF8BOM)
  corresponds to utf-8 decode in the spec [2],
- Fixes Response.text() by using CheckForOnlyUTF8BOM in FetchDataLoaderAsString.
- Adds layout tests for Fetch API and XHRs
  (but doesn't change the behavior of XHRs).

[1] https://fetch.spec.whatwg.org/#concept-body-package-data
[2] https://encoding.spec.whatwg.org/#utf-8-decode

BUG=557510
==========

to

==========
[Fetch] Always use utf-8 for decoding in text()

In the spec, Response.text() should run utf-8 decode [1][2], i.e. always use
utf-8. But in the implementation, TextResourceDecoder used by Response.text()
decodes the input as utf-16/32 if it contains utf-16/32 BOMs.

This CL
- Adds TextResourceDecoder::createAlwaysUseUTF8ForText() and
  AlwaysUseUTF8ForText that corresponds to utf-8 decode in the spec [2].
- Fixes Response.text() by using CheckForOnlyUTF8BOM in FetchDataLoaderAsString.
- Adds layout tests for Fetch API and XHRs
  (but doesn't change the behavior of XHRs).

[1] https://fetch.spec.whatwg.org/#concept-body-package-data
[2] https://encoding.spec.whatwg.org/#utf-8-decode

BUG=557510
==========

[hiroshige, 2015-12-14 05:44:03]

Also updated CL description.

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp (right):

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp:128:
ASSERT(!(m_encodingDetectionOption == AlwaysUseUTF8ForText && (m_contentType !=
PlainTextContent || m_encoding != UTF8Encoding())));
On 2015/12/14 02:00:22, kouhei wrote:
> Optional nit: Would you split this ASSERT stmt?
> if (m_encodingDetectionOption == AlwaysUseUTF8ForText)
>     ASSERT(m_contentType != PlainTextContent || m_encoding != UTF8Encoding());

Done.

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp:214: if
(m_encodingDetectionOption != AlwaysUseUTF8ForText && c1 == 0xFF && c2 == 0xFE)
{
On 2015/12/14 02:00:22, kouhei wrote:
> Can we reorder the if stmts here?
> 
> if (c1 == 0xEF && c2 == 0xBB && c3 == 0xBF) {
> 
> } else if (m_encodingDetectionOption != AlwaysUseUTF8ForText) {
>   if (c1 == 0xFF && c2 == 0xFE) {
> 
>   } else if (c1 == 0xFE && c2 == 0xFF) {
> 
>   } else if (...) {
> 
>   }
> }

Done.

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.h (right):

https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/TextResourceDecoder.h:49: static
PassOwnPtr<TextResourceDecoder> create(const String& mimeType, const
WTF::TextEncoding& defaultEncoding = WTF::TextEncoding(), bool
usesEncodingDetector = false)
On 2015/12/14 02:00:22, kouhei wrote:
> Most of the create() methods use exactly same params as c-tors.
> Would you change "bool usesEncodingDetector" to EncodingDecodingOption?

I'd like to enforce mimeType == "plain/text" && defaultEncoding ==
UTF8Enconding() if EncodingDecodingOption is AlwaysUseUTF8ForText, so
AlwaysUseUTF8ForText shouldn't specified from create().
Changing the last param to EncodingDetectionOption and placing RELEASE_ASSERT or
ASSERT(encodingDetectionOption != AlwaysUseUTF8ForText) would also work (but I
slightly prefer the current Patch Set).
How do you think?

[kouhei (in TOK), 2015-12-14 05:47:45]

lgtm

>
https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.h (right):
> 
>
https://codereview.chromium.org/1470893002/diff/150001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/parser/TextResourceDecoder.h:49: static
> PassOwnPtr<TextResourceDecoder> create(const String& mimeType, const
> WTF::TextEncoding& defaultEncoding = WTF::TextEncoding(), bool
> usesEncodingDetector = false)
> On 2015/12/14 02:00:22, kouhei wrote:
> > Most of the create() methods use exactly same params as c-tors.
> > Would you change "bool usesEncodingDetector" to EncodingDecodingOption?
> 
> I'd like to enforce mimeType == "plain/text" && defaultEncoding ==
> UTF8Enconding() if EncodingDecodingOption is AlwaysUseUTF8ForText, so
> AlwaysUseUTF8ForText shouldn't specified from create().
> Changing the last param to EncodingDetectionOption and placing RELEASE_ASSERT
or
> ASSERT(encodingDetectionOption != AlwaysUseUTF8ForText) would also work (but I
> slightly prefer the current Patch Set).
> How do you think?

ok

[hiroshige, 2015-12-14 06:35:13]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2015-12-14 06:35:18]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2015-12-14 06:35:18]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1470893002/#ps170001
(title: "Reflect comments.")

[commit-bot: I haz the power, 2015-12-14 06:35:26]

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

[commit-bot: I haz the power, 2015-12-14 07:17:36]

Description was changed from

==========
[Fetch] Always use utf-8 for decoding in text()

In the spec, Response.text() should run utf-8 decode [1][2], i.e. always use
utf-8. But in the implementation, TextResourceDecoder used by Response.text()
decodes the input as utf-16/32 if it contains utf-16/32 BOMs.

This CL
- Adds TextResourceDecoder::createAlwaysUseUTF8ForText() and
  AlwaysUseUTF8ForText that corresponds to utf-8 decode in the spec [2].
- Fixes Response.text() by using CheckForOnlyUTF8BOM in FetchDataLoaderAsString.
- Adds layout tests for Fetch API and XHRs
  (but doesn't change the behavior of XHRs).

[1] https://fetch.spec.whatwg.org/#concept-body-package-data
[2] https://encoding.spec.whatwg.org/#utf-8-decode

BUG=557510
==========

to

==========
[Fetch] Always use utf-8 for decoding in text()

In the spec, Response.text() should run utf-8 decode [1][2], i.e. always use
utf-8. But in the implementation, TextResourceDecoder used by Response.text()
decodes the input as utf-16/32 if it contains utf-16/32 BOMs.

This CL
- Adds TextResourceDecoder::createAlwaysUseUTF8ForText() and
  AlwaysUseUTF8ForText that corresponds to utf-8 decode in the spec [2].
- Fixes Response.text() by using CheckForOnlyUTF8BOM in FetchDataLoaderAsString.
- Adds layout tests for Fetch API and XHRs
  (but doesn't change the behavior of XHRs).

[1] https://fetch.spec.whatwg.org/#concept-body-package-data
[2] https://encoding.spec.whatwg.org/#utf-8-decode

BUG=557510
==========

[commit-bot: I haz the power, 2015-12-14 07:17:36]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-12-14 07:18:22]

Description was changed from

==========
[Fetch] Always use utf-8 for decoding in text()

In the spec, Response.text() should run utf-8 decode [1][2], i.e. always use
utf-8. But in the implementation, TextResourceDecoder used by Response.text()
decodes the input as utf-16/32 if it contains utf-16/32 BOMs.

This CL
- Adds TextResourceDecoder::createAlwaysUseUTF8ForText() and
  AlwaysUseUTF8ForText that corresponds to utf-8 decode in the spec [2].
- Fixes Response.text() by using CheckForOnlyUTF8BOM in FetchDataLoaderAsString.
- Adds layout tests for Fetch API and XHRs
  (but doesn't change the behavior of XHRs).

[1] https://fetch.spec.whatwg.org/#concept-body-package-data
[2] https://encoding.spec.whatwg.org/#utf-8-decode

BUG=557510
==========

to

==========
[Fetch] Always use utf-8 for decoding in text()

In the spec, Response.text() should run utf-8 decode [1][2], i.e. always use
utf-8. But in the implementation, TextResourceDecoder used by Response.text()
decodes the input as utf-16/32 if it contains utf-16/32 BOMs.

This CL
- Adds TextResourceDecoder::createAlwaysUseUTF8ForText() and
  AlwaysUseUTF8ForText that corresponds to utf-8 decode in the spec [2].
- Fixes Response.text() by using CheckForOnlyUTF8BOM in FetchDataLoaderAsString.
- Adds layout tests for Fetch API and XHRs
  (but doesn't change the behavior of XHRs).

[1] https://fetch.spec.whatwg.org/#concept-body-package-data
[2] https://encoding.spec.whatwg.org/#utf-8-decode

BUG=557510

Committed: https://crrev.com/2198aba1db661a195eb6f191274c1595c3bc6867
Cr-Commit-Position: refs/heads/master@{#364978}
==========

[commit-bot: I haz the power, 2015-12-14 07:18:23]

Patchset 10 (id:??) landed as
https://crrev.com/2198aba1db661a195eb6f191274c1595c3bc6867
Cr-Commit-Position: refs/heads/master@{#364978}