Bug 71383 - Replace usage of StringImpl with String where possible in CharacterData and Text
Summary: Replace usage of StringImpl with String where possible in CharacterData and Text
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-02 12:22 PDT by Adam Klein
Modified: 2011-11-02 18:37 PDT (History)
3 users (show)

See Also:


Attachments
Patch (10.66 KB, patch)
2011-11-02 12:26 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (11.94 KB, patch)
2011-11-02 16:13 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (12.22 KB, patch)
2011-11-02 16:39 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (12.19 KB, patch)
2011-11-02 17:06 PDT, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-11-02 12:22:11 PDT
Replace usage of StringImpl with String where possible in CharacterData and Text
Comment 1 Adam Klein 2011-11-02 12:26:14 PDT
Created attachment 113351 [details]
Patch
Comment 2 Darin Adler 2011-11-02 14:44:37 PDT
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.
Comment 3 Adam Klein 2011-11-02 16:12:59 PDT
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.
Comment 4 Adam Klein 2011-11-02 16:13:27 PDT
Created attachment 113392 [details]
Patch for landing
Comment 5 Darin Adler 2011-11-02 16:25:01 PDT
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());
Comment 6 Adam Klein 2011-11-02 16:28:29 PDT
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?
Comment 7 Darin Adler 2011-11-02 16:31:53 PDT
(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.
Comment 8 Adam Klein 2011-11-02 16:39:16 PDT
Created attachment 113400 [details]
Patch for landing
Comment 9 Adam Klein 2011-11-02 16:39:46 PDT
(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).
Comment 10 Darin Adler 2011-11-02 16:40:50 PDT
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.
Comment 11 Adam Klein 2011-11-02 16:53:25 PDT
(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
Comment 12 Darin Adler 2011-11-02 17:01:36 PDT
(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.
Comment 13 Adam Klein 2011-11-02 17:06:15 PDT
Created attachment 113404 [details]
Patch for landing
Comment 14 Adam Klein 2011-11-02 17:07:17 PDT
So after all that, turns out String::empty() already exists.  Except it's called WTF::emptyString().

CharacterData no longer knows anything about StringImpl.
Comment 15 WebKit Review Bot 2011-11-02 18:29:15 PDT
Comment on attachment 113404 [details]
Patch for landing

Clearing flags on attachment: 113404

Committed r99128: <http://trac.webkit.org/changeset/99128>
Comment 16 WebKit Review Bot 2011-11-02 18:29:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Ryosuke Niwa 2011-11-02 18:37:25 PDT
Thanks for the cleanup, Adam! It's a great improvement.