Bug 100796 – Should add FixedPositionViewportConstraints to ScrollingConstraints.h

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

[Beth Dakin]

ScrollingConstraints.h currently contains an abstract class called ViewportConstraints that is intended to encapsulate data and logic required to reposition elements whose layout depends on the viewport rect (positions fixed and sticky), when scrolling and zooming. However, at this time there is only a subclass for sticky. We should add a sub-class for fixed. This is required to get pages with fixes position elements scrolling on the scrolling thread.

[Beth Dakin]

Created attachment 171551 [details]
Patch

[Beth Dakin]

Thanks Simon!

http://trac.webkit.org/changeset/132968

[Darin Adler]

Comment on attachment 171551 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingConstraints.h:54
> +    ViewportConstraints(ViewportConstraints* constraints)

This seems a little strange. It’s like a copy constructor, but takes a pointer instead of a reference. Can we just make this a copy constructor instead? I suspect we could, and we could let the compiler generate it.

> Source/WebCore/page/scrolling/ScrollingConstraints.h:78
> +    FixedPositionViewportConstraints()

If we eliminate the other constructor, replacing it with an automatically generated copy constructor, I believe we can also remove this default constructor and let the compiler generate it.

> Source/WebCore/page/scrolling/ScrollingConstraints.h:81
> +    FixedPositionViewportConstraints(FixedPositionViewportConstraints* constraints)

Same comment about this copy constructor.

> Source/WebCore/page/scrolling/ScrollingConstraints.h:87
> +    virtual ConstraintType constraintType() const OVERRIDE { return FixedPositionConstaint; };

Can this override be private instead of public?

[Beth Dakin]

(In reply to comment #3)
> (From update of attachment 171551 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171551&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingConstraints.h:54
> > +    ViewportConstraints(ViewportConstraints* constraints)
> 
> This seems a little strange. It’s like a copy constructor, but takes a pointer instead of a reference. Can we just make this a copy constructor instead? I suspect we could, and we could let the compiler generate it.
> 
> > Source/WebCore/page/scrolling/ScrollingConstraints.h:78
> > +    FixedPositionViewportConstraints()
> 
> If we eliminate the other constructor, replacing it with an automatically generated copy constructor, I believe we can also remove this default constructor and let the compiler generate it.
> 
> > Source/WebCore/page/scrolling/ScrollingConstraints.h:81
> > +    FixedPositionViewportConstraints(FixedPositionViewportConstraints* constraints)
> 
> Same comment about this copy constructor.
> 
> > Source/WebCore/page/scrolling/ScrollingConstraints.h:87
> > +    virtual ConstraintType constraintType() const OVERRIDE { return FixedPositionConstaint; };
> 
> Can this override be private instead of public?

Sure. I wil address these issues in a follow-up patch.