Replace usage of StringImpl with String where possible in CharacterData and Text
Created attachment 113351 [details] Patch
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 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.
Created attachment 113392 [details] Patch for landing
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 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?
(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.
Created attachment 113400 [details] Patch for landing
(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 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.
(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
(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.
Created attachment 113404 [details] Patch for landing
So after all that, turns out String::empty() already exists. Except it's called WTF::emptyString(). CharacterData no longer knows anything about StringImpl.
Comment on attachment 113404 [details] Patch for landing Clearing flags on attachment: 113404 Committed r99128: <http://trac.webkit.org/changeset/99128>
All reviewed patches have been landed. Closing bug.
Thanks for the cleanup, Adam! It's a great improvement.