[webkit-reviews] review denied: [Bug 110786] Performance issues adding large numbers of whitespace text nodes : [Attachment 190557] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kentaro Hara <haraken at chromium.org>'s
request for review:
Bug 110786: Performance issues adding large numbers of whitespace text nodes
https://bugs.webkit.org/show_bug.cgi?id=110786

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=190557&action=review


The idea here seems basically right, but there is some kind of fundamental
mistake.

> Source/WebCore/dom/Node.cpp:1082
>	       if (next->renderer())
>		   break;
> +	       // This means that we intentionally did not create a renderer
for the node.
> +	       if (next->attached() && !next->renderer())
> +		   break;
> +	       // This means none of the following siblings are attached.
>	       if (!next->attached())
> -		   break;  // Assume this means none of the following siblings
are attached.
> +		   break;

This can’t be right. It says:

    if (a)
	break;
    if (b && !a)
	break;
    if (!b)
	break;

That’s the same as just:

    break;

So what this patch does is remove the code entirely in a roundabout way. The
createTextRendererIfNeeded function will never be called.

Maybe that’s what we’re really learning. That this code is not needed! If so,
we should be sure we examine test cases for why this code was originally added
to confirm that.