Bug 220904 – Clean up CharacterData

• Component: Text
• Status: NEW

[Yusuke Suzuki]

Clean up CharacterData

[Yusuke Suzuki]

Created attachment 418241 [details]
Patch

[Yusuke Suzuki]

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.

[Ryosuke Niwa]

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.

[Yusuke Suzuki]

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.

[Said Abou-Hallawa]

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;

[Ryosuke Niwa]

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

[Said Abou-Hallawa]

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?

[Ryosuke Niwa]

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

[Said Abou-Hallawa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/73810544>