Bug 61111 – RenderText with empty text is not created inside ShadowContentElement

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Created attachment 94045 [details]
A repro

This is because Text::renererIsNeeded() referes parentNode() and previousRenderer(), 
which should be based on content render tree, but is not at this time.

Note that this missing RenderText problem is happens
only after dynamics change of content element children.

[Hajime Morrita]

Created attachment 94595 [details]
Patch

[WebKit Review Bot]

Comment on attachment 94595 [details]
Patch

Attachment 94595 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8730453

New failing tests:
fast/forms/select-change-listbox-to-popup.html
fast/forms/ValidityState-001.html
fast/forms/input-file-re-render.html
dom/html/level2/html/AppletsCollection.html
svg/custom/getsvgdocument.html
fast/forms/element-order.html
fast/forms/preserveFormDuringResidualStyle.html
fast/forms/input-type-change.html
fast/forms/checkValidity-001.html
fast/forms/input-type-change2.html
tables/mozilla/bugs/bug113235-1.html
fast/forms/select-change-listbox-size.html
fast/forms/select-change-popup-to-listbox.html
fast/forms/form-in-malformed-markup.html
fast/forms/input-align-image.html
fast/forms/input-number-change-type-on-focus.html
fast/forms/input-align.html
tables/mozilla/bugs/bug113235-2.html
fast/forms/file-input-disabled.html
fast/forms/formmove3.html

[WebKit Review Bot]

Created attachment 94597 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dimitri Glazkov (Google)]

Comment on attachment 94595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94595&action=review

cr-linux is angry plus comments:

> Source/WebCore/dom/NodeRenderingContext.cpp:56
> +    // Doesn't resolve() yet. You can do it later by calling ensureResolved().

This comment is unnecessary.

> Source/WebCore/dom/NodeRenderingContext.cpp:59
> +void NodeRenderingContext::resolve()

determineLocation?

> Source/WebCore/dom/NodeRenderingContext.h:66
> +        LocationUnresolved,

LocationUndetermined?

> Source/WebCore/dom/NodeRenderingContext.h:102
> +inline void NodeRenderingContext::ensureResolved()

usually this things have IfNeeded suffix, like resolveIfNeeded.

> Source/WebCore/dom/Text.cpp:202
> +    const_cast<NodeRenderingContext&>(context).ensureResolved();

This looks bad. Can we make the m_location mutable?

[Hajime Morrita]

Created attachment 94738 [details]
Patch

[Hajime Morrita]

Hi Dimitri, thank you for reviewing!
I updated the patch.
After some digging, I found that we don't need resolve() phase.
So I removed it and added an alternative way for updateRenderer()
The point is that we can assume that the text node already has an renderer for it.

[WebKit Review Bot]

Comment on attachment 94738 [details]
Patch

Attachment 94738 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8731621

New failing tests:
fast/html/details-replace-text.html
fast/html/details-replace-summary-child.html

[WebKit Review Bot]

Created attachment 94745 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dimitri Glazkov (Google)]

Comment on attachment 94738 [details]
Patch

Ok, but please look at the EWS test failures.

[Hajime Morrita]

> (From update of attachment 94738 [details])
> Ok, but please look at the EWS test failures.
Ops. It needs expectations. I mark it before landing.
Thanks for the review!

[Hajime Morrita]

Committed r87351: <http://trac.webkit.org/changeset/87351>