Bug 108872 - Change computeStickyPositionConstraints to use LayoutBoxExtent for margins
Summary: Change computeStickyPositionConstraints to use LayoutBoxExtent for margins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-04 15:40 PST by Emil A Eklund
Modified: 2013-02-19 16:47 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.47 KB, patch)
2013-02-04 15:44 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (4.44 KB, patch)
2013-02-19 15:35 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2013-02-04 15:40:23 PST
Change RenderBoxModelObject::computeStickyPositionConstraints to use a LayoutBoxExtent to represent margins.
Comment 1 Emil A Eklund 2013-02-04 15:44:00 PST
Created attachment 186482 [details]
Patch
Comment 2 Levi Weintraub 2013-02-14 10:40:05 PST
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.
Comment 3 Emil A Eklund 2013-02-14 13:56:57 PST
(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.
Comment 4 Levi Weintraub 2013-02-19 12:09:39 PST
(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.
Comment 5 Levi Weintraub 2013-02-19 12:09:58 PST
(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/
Comment 6 Emil A Eklund 2013-02-19 15:34:55 PST
(In reply to comment #5)
Ah, I see what you are saying now. Thanks for clarifying.
Renamed contract to shrink.
Comment 7 Emil A Eklund 2013-02-19 15:35:01 PST
Created attachment 189186 [details]
Patch
Comment 8 Levi Weintraub 2013-02-19 15:38:31 PST
Comment on attachment 189186 [details]
Patch

I like it :)
Comment 9 WebKit Review Bot 2013-02-19 16:47:25 PST
Comment on attachment 189186 [details]
Patch

Clearing flags on attachment: 189186

Committed r143410: <http://trac.webkit.org/changeset/143410>
Comment 10 WebKit Review Bot 2013-02-19 16:47:28 PST
All reviewed patches have been landed.  Closing bug.