[webkit-reviews] review denied: [Bug 69210] CSS 2.1 failure: inline-box-002.htm fails : [Attachment 124575] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Robert Hogan <robert at webkit.org>'s
request for review:
Bug 69210: CSS 2.1 failure: inline-box-002.htm fails
https://bugs.webkit.org/show_bug.cgi?id=69210

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=124575&action=review


> Source/WebCore/rendering/RenderInline.cpp:140
> +static bool containsDescendantRenderBlock(RenderObject* child, RenderObject*
ancestor)
> +{
> +    ASSERT(child->firstChild() && child->firstChild()->isRenderBlock());
> +    
> +    RenderObject* p = toRenderBlock(child)->inlineElementContinuation();
> +    while (p && p->isRenderInline()) {
> +	   if (p == ancestor)
> +	       return true;
> +	   p = p->parent();
> +    }
> +    return false;
> +}

This function is not necessary. You can just cut it.

> Source/WebCore/rendering/RenderInline.cpp:159
> +	   if (block && block->isAnonymousBlock() &&
containsDescendantRenderBlock(block, currCont)) {

The containing block's next sibling will always be an anonymous block, and its
contents will always be descendants of this inline. So I think you could just
change the block && block->isAnonymousBlock() to be an assert instead and you
can just remove containsDescendantRenderBlock.

> Source/WebCore/rendering/RenderInline.cpp:167
> +    for (; currCont; currCont = currCont->inlineElementContinuation()) {

One issue I see is it looks like you update only one of the anonymous blocks?
An inline can be split multiple times and thus there can be a whole chain of
descendant blocks that need updating. It looks like your code only updates one.


The other issue I see is that you are assuming that your style turns relative
positioning on and off for the block, but what if multiple inlines are
contributing relative positioning? For example:

<span><span style="position:relative; left:10px;top:10px"><div>Hello</div>

In the above example, turning relative positioning off and on for the outermost
span has no impact, since the inner span is still position:relative.

I think you are on the right track, but you probably need to find a way to just
:"dirty" the blocks somehow so that the positioning change update is done by
the block one time and it's done by crawling up the inlines chain examining
their position values.

You might need some kind of state for this.