Text Encoding: Fix "stop on error" for ICU-based codecs

Text Encoding: Fix "stop on error" for ICU-based codecs

Handling of errors when the "stopOnError" flag was set in
TextCodecICU::decode() was incorrect; the replacement character was
always emitted instead. Align the initialization of the ICU error
callback with what i18n/icu_string_conversions.cc does.

BUG=607540
R=jshin@chromium.org

Committed: https://crrev.com/005842b420cace28845fc552ef6da8086fe527a5
Cr-Commit-Position: refs/heads/master@{#412170}

[jsbell, 2016-08-09 22:24:17]

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

[commit-bot: I haz the power, 2016-08-09 22:25:06]

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

[jsbell, 2016-08-09 22:27:24]

jshin@ please take a look?

https://codereview.chromium.org/2235453002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/TextCodecICU.cpp (right):

https://codereview.chromium.org/2235453002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/TextCodecICU.cpp:318:
ucnv_setToUCallBack(m_converter, UCNV_TO_U_CALLBACK_SUBSTITUTE, 0,
&m_savedAction, &m_savedContext, &err);
Maybe we only want the `if (m_shouldStopOnEncodingErrors)` branch, and do
nothing if !m_shouldStopOnEncodingErrors 

The ICU docs are terrible for the combination of UCNV_TO_U_CALLBACK_SUBSTITUTE /
UCNV_SUB_STOP_ON_ILLEGAL - it's unclear to me what that even means, even though
it's called out explicitly as a valid combination.

[commit-bot: I haz the power, 2016-08-10 00:19:19]

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

[commit-bot: I haz the power, 2016-08-10 00:19:20]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-08-11 16:47:45]

jshin@ - ping!

[jungshik at Google, 2016-08-12 07:20:42]

LGTM

https://codereview.chromium.org/2235453002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/TextCodecICU.cpp (right):

https://codereview.chromium.org/2235453002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/TextCodecICU.cpp:318:
ucnv_setToUCallBack(m_converter, UCNV_TO_U_CALLBACK_SUBSTITUTE, 0,
&m_savedAction, &m_savedContext, &err);
On 2016/08/09 22:27:24, jsbell wrote:
> Maybe we only want the `if (m_shouldStopOnEncodingErrors)` branch, and do
> nothing if !m_shouldStopOnEncodingErrors 
> 
> The ICU docs are terrible for the combination of UCNV_TO_U_CALLBACK_SUBSTITUTE
/
> UCNV_SUB_STOP_ON_ILLEGAL - it's unclear to me what that even means, even
though
> it's called out explicitly as a valid combination.

Indeed, confusing. 

https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/uc...

helps. 

For 'ToUnicode' direction, 
UCNV_SUB_STOP_ON_ILLEGAL means  that the conversion should stop right away when
coming across a byte/byte sequence NOT allowed by the current charset (that is,
*illegal*  or outside the range).  However, 'valid but unassigned' byte
sequences will STILL be substituted per UCNV_TO_U_CALLBACK_SUBSTITUTE,. 

Depending on how tightly valid byte-ranges are defined, there may or may not be 
byte sequences falling into the second group (valid but unassigned)> .

Because the encoding spec does not make a distinction between the former and the
latter, I think we don't need 'else' branch.

[jsbell, 2016-08-12 17:53:09]

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

[commit-bot: I haz the power, 2016-08-12 17:53:49]

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

[jsbell, 2016-08-12 18:05:50]

jshin@ - updated the CL to not have the "else" paths. Can you do one more
review?

[commit-bot: I haz the power, 2016-08-12 18:15:43]

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

[commit-bot: I haz the power, 2016-08-12 18:15:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jungshik at Google, 2016-08-16 00:10:10]

On 2016/08/12 18:05:50, jsbell wrote:
> jshin@ - updated the CL to not have the "else" paths. Can you do one more
> review?

LGTM

[jsbell, 2016-08-16 01:59:22]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-08-16 01:59:55]

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, 2016-08-16 02:06:28]

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

[commit-bot: I haz the power, 2016-08-16 02:06:29]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jsbell, 2016-08-16 02:25:32]

jsbell@chromium.org changed reviewers:
+ haraken@chromium.org

[jsbell, 2016-08-16 02:25:33]

haraken@ - can you review as wtf OWNER ? (and cq if lg?)

[haraken, 2016-08-16 02:37:35]

LGTM

[haraken, 2016-08-16 02:37:40]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-08-16 02:38:05]

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, 2016-08-16 05:11:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-16 05:12:45]

Description was changed from

==========
Text Encoding: Fix "stop on error" for ICU-based codecs

Handling of errors when the "stopOnError" flag was set in
TextCodecICU::decode() was incorrect; the replacement character was
always emitted instead. Align the initialization of the ICU error
callback with what i18n/icu_string_conversions.cc does.

BUG=607540
R=jshin@chromium.org
==========

to

==========
Text Encoding: Fix "stop on error" for ICU-based codecs

Handling of errors when the "stopOnError" flag was set in
TextCodecICU::decode() was incorrect; the replacement character was
always emitted instead. Align the initialization of the ICU error
callback with what i18n/icu_string_conversions.cc does.

BUG=607540
R=jshin@chromium.org

Committed: https://crrev.com/005842b420cace28845fc552ef6da8086fe527a5
Cr-Commit-Position: refs/heads/master@{#412170}
==========

[commit-bot: I haz the power, 2016-08-16 05:12:46]

Patchset 2 (id:??) landed as
https://crrev.com/005842b420cace28845fc552ef6da8086fe527a5
Cr-Commit-Position: refs/heads/master@{#412170}