Bug 71383 – Replace usage of StringImpl with String where possible in CharacterData and Text

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

Replace usage of StringImpl with String where possible in CharacterData and Text

[Adam Klein]

Created attachment 113351 [details]
Patch

[Darin Adler]

Comment on attachment 113351 [details]
Patch

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.

[Adam Klein]

Comment on attachment 113351 [details]
Patch

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

>> Source/WebCore/dom/CharacterData.cpp:42
>> +    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?

No, I'm not sure it's ok, and I didn't actually mean to cause a behavior change here (forgot that empty and null strings aren't equal).  Brought back dataImpl.

>> Source/WebCore/dom/CharacterData.cpp:164
>> +    return !m_data.impl() || m_data.impl()->containsOnlyWhitespace();
> 
> I think we should make a String::containsOnlyWhitespace function instead of doing this here.

Done.

>> 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?

This was part of the same change above, where I tried to simplify setData().  Removed.

>> 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>

Ah, I'd seen that but didn't know it was the new style. Done.

>> 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.

Added ASSERT.

It already is protected.

setDataWithoutUpdate sounds fine by me (this only has one caller, and that seems to be the intention). This is still a better name than setDataImpl.

[Adam Klein]

Created attachment 113392 [details]
Patch for landing

[Darin Adler]

Comment on attachment 113392 [details]
Patch for landing

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

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

You don’t really need to use a StringImpl* for this. It could just be:

    const String& nonNullData = data.isNull() ? data : String::empty();
    if (m_ data == nonNullData)
        return;

    [...]

    setDataAndUpdate(nonNullData, 0, oldLength, nonNullData.length());

[Adam Klein]

Comment on attachment 113392 [details]
Patch for landing

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

>> Source/WebCore/dom/CharacterData.cpp:44
>>          return;
> 
> You don’t really need to use a StringImpl* for this. It could just be:
> 
>     const String& nonNullData = data.isNull() ? data : String::empty();
>     if (m_ data == nonNullData)
>         return;
> 
>     [...]
> 
>     setDataAndUpdate(nonNullData, 0, oldLength, nonNullData.length());

Given that there's no such thing as String::empty() (only StringImpl::empty()), I'm disinclined to use this approach, since I'd still need to refer to StringImpl.

Or are you suggesting creating an |empty| method on String?

[Darin Adler]

(In reply to comment #6)
> Given that there's no such thing as String::empty() (only StringImpl::empty()), I'm disinclined to use this approach, since I'd still need to refer to StringImpl.
> 
> Or are you suggesting creating an |empty| method on String?

I think it would probably be good to have a String::empty() function, but for an alternative that already works, we could just use emptyAtom.

[Adam Klein]

Created attachment 113400 [details]
Patch for landing

[Adam Klein]

(In reply to comment #7)
> (In reply to comment #6)
> > Given that there's no such thing as String::empty() (only StringImpl::empty()), I'm disinclined to use this approach, since I'd still need to refer to StringImpl.
> > 
> > Or are you suggesting creating an |empty| method on String?
> 
> I think it would probably be good to have a String::empty() function, but for an alternative that already works, we could just use emptyAtom.

Done, using emptyAtom (though it required a static_cast to satisfy the ternary operator).

[Darin Adler]

Comment on attachment 113400 [details]
Patch for landing

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

> Source/WebCore/dom/CharacterData.cpp:42
> +    const String& nonNullData = !data.isNull() ? data : static_cast<const String&>(emptyAtom);

We should adding a String::empty() function and use it here to clean this up some time soon.

[Adam Klein]

(In reply to comment #10)
> (From update of attachment 113400 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113400&action=review
> 
> > Source/WebCore/dom/CharacterData.cpp:42
> > +    const String& nonNullData = !data.isNull() ? data : static_cast<const String&>(emptyAtom);
> 
> We should adding a String::empty() function and use it here to clean this up some time soon.

Are you thinking it'd return a const String&?  If so, the simplest thing'd seem to just be

// static
const String& String::empty()
{
    return emptyAtom;
}

in StringStatics.cpp

[Darin Adler]

(In reply to comment #11)
> (In reply to comment #10)
> > We should add a String::empty() function and use it here to clean this up some time soon.
> 
> Are you thinking it'd return a const String&?

Yes.

> If so, the simplest thing'd seem to just be
> 
> // static
> const String& String::empty()
> {
>     return emptyAtom;
> }
> 
> in StringStatics.cpp

Maybe. We might also want to make it be an inlined access to a global so it’s fast.

[Adam Klein]

Created attachment 113404 [details]
Patch for landing

[Adam Klein]

So after all that, turns out String::empty() already exists.  Except it's called WTF::emptyString().

CharacterData no longer knows anything about StringImpl.

[WebKit Review Bot]

Comment on attachment 113404 [details]
Patch for landing

Clearing flags on attachment: 113404

Committed r99128: <http://trac.webkit.org/changeset/99128>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Thanks for the cleanup, Adam! It's a great improvement.