[webkit-reviews] review canceled: [Bug 97740] HTMLConstructionSite::insertTextNode isn't optimized for 8 bit strings : [Attachment 165912] Patch

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has canceled 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 165912: Patch
https://bugs.webkit.org/attachment.cgi?id=165912&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
(In reply to comment #2)
> (From update of attachment 165912 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=165912&action=review
> 
> > Source/WTF/wtf/text/WTFString.cpp:186
> > +	     LChar* data;
> 
> This could go after the if() for clarity.

Moved this and the similar instances of UChar* data.
 
> > Source/WTF/wtf/text/WTFString.cpp:187
> > +	     if (lengthToAppend > numeric_limits<unsigned>::max() - length())
> 
> In String, length() will test if m_impl is null. At this point, you know you
have m_impl. This call to length() and the following should use the StringImpl
directly.
 
Factored out length to a local.

> > Source/WTF/wtf/text/WTFString.cpp:190
> > +	     memcpy(data, characters8(), length() * sizeof(LChar));
> 
> characters8() also checks if m_impl is null.

Changed occurrences of characters8() and characters16() to be via m_impl.

> I am not sure what is better here, memcpy or StringImpl::copyChars()?

Since compChars() has some optimization for word at once and reverts to memcpy
after one check, I used it in all cases.

> > Source/WebCore/dom/CharacterData.cpp:88
> > -	 m_data.append(data, end);
> > +	 if (string.is8Bit())
> > +	     m_data.append(string.characters8() + offset, end);
> > +	 else
> > +	     m_data.append(string.characters16() + offset, end);
> 
> Can "string" ever be null?

I don't think string can be null in the current path to this method, but I
added a check anyway.