[webkit-reviews] review canceled: [Bug 192945] Use unorm2_normalize instead of precomposedStringWithCanonicalMapping in userVisibleString : [Attachment 357865] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Dec 20 19:47:11 PST 2018

Michael Catanzaro <mcatanzaro at igalia.com> has canceled Michael Catanzaro
<mcatanzaro at igalia.com>'s request for review:
Bug 192945: Use unorm2_normalize instead of
precomposedStringWithCanonicalMapping in userVisibleString

Attachment 357865: Patch


--- Comment #10 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 357865
  --> https://bugs.webkit.org/attachment.cgi?id=357865

View in context: https://bugs.webkit.org/attachment.cgi?id=357865&action=review

>> Source/WTF/wtf/cocoa/NSURLExtras.mm:1170
>> +	ASSERT(sourceBuffer.last() == '\0');
> This assertion seems redundant.

It is, but it makes the next line sourceBuffer.removeLast() easier to
understand, so seems worth keeping.

>> Source/WTF/wtf/cocoa/NSURLExtras.mm:1175
>> +	const UNormalizer2 *normalizer = unorm2_getNFCInstance(&uerror);
> Can unorm2_getNFCInstance never fail?

Good catch. It can fail if there is a memory allocation error. So not in
practice, but we should check it regardless, especially since we create an
error, pass it in, and don't check it... that's no good. And maybe in the
future the function could change to fail in different ways.

>> Source/WTF/wtf/cocoa/NSURLExtras.mm:1177
>> +	if (uerror == U_BUFFER_OVERFLOW_ERROR) {
> Is this case covered by tests?  Are there tests with emoji?

There is a test with a snowman emoji.

None of the tests intentionally triggers this "error condition". (Of course
it's not a real error, just bad API design to require the caller to allocate a
bigger buffer instead of ICU allocating the right size and returning it.) I can
check the patch in bug #174816 tomorrow to see if any of those tests hit this
codepath in the big patch.

More information about the webkit-reviews mailing list