Bug 61111 - RenderText with empty text is not created inside ShadowContentElement
Summary: RenderText with empty text is not created inside ShadowContentElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on: 61114
Blocks: 56973
  Show dependency treegraph
 
Reported: 2011-05-19 00:00 PDT by Hajime Morrita
Modified: 2011-05-25 19:32 PDT (History)
3 users (show)

See Also:


Attachments
A repro (638 bytes, text/html)
2011-05-19 00:00 PDT, Hajime Morrita
no flags Details
Patch (37.22 KB, patch)
2011-05-24 04:53 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.26 MB, application/zip)
2011-05-24 06:07 PDT, WebKit Review Bot
no flags Details
Patch (36.46 KB, patch)
2011-05-24 20:50 PDT, Hajime Morrita
dglazkov: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.28 MB, application/zip)
2011-05-24 22:42 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-05-19 00:00:59 PDT
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.
Comment 1 Hajime Morrita 2011-05-24 04:53:37 PDT
Created attachment 94595 [details]
Patch
Comment 2 WebKit Review Bot 2011-05-24 06:07:03 PDT
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
Comment 3 WebKit Review Bot 2011-05-24 06:07:08 PDT
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
Comment 4 Dimitri Glazkov (Google) 2011-05-24 08:13:06 PDT
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?
Comment 5 Hajime Morrita 2011-05-24 20:50:48 PDT
Created attachment 94738 [details]
Patch
Comment 6 Hajime Morrita 2011-05-24 21:13:39 PDT
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.
Comment 7 WebKit Review Bot 2011-05-24 22:42:11 PDT
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
Comment 8 WebKit Review Bot 2011-05-24 22:42:17 PDT
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
Comment 9 Dimitri Glazkov (Google) 2011-05-25 08:33:41 PDT
Comment on attachment 94738 [details]
Patch

Ok, but please look at the EWS test failures.
Comment 10 Hajime Morrita 2011-05-25 18:04:53 PDT
> (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!
Comment 11 Hajime Morrita 2011-05-25 19:32:07 PDT
Committed r87351: <http://trac.webkit.org/changeset/87351>