[Webkit-unassigned] [Bug 52268] Setting outerText should convert CR/LF to <br>

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 12 15:06:40 PST 2011


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





--- Comment #7 from Eric Seidel <eric at webkit.org>  2011-01-12 15:06:40 PST ---
(From update of attachment 78736)
View in context: https://bugs.webkit.org/attachment.cgi?id=78736&action=review

I think the biggest trouble with this patch is that you're inheriting some less-than-perfectly designed code, which has historically been poorly tested.  It's difficult for me to draw the line between your current definite improvement of that code, and the ideal for said code.  This current iteration is better than your last one, and *certainly* way beter than what we had before.  If I were writing this patch, I'd want to go one more round.  But I'm also open to the idea of landing this and iterating further in a separate patch or at a later time.

> Source/WebCore/html/HTMLElement.cpp:387
> +        if (c == '\n' || c == '\r') {

I feel like this should be an early contineue, but that gets a bit ugly with the need for prev = c.

> Source/WebCore/html/HTMLElement.cpp:403
> +    if (length > lineStart)
> +        fragment->appendChild(Text::create(document(), text.substring(lineStart, length - lineStart)), ec);

This is repeated from above, but missing the ec check.  Is that intentional?  how do we exercise this case?

> Source/WebCore/html/HTMLElement.cpp:460


Should just early return instead of making a long if block.

> Source/WebCore/html/HTMLElement.cpp:498
> +    RefPtr<Node> prev = previousSibling();
> +    RefPtr<Node> next = nextSibling();
> +    if (text.isEmpty() && (!prev || !prev->isTextNode()) && (!next || !next->isTextNode())) {
> +        parent->replaceChild(Text::create(document(), ""), this, ec);
> +        return;
> +    }

I'm not sure I understand this quirk or why it needs to be a separate if.  I assume it's tested?

> Source/WebCore/html/HTMLElement.cpp:506
> +    if (text.contains('\r') || text.contains('\n'))
> +        newChild = textToFragment(text, ec);
> +    else
> +        newChild = Text::create(document(), text);

I would just have put this if inside textToFragment, making ti just reutrn a Text node if there are no \n, \r.

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



More information about the webkit-unassigned mailing list