[webkit-reviews] review denied: [Bug 67081] Basic enhancements to StringBuilder : [Attachment 110288] patch v4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Xianzhu Wang
<wangxianzhu at chromium.org>'s request for review:
Bug 67081: Basic enhancements to StringBuilder
https://bugs.webkit.org/show_bug.cgi?id=67081

Attachment 110288: patch v4
https://bugs.webkit.org/attachment.cgi?id=110288&action=review

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


This looks good, but is not quite ready to land. I have some comments and there
is a problem because this did not build on EWS with the Windows bot.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:118
> +    AtomicString toAtomicString() const
> +    {
> +	   if (!m_length)
> +	       return AtomicString();
> +	   if (canShrink())
> +	       return AtomicString(characters(), length());
> +	   if (!m_string.isNull())
> +	       return AtomicString(m_string);
> +	   return AtomicString(toStringPreserveCapacity());

I don’t understand the canShrink logic here. That function determines if it’s a
good idea to just use the buffer rather than creating a new one since it’s
already mostly full. But here, the code says “if the buffer is already full
then don’t use it”, which seems like the opposite of what canShrink is supposed
to mean. If this is a good algorithm, then we need a comment explaining why.

The if (!m_string.isNull) part of this function has no value; it just repeats
what toStringPreserveCapacity will already do.

Your comment said that there is no easy way to optimize the case where the
string is already the right size and happens to be in the table without
complicating the relationship between AtomicString and StringBuilder, but that
does not seem to be the case given the current form of this function.

If the AtomicString constructor had a version that took a character pointer, a
length, and a StringImpl, it could have all the correct logic;
StringBuilder::reifyString has nothing in it that is truly specific to
StringBuilder policy.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:175
> +    // Disallow the copy constructor and the assignment operator otherwise
the cost could be easily ignored.
> +    StringBuilder(const StringBuilder&);
> +    StringBuilder& operator=(const StringBuilder&);

This is a great idea. In WebKit the normal way to do this is to use
WTF_NONCOPYABLE.

I would word it like this:

    // Disallow copying since it’s expensive and we don’t want code to do it by
accident.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:192
> +    if (s.isEmpty() && !length)
> +	   return true;
> +    if (s.length() != length)
> +	   return false;

If the StringBuilder is empty, presumably the cost of calling the length
function is quite small, so the first check seems to be wasteful.

> Source/JavaScriptCore/wtf/text/StringBuilder.h:195
> +    if (s.characters() == buffer)
> +	   return true;

While this condition is possible in theory, it seems it would never occur in
practice. Accordingly, I suggest omitting this.

> Source/WebCore/editing/markup.cpp:218
> -    return result.toString().replace(0, "");
> +    String string = result.toString();
> +    return string.replace(0, "");

What’s the rationale for this change? It doesn’t seem that the copy constructor
or the assignment operator are relevant here.