[webkit-reviews] review denied: [Bug 65477] Nested fixed position element not staying with parent : [Attachment 178583] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 65477: Nested fixed position element not staying with parent
https://bugs.webkit.org/show_bug.cgi?id=65477

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=178583&action=review


Dave mentioned using RenderBlock::layoutPositionedObjects for the fix instead
of tying it to style change. Your earlier efforts went in this direction, is
there something that made your rethink the approach?

One upside of a layout approach is that it removes the need for an extra
positioned object traversal. Also during RenderBlock::layoutPositionedObjects,
you know your containing block and it should fix both full and simplified
layout.

> Source/WebCore/ChangeLog:17
> +	   (WebCore):

Let's remove these nasty lines.

> Source/WebCore/ChangeLog:18
> +	   (WebCore::RenderBlock::styleWillChange):

Does this code need to run on styleWillChange? AFAICT unless you really need to
do some mutation pre-style change, this should be moved to styleDidChange (see
for example, the code for floats).

> Source/WebCore/rendering/RenderBlock.cpp:288
> +static void setFixedPositionDescendantsNeedLayout(RenderBlock* child)

The naming is just confusing: |child| is actually the starting renderer and you
pass |this| to it.

> Source/WebCore/rendering/RenderBlock.cpp:294
> +    RenderBox* fixedPositionObject;

We usually define the variable when we need them.

> Source/WebCore/rendering/RenderBlock.cpp:308
> +	   while (parent && !parent->isRenderView()) {
> +	       if (parent == child) {
> +		   fixedPositionObject->setChildNeedsLayout(true,
MarkContainingBlockChain);
> +		   return;
> +	       }
> +	       parent = parent->parent();
> +	   }

I don't understand the need for this loop. A positioned object gets inserted
into its containing block's map (look at the logic in
RenderBlock::handlePositionedChild) so fixedPositionedObject->containingBlock()
should ALWAYS be |child|.