[webkit-reviews] review denied: [Bug 90046] Implement sticky positioning : [Attachment 154208] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 90046: Implement sticky positioning
https://bugs.webkit.org/show_bug.cgi?id=90046

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

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


Looks pretty good, but I am confused about the margin issue. What is keeping
you from using the values on the RenderObject instead of trying to compute them
yourself.

> Source/WebCore/rendering/RenderBox.cpp:1517
> +	       if ((styleToUse->position() == RelativePosition ||
styleToUse->position() == StickyPosition) && layer())
> +		   rect.move(layer()->offsetForInFlowPosition());

It does not use the RenderObject bits because this can be called when
re-resolving style, before the bits have been set. Rather than making this a
FIXME, you could just explain that.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:567
> +    LayoutUnit marginLeftLength =
minimumValueForLength(style()->marginLeft(),
containingBlockLogicalWidthForContent(), view());
> +    LayoutUnit marginTopLength = minimumValueForLength(style()->marginTop(),
containingBlock->availableLogicalHeight(), view());
> +    LayoutUnit marginRightLength =
minimumValueForLength(style()->marginRight(),
containingBlockLogicalWidthForContent(), view());

As Tony mentioned, this isn't correct. You're missing physical and logical
coordinate systems here. I'm a bit confused why you don't have your margins
computed already? Why can't you just use the RenderBox accessors?

> Source/WebCore/rendering/RenderInline.cpp:173
> +void RenderInline::styleWillChange(StyleDifference diff, const RenderStyle*
newStyle)
> +{
> +    if (FrameView *frameView = view()->frameView()) {
> +	   RenderStyle* oldStyle = style();
> +	   bool newStyleIsSticky = newStyle && newStyle->position() ==
StickyPosition;
> +	   bool oldStyleIsSticky = oldStyle && oldStyle->position() ==
StickyPosition;
> +	   if (newStyleIsSticky != oldStyleIsSticky) {
> +	       if (newStyleIsSticky)
> +		   frameView->addFixedObject(this);
> +	       else
> +		   frameView->removeFixedObject(this);
> +	   }
> +    }
> +
> +    RenderBoxModelObject::styleWillChange(diff, newStyle);
> +}

Have you verified that the scrolling code that looks at fixed objects does the
right thing with inlines? I'm suspicious as to whether it really does or not.

> Source/WebCore/rendering/RenderInline.cpp:1001
> +	      
repaintRect.move(toRenderInline(inlineFlow)->layer()->offsetForInFlowPosition()
);

Agree with Tony, but I would just use your in-flow terminology, e.g.,
inlineFlow->style()->isInFlowPositioned(). isOutOfFlowPositioned() would
probably be good too, since I bet we ask absolute || fixed of the style also.

> Source/WebCore/rendering/RenderInline.cpp:1054
> +	       if ((style()->position() == RelativePosition ||
style()->position() == StickyPosition) && layer())

Make a helper for this. isInFlowPositioned()

> Source/WebCore/rendering/RenderInline.cpp:1083
> +    if ((style()->position() == RelativePosition || style()->position() ==
StickyPosition) && layer()) {

Ditto.