Created attachment 418241 [details] Patch

Clean up CharacterData. Originally, I was thinking whether this can affect on SP2/Elm since the hottest function is CharacterData::replaceData. But it turned out that Elm is consuming most of time for rendering that is invoked from CharacterData::replaceData, and it is not consuming much time for CharacterData itself. But when looking into CharacterData, I thought we should just clean up since there are many non-efficient things.

Comment on attachment 418241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418241&action=review > Source/WebCore/dom/CharacterData.cpp:179 > + NodeType nodeType = this->nodeType(); nodeType is a virtual function. Let's not use that? > Source/WebCore/dom/CharacterData.cpp:187 > - if (is<Text>(*this) && parentNode()) > + if (nodeType == TEXT_NODE && parentNode()) Keep the old code. is<Text>(*this) is a node flag check (bit flag). > Source/WebCore/dom/CharacterData.cpp:190 > - if (is<ProcessingInstruction>(*this)) > + if (nodeType == PROCESSING_INSTRUCTION_NODE) Let's just add ProcessingInstruction as a node flag too. We've got quite a few bits available in node flags now. > Source/WebCore/dom/Text.h:61 > + // Like appendData, but optimized for the parser (e.g., no mutation events). > + // Returns how much could be added before length limit was met. I don't think this comment is necessary.

Comment on attachment 418241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418241&action=review Thanks! >> Source/WebCore/dom/CharacterData.cpp:179 >> + NodeType nodeType = this->nodeType(); > > nodeType is a virtual function. Let's not use that? Sounds good! Will change. >> Source/WebCore/dom/CharacterData.cpp:187 >> + if (nodeType == TEXT_NODE && parentNode()) > > Keep the old code. is<Text>(*this) is a node flag check (bit flag). Nice, fixed. >> Source/WebCore/dom/CharacterData.cpp:190 >> + if (nodeType == PROCESSING_INSTRUCTION_NODE) > > Let's just add ProcessingInstruction as a node flag too. We've got quite a few bits available in node flags now. Sounds good! >> Source/WebCore/dom/Text.h:61 >> + // Returns how much could be added before length limit was met. > > I don't think this comment is necessary. Removed.

Comment on attachment 418241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418241&action=review > Source/WebCore/dom/CharacterData.cpp:106 > ASSERT(!renderer() || is<Text>(*this)); Should this assertion be removed since we ASSERT(is<Text>(*this)) above ? > Source/WebCore/dom/CharacterData.cpp:201 > + if (nodeType == TEXT_NODE && !offsetOfReplacedData) { Should is<Text>(*this) be used instead? > Source/WebCore/dom/Text.h:64 > + return CharacterData::parserAppendDataForText(string, offset, lengthLimit); It is weird to have the derived class exclusively calls a function in the base class. I think it is cleaner if we move the code of CharacterData::parserAppendDataForText() into Text::parserAppendData(). We just need to make these members of CharacterData be protected: protected: void notifyParentAfterChange(ContainerNode::ChildChange::Source); String m_data;

(In reply to Said Abou-Hallawa from comment #5) > Comment on attachment 418241 [details] > Patch > > > Source/WebCore/dom/Text.h:64 > > + return CharacterData::parserAppendDataForText(string, offset, lengthLimit); > > It is weird to have the derived class exclusively calls a function in the > base class. I think it is cleaner if we move the code of > CharacterData::parserAppendDataForText() into Text::parserAppendData(). We > just need to make these members of CharacterData be protected: > > protected: > void notifyParentAfterChange(ContainerNode::ChildChange::Source); > String m_data; That would be strictly worse. We don't want to let subclasses of CharacterData have direct access to m_data.

Comment on attachment 418241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418241&action=review >>> Source/WebCore/dom/Text.h:64 >>> + return CharacterData::parserAppendDataForText(string, offset, lengthLimit); >> >> It is weird to have the derived class exclusively calls a function in the base class. I think it is cleaner if we move the code of CharacterData::parserAppendDataForText() into Text::parserAppendData(). We just need to make these members of CharacterData be protected: >> >> protected: >> void notifyParentAfterChange(ContainerNode::ChildChange::Source); >> String m_data; > > That would be strictly worse. We don't want to let subclasses of CharacterData have direct access to m_data. How can giving the superclasses a direct access to m_data be a problem?

(In reply to Said Abou-Hallawa from comment #7) > Comment on attachment 418241 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418241&action=review > > >>> Source/WebCore/dom/Text.h:64 > >>> + return CharacterData::parserAppendDataForText(string, offset, lengthLimit); > >> > >> It is weird to have the derived class exclusively calls a function in the base class. I think it is cleaner if we move the code of CharacterData::parserAppendDataForText() into Text::parserAppendData(). We just need to make these members of CharacterData be protected: > >> > >> protected: > >> void notifyParentAfterChange(ContainerNode::ChildChange::Source); > >> String m_data; > > > > That would be strictly worse. We don't want to let subclasses of CharacterData have direct access to m_data. > > How can giving the superclasses a direct access to m_data be a problem? If m_data is a protected member of CharacterData, then it's unclear to the readers of CharacterData.h/cpp in what all the ways m_data could be mutated without having to read all the subclasses of CharacterData. The way code is organized right now, all mutations of m_data is done in CharacterData itself so the future readers can study them all without having to enumerate all its subclasses.

Comment on attachment 418241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418241&action=review >>>>> Source/WebCore/dom/Text.h:64 >>>>> + return CharacterData::parserAppendDataForText(string, offset, lengthLimit); >>>> >>>> It is weird to have the derived class exclusively calls a function in the base class. I think it is cleaner if we move the code of CharacterData::parserAppendDataForText() into Text::parserAppendData(). We just need to make these members of CharacterData be protected: >>>> >>>> protected: >>>> void notifyParentAfterChange(ContainerNode::ChildChange::Source); >>>> String m_data; >>> >>> That would be strictly worse. We don't want to let subclasses of CharacterData have direct access to m_data. >> >> How can giving the superclasses a direct access to m_data be a problem? > > If m_data is a protected member of CharacterData, then it's unclear to the readers of CharacterData.h/cpp in what all the ways m_data could be mutated without having to read all the subclasses of CharacterData. The way code is organized right now, all mutations of m_data is done in CharacterData itself so the future readers can study them all without having to enumerate all its subclasses. In this case, m_data can stay private and Text::parserAppendData() can use local variable and pass it as an argument to setDataWithoutUpdate(). This is what Text::splitText() does.

<rdar://problem/73810544>