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

third_party/WebKit/Source/wtf/text/TextCodecICU.cpp

Index: third_party/WebKit/Source/wtf/text/TextCodecICU.cpp
diff --git a/third_party/WebKit/Source/wtf/text/TextCodecICU.cpp b/third_party/WebKit/Source/wtf/text/TextCodecICU.cpp
index b2c4d3f58a17c8be4cd1af6af838618de3809e26..4adf5eacbee7aced8cdeb99b16ecc04e4370d2fa 100644
--- a/third_party/WebKit/Source/wtf/text/TextCodecICU.cpp
+++ b/third_party/WebKit/Source/wtf/text/TextCodecICU.cpp
@@ -311,25 +311,22 @@ public:
         : m_converter(converter)
         , m_shouldStopOnEncodingErrors(stopOnError)
     {
-        if (m_shouldStopOnEncodingErrors) {
-            UErrorCode err = U_ZERO_ERROR;
-            ucnv_setToUCallBack(m_converter, UCNV_TO_U_CALLBACK_SUBSTITUTE,
-                UCNV_SUB_STOP_ON_ILLEGAL, &m_savedAction,
-                &m_savedContext, &err);
-            ASSERT(err == U_ZERO_ERROR);
-        }
+        UErrorCode err = U_ZERO_ERROR;
+        if (m_shouldStopOnEncodingErrors)
+            ucnv_setToUCallBack(m_converter, UCNV_TO_U_CALLBACK_STOP, 0, &m_savedAction, &m_savedContext, &err);
+        else
+            ucnv_setToUCallBack(m_converter, UCNV_TO_U_CALLBACK_SUBSTITUTE, 0, &m_savedAction, &m_savedContext, &err);

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

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.

[jungshik at Google, 2016/08/12 07:20:41]

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. 

+        DCHECK_EQ(err, U_ZERO_ERROR);
     }
     ~ErrorCallbackSetter()
     {
-        if (m_shouldStopOnEncodingErrors) {
-            UErrorCode err = U_ZERO_ERROR;
-            const void* oldContext;
-            UConverterToUCallback oldAction;
-            ucnv_setToUCallBack(m_converter, m_savedAction, m_savedContext, &oldAction, &oldContext, &err);
-            ASSERT(oldAction == UCNV_TO_U_CALLBACK_SUBSTITUTE);
-            ASSERT(!strcmp(static_cast<const char*>(oldContext), UCNV_SUB_STOP_ON_ILLEGAL));
-            ASSERT(err == U_ZERO_ERROR);
-        }
+        UErrorCode err = U_ZERO_ERROR;
+        const void* oldContext;
+        UConverterToUCallback oldAction;
+        ucnv_setToUCallBack(m_converter, m_savedAction, m_savedContext, &oldAction, &oldContext, &err);
+        DCHECK_EQ(oldAction, m_shouldStopOnEncodingErrors ? UCNV_TO_U_CALLBACK_STOP : UCNV_TO_U_CALLBACK_SUBSTITUTE);
+        DCHECK(!oldContext);
+        DCHECK_EQ(err, U_ZERO_ERROR);
     }
 
 private: