[webkit-reviews] review granted: [Bug 71383] Replace usage of StringImpl with String where possible in CharacterData and Text : [Attachment 113351] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adam Klein <adamk at chromium.org>'s
request for review:
Bug 71383: Replace usage of StringImpl with String where possible in
CharacterData and Text
https://bugs.webkit.org/show_bug.cgi?id=71383

Attachment 113351: Patch
https://bugs.webkit.org/attachment.cgi?id=113351&action=review

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


> Source/WebCore/dom/CharacterData.cpp:42
> -    StringImpl* dataImpl = data.impl() ? data.impl() : StringImpl::empty();
> -    if (equal(m_data.get(), dataImpl))
> +    if (m_data == data)

By moving the check for null strings into setDataAndUpdate, we have changed the
behavior of this function. We will now call setDataAndUpdate and call
textRemoved if the passed in data is a null string and the value of m_data is
the empty string. Are you sure that's OK?

> Source/WebCore/dom/CharacterData.cpp:164
> -    return !m_data || m_data->containsOnlyWhitespace();
> +    return !m_data.impl() || m_data.impl()->containsOnlyWhitespace();

I think we should make a String::containsOnlyWhitespace function instead of
doing this here.

> Source/WebCore/dom/CharacterData.cpp:177
> +    m_data = !newData.isNull() ? newData : StringImpl::empty();

It used to be the caller’s responsibility to null-check the data. Now you have
put the check inside this function. Why? Was there some benefit to doing so?

> Source/WebCore/dom/CharacterData.h:27
> +#include "PlatformString.h"

This is the old style of include, and should not be used in new code. The new
style is:

    #include <wtf/text/WTFString.h>

> Source/WebCore/dom/CharacterData.h:60
> +    void setDataNoUpdate(const String& data) { m_data = data; }

Maybe we should be asserting that data is not null?

Maybe this can be protected instead of public?

The name setDataNoUpdate is kind of caveman talk: “me set data, no update”.
Maybe setDataWithoutUpdate? Or maybe we can figure out an even better name.