[webkit-reviews] review granted: [Bug 194272] URLHelpers should use unorm2_quickCheck before converting to NFC : [Attachment 362841] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Michael Catanzaro
<mcatanzaro at igalia.com>'s request for review:
Bug 194272: URLHelpers should use unorm2_quickCheck before converting to NFC
https://bugs.webkit.org/show_bug.cgi?id=194272

Attachment 362841: Patch

https://bugs.webkit.org/attachment.cgi?id=362841&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 362841
  --> https://bugs.webkit.org/attachment.cgi?id=362841
Patch

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

This seems OK as is, but I think there is room for improvement.

> Source/WTF/wtf/URLHelpers.cpp:778
> -    auto sourceBuffer = string.charactersWithNullTermination();
> +    Vector<UChar> sourceBuffer = string.charactersWithNullTermination();

I guess you don’t like auto in a case like this. I do; keeps the code less
wordy.

I don’t understand why we need the null termination. I think unorm2_quickCheck
can work on characters in place; charactersWithNullTermination is an expensive
operation that should be avoided if possible.

> Source/WTF/wtf/URLHelpers.cpp:783
> +    const UNormalizer2* normalizer = unorm2_getNFCInstance(&uerror);

I prefer auto to writing out const UNormalizer2*, but I suppose tastes differ.

> Source/WTF/wtf/URLHelpers.cpp:787
> +    UNormalizationCheckResult checkResult = unorm2_quickCheck(normalizer,
sourceBuffer.data(), sourceBuffer.size(), &uerror);

I prefer auto to writing out UNormalizationCheckResult, but I suppose tastes
differ.

> Source/WTF/wtf/URLHelpers.cpp:793
> +	   return String(sourceBuffer.data(), sourceBuffer.size());

Better performance if this does:

    return string;

Then we share the StringImpl instead of allocating a new one with the same
characters in it.

> Source/WTF/wtf/URLHelpers.cpp:798
> +    uerror = U_ZERO_ERROR;

Is this needed? If there was an error, we would have returned, I think.

> Source/WTF/wtf/URLHelpers.cpp:799
> +    if (U_SUCCESS(uerror)) {

This is strange. It checks the value of something we set the line before. I
think we instead want to remove this if statement.