Bug 108872 – Change computeStickyPositionConstraints to use LayoutBoxExtent for margins

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Emil A Eklund]

Change RenderBoxModelObject::computeStickyPositionConstraints to use a LayoutBoxExtent to represent margins.

[Emil A Eklund]

Created attachment 186482 [details]
Patch

[Levi Weintraub]

Comment on attachment 186482 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186482&action=review

> Source/WebCore/platform/graphics/LayoutSize.h:75
> +    void contract(LayoutUnit width, LayoutUnit height)

This seems like the analogue of "shrunkTo" below. We should be consistent, though I understand this is consistent with LayourRect's naming.

[Emil A Eklund]

(In reply to comment #2)
> (From update of attachment 186482 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186482&action=review
> 
> > Source/WebCore/platform/graphics/LayoutSize.h:75
> > +    void contract(LayoutUnit width, LayoutUnit height)
> 
> This seems like the analogue of "shrunkTo" below. We should be consistent, though I understand this is consistent with LayourRect's naming.

Not quite. shrunkTo shrinks it to the minimum of the current and supplied size. Contract always shrinks it by the supplied delta.

[Levi Weintraub]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 186482 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186482&action=review
> > 
> > > Source/WebCore/platform/graphics/LayoutSize.h:75
> > > +    void contract(LayoutUnit width, LayoutUnit height)
> > 
> > This seems like the analogue of "shrunkTo" below. We should be consistent, though I understand this is consistent with LayourRect's naming.
> 
> Not quite. shrunkTo shrinks it to the minimum of the current and supplied size. Contract always shrinks it by the supplied delta.

I'm not sure if I follow your point. There's expand (always expands) expandTo (max of the two values) and shrunkTo (min of the two values). The missing value would make sense to be called shrunk, not contract. Being internally consistent to LayoutSize makes more sense to me than matching what LayoutRect uses.

[Levi Weintraub]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 186482 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=186482&action=review
> > > 
> > > > Source/WebCore/platform/graphics/LayoutSize.h:75
> > > > +    void contract(LayoutUnit width, LayoutUnit height)
> > > 
> > > This seems like the analogue of "shrunkTo" below. We should be consistent, though I understand this is consistent with LayourRect's naming.
> > 
> > Not quite. shrunkTo shrinks it to the minimum of the current and supplied size. Contract always shrinks it by the supplied delta.
> 
> I'm not sure if I follow your point. There's expand (always expands) expandTo (max of the two values) and shrunkTo (min of the two values). The missing value would make sense to be called shrunk, not contract. Being internally consistent to LayoutSize makes more sense to me than matching what LayoutRect uses.

Sorry, s/shrunk/shrink/

[Emil A Eklund]

(In reply to comment #5)
Ah, I see what you are saying now. Thanks for clarifying.
Renamed contract to shrink.

[Emil A Eklund]

Created attachment 189186 [details]
Patch

[Levi Weintraub]

Comment on attachment 189186 [details]
Patch

I like it :)

[WebKit Review Bot]

Comment on attachment 189186 [details]
Patch

Clearing flags on attachment: 189186

Committed r143410: <http://trac.webkit.org/changeset/143410>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.