[Webkit-unassigned] [Bug 136519] WebProcess spins loading a very large textarea

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=136519


Andy Estes <aestes at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237636|review?                     |review-
               Flag|                            |




--- Comment #4 from Andy Estes <aestes at apple.com>  2014-09-09 18:18:00 PST ---
(From update of attachment 237636)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.