[webkit-reviews] review granted: [Bug 97740] HTMLConstructionSite::insertTextNode isn't optimized for 8 bit strings : [Attachment 166035] Patch with extra space removed

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 97740: HTMLConstructionSite::insertTextNode isn't optimized for 8 bit
strings
https://bugs.webkit.org/show_bug.cgi?id=97740

Attachment 166035: Patch with extra space removed
https://bugs.webkit.org/attachment.cgi?id=166035&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=166035&action=review


> Source/WebCore/dom/CharacterData.cpp:66
> -unsigned CharacterData::parserAppendData(const UChar* data, unsigned
dataLength, unsigned lengthLimit)
> +unsigned CharacterData::parserAppendData(const String& string, unsigned
dataLength, unsigned lengthLimit, unsigned offset)

None of the callers need the dataLength argument. They all just pass the string
length minus the offset.

I also think the name dataLength is confusing given that the first line in this
function evaluates m_data.length(), which is not the same thing as dataLength.
It was defensible before when the argument passed in was named “data”, but now
we’re passing a string that already has its intrinsic length so it’s not good
to pass a separate length.

I suggest removing the dataLength argument and using the string length.

I also think the offset would be more logical as the first parameter, right
after the string. A little strange to have it be the last one separated from
the string by the length limit, even though having it last allows it to be
optional.

On reflection, the arguments to this are similar the arguments to the
String::substring function: An offset followed by a count. The difference here
is that lengthLimit is a limit on the total size of the CharacterData object
rather than a limit on the number of new characters to be added from the
string.

I also think this function needs to assert that lengthLimit >= oldLength,
because otherwise it will malfunction. Or be re-coded to handle that case if
the assert fires.

> Source/WebCore/dom/CharacterData.cpp:82
> +    if (!end || !string.length())

This string.length() check we are adding will never be true. That’s because
dataLength == (string.length() - offset), and end <= dataLength <=
string.length(). So if string.length() is zero, end will also be zero. Also,
end is not a good name for the variable now that we have an offset. With the
offset involved, end is the length of characters, not the offset of the end in
the string, so it no longer makes sense to call it “end”.

> Source/WebCore/dom/CharacterData.h:48
> -    unsigned parserAppendData(const UChar*, unsigned dataLength, unsigned
lengthLimit);
> +    unsigned parserAppendData(const String&, unsigned, unsigned , unsigned
offset = 0);

Please don’t remove the argument names here. Callers need the names to
understand what the otherwise undocumented arguments are.

Also got a stray space here before a comma.