[webkit-reviews] review denied: [Bug 136519] WebProcess spins loading a very large textarea : [Attachment 237636] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has denied Ding Zhao <ding_zhao at apple.com>'s
request for review:
Bug 136519: WebProcess spins loading a very large textarea
https://bugs.webkit.org/show_bug.cgi?id=136519

Attachment 237636: Patch
https://bugs.webkit.org/attachment.cgi?id=237636&action=review

------- Additional Comments from Andy Estes <aestes at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237636&action=review


r- because the behavior of HTMLTextFormControlElement::setInnerTextValue is
wrong.

> Source/WebCore/dom/ContainerNode.h:123
> +	   Node* childPtr;

This should be of type CharacterData, not Node. Also, we don't use
abbreviations like Ptr. And the name is too general since this only has a
meaningful value in certain cases. I'd call it insertedTextNode.

> Source/WebCore/dom/ContainerNode.h:124
> +	   unsigned numberOfCharactersAppended;

Instead of a Node + unsigned, could we use a WTF::StringView here instead that
references the range of characters that was appended? If you had that then
there'd be no need to know the child node.

> Source/WebCore/html/HTMLTextAreaElement.cpp:148
> +    else if (change.type == TextInserted) {
> +	   CharacterData* child = (CharacterData*)change.childPtr;
> +	   setNonDirtyValue(child->data(), true);
> +    } else if (change.type == TextChanged &&
change.numberOfCharactersAppended > 0) {
> +	   CharacterData* child = (CharacterData*)change.childPtr;
> +	   String text = child->data();
> +	   String newText = text.substring(text.length() -
change.numberOfCharactersAppended, change.numberOfCharactersAppended);
> +	   setNonDirtyValue(newText, true);
> +    } else

I don't like that there are two different change types used for when text is
inserted. Can CharacterData::parserAppendData() use TextInserted instead of
TextChanged? I think it should be able to. I also don't like how
numberOfCharactersAppended only has meaning for TextChanged. Why not always set
it to a valid value? If you did these two things you'd only need one condition
here instead of two.

> Source/WebCore/html/HTMLTextAreaElement.cpp:373
> +void HTMLTextAreaElement::setNonDirtyValue(const String& value, bool
didAppendText)

I don't like that this is still called setNonDirtyValue but it sometimes
appends to the non-dirty value rather than setting it. I would create a new
function called appendToNonDirtyValue, and call it in the two places above that
pass true to setNonDirtyValue.

> Source/WebCore/html/HTMLTextAreaElement.cpp:380
> +void HTMLTextAreaElement::setValueCommon(const String& newValue, bool
didAppendText)

I would rename this to updateValueCommon. I would also have it take an enum
rather than a bool. The enum should have values like OverwriteExistingValue and
AppendToExistingValue.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:524
> +void HTMLTextFormControlElement::setInnerTextValue(const String& value, bool
didAppendText)

I don't like that this is still called setInnerTextValue but it sometimes
appends to the inner text value rather than setting it. I also don't like that
this function still does an expensive string comparison to see if text has
changed even when we know we're appending text.

Let's call this updateInnerTextValue instead, and have it take the enum passed
to setValueCommon instead of a bool. When appending, you'll want to append a
new text node child to innerTextElement() followed by a new <br> element if the
value ends with a line break.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:535
> +	   innerTextElement()->setInnerText(value, ASSERT_NO_EXCEPTION,
didAppendText);

This isn't right. You're potentially appending text before the <br> node
created by the previous call to setInnerTextValue(). Instead you should be
creating a new text node after the <br>. If you did this I don't believe you'd
need any of the changes to HTMLElement::setInnerText or
replaceChildrenWithText.