[webkit-reviews] review denied: [Bug 69210] CSS 2.1 failure: inline-box-002.htm fails : [Attachment 123023] 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 123023: Patch
https://bugs.webkit.org/attachment.cgi?id=123023&action=review

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


> Source/WebCore/rendering/RenderBoxModelObject.cpp:411
> +    if (isAnonymousBlock() && isRelPositioned()) {
> +	   RenderInline* p = toRenderBlock(this)->inlineElementContinuation();
> +	   while (p) {
> +	       offset += p->relativePositionOffsetX();
> +	       RenderObject* parent = p->parent();
> +	       p = parent->isRenderInline() ? toRenderInline(parent) : 0;
> +	   }
> +    }

All this code is duplicated in relativePositionOffsetY as well. Pull it into a
little static helper function please.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:437
> +	   while (p) {

Seems easier to just write while (p && p->isRenderInline()) and then just p =
p->parent();

> Source/WebCore/rendering/RenderInline.cpp:246
> +static bool hasRelPositionedAncestor(RenderInline* p)
> +{
> +    while (p) {
> +	   if (p->isInline() && p->isRelPositioned())
> +	       return true;
> +	   RenderObject* parent = p->parent();
> +	   p = parent->isRenderInline() ? toRenderInline(parent) : 0;
> +    }
> +    return false;
> +}

Seems like this could be more easily written (assuming p is just a RenderObject
as):

while (p && p->isRenderInline()) {
    if (p->isRelPositioned())
	return true;
    p = p->parent();
}
return false;

> Source/WebCore/rendering/RenderInline.cpp:265
> +	   if (hasRelPositionedAncestor(this))
> +	       newStyle->setPosition(RelativePosition);

You can't just do this at creation time. You're assuming a static state as far
as all the enclosing RenderInlines at the time the splitting of the inlines
happens. However, it's possible that much later on, someone could come along
and make one of the RenderInlines relative positioned (or the opposite, take
away relative positioning).

You need a test case that starts off with nothing relatively positioned, and
then waits and converts one of the inlines in the chain to be relatively
positioned later to see what I mean.

Might be easier to just chat on IRC about how to handle the dynamic situation.

Note that I will still r+ the non-dynamic case since it is an improvement, so
if you want to land this initially I'm cool with it.