[webkit-reviews] review denied: [Bug 3359] Crash on hover with certain styles on the text applied : [Attachment 3014] Patch

[bugzilla-request-daemon at opendarwin.org]

Maciej Stachowiak <mjs at apple.com> has denied Justin Garcia
<justin.garcia at apple.com>'s request for review:
Bug 3359: Crash on hover with certain styles on the text applied
http://bugzilla.opendarwin.org/show_bug.cgi?id=3359

Attachment 3014: Patch
http://bugzilla.opendarwin.org/attachment.cgi?id=3014&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
I don't think this patch is quite right. When the document as a whole is
attached, individual nodes should not be detached, even if they do not need a
renderer. If a node changes to a state where it should no longer have a
renderer, then the right thing to do is to detach and reattach it, if it is
already attached. Then createRendererIfNeeded will do the right thing and make
a new renderer or not. Note that CharacterDataImpl::rendererIsNeeded will
already refuse to create a renderer if the string is empty.

Conversely, you can't just unconditionally attach a text node if its text is
getting changed to non-empty. If style hasn't been resolved yet, it wouldn't
have been attached in the first place.

And finally, note that the EditingTextImpl subclass of TextImpl can validly
have a renderer even if empty. This is the type of text node that gets inserted
in preparation for user typing in a space where there is no text already
present. So in addition to straightening out whether empty text nodes have a
renderer, you may also have to address the specific circumastances that cause a
crash in this case.