[webkit-reviews] review granted: [Bug 180009] Modernize some aspects of text codecs, eliminate WebKit use of strcasecmp : [Attachment 327565] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 27 21:46:22 PST 2017


Alex Christensen <achristensen at apple.com> has granted Darin Adler
<darin at apple.com>'s request for review:
Bug 180009: Modernize some aspects of text codecs, eliminate WebKit use of
strcasecmp
https://bugs.webkit.org/show_bug.cgi?id=180009

Attachment 327565: Patch

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




--- Comment #5 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 327565
  --> https://bugs.webkit.org/attachment.cgi?id=327565
Patch

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

> Source/WTF/wtf/text/LineEnding.cpp:57
> +	       needFix = true;

You could either break after either one of these, or you could count the number
of changes you will need to make to call reserveInitialCapacity and
uncheckedAppend in the loop below.  The first condition would reduce the length
by one and this could just replace the character in place.  You could even do
both in place then adjust the size of the Vector but not the capacity to make 0
allocations in this function.

Maybe a FIXME would be appropriate for these because they're just performance
optimizations and you're developing on a platform that doesn't even compile
them.

> Source/WTF/wtf/text/LineEnding.cpp:131
> +    return result;

ASSERT(q == result.data() + result.size()) might be nice here.

> Source/WTF/wtf/text/StringCommon.h:659
> +    return length == strlen(b) && equalIgnoringASCIICase(a, b, length);

This reads each character twice when only one read is necessary.  A loop that
checks for the null character before when it's expected might have better
performance.

> Source/WTF/wtf/text/StringCommon.h:665
> +    return strlen(string) == length && equalLettersIgnoringASCIICase(string,
lowercaseLetters, length);

ditto

> Source/WebCore/PAL/pal/text/UnencodableHandling.h:32
> +enum UnencodableHandling {

enum classes are all the new hotness because they don't pollute the namespace
as much.  It's not the biggest deal for this one, but I think it's good
practice to use enum classes unless there's a reason to use an enum.

> Source/WebCore/fileapi/BlobBuilder.cpp:59
> -    m_appendableData.append(static_cast<const
char*>(arrayBufferView->baseAddress()), arrayBufferView->byteLength());
> +    m_appendableData.append(static_cast<const
uint8_t*>(arrayBufferView->baseAddress()), arrayBufferView->byteLength());

I think these static casts are no longer necessary.

> Source/WebCore/platform/SharedBuffer.cpp:80
> +    return adoptRef(*new SharedBuffer { vector.data(), vector.size() });

Couldn't we do some kind of awful casting instead of copying the data
unnecessarily?


More information about the webkit-reviews mailing list