100796 2012-10-30 16:53:52 -0700 Should add FixedPositionViewportConstraints to ScrollingConstraints.h 2012-11-01 10:22:08 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 bdakin bdakin andersca bdakin jamesr simon.fraser thorton tonikitoo webkit.review.bot oldest_to_newest 754797 0 bdakin 2012-10-30 16:53:52 -0700 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. 754799 1 171551 bdakin 2012-10-30 16:57:52 -0700 Created attachment 171551 Patch 754817 2 bdakin 2012-10-30 17:18:53 -0700 Thanks Simon! http://trac.webkit.org/changeset/132968 756381 3 171551 darin 2012-11-01 09:59:28 -0700 Comment on attachment 171551 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? 756412 4 bdakin 2012-11-01 10:22:08 -0700 (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. 171551 2012-10-30 16:57:52 -0700 2012-11-01 09:59:28 -0700 Patch constraints-for-review.txt text/plain 5522 bdakin SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzMjk2NCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDMzIEBACisyMDEyLTEwLTMwICBCZXRoIERh a2luICA8YmRha2luQGFwcGxlLmNvbT4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MTAwNzk2CisgICAgICAgIFNob3VsZCBhZGQgRml4ZWRQb3NpdGlv blZpZXdwb3J0Q29uc3RyYWludHMgdG8gU2Nyb2xsaW5nQ29uc3RyYWludHMuaAorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFNjcm9sbGluZ0NvbnN0cmFp bnRzLmggY3VycmVudGx5IGNvbnRhaW5zIGFuIGFic3RyYWN0IGNsYXNzIGNhbGxlZCAKKyAgICAg ICAgVmlld3BvcnRDb25zdHJhaW50cyB0aGF0IGlzIGludGVuZGVkIHRvIGVuY2Fwc3VsYXRlIGRh dGEgYW5kIGxvZ2ljIAorICAgICAgICByZXF1aXJlZCB0byByZXBvc2l0aW9uIGVsZW1lbnRzIHdo b3NlIGxheW91dCBkZXBlbmRzIG9uIHRoZSB2aWV3cG9ydCAKKyAgICAgICAgcmVjdCAocG9zaXRp b25zIGZpeGVkIGFuZCBzdGlja3kpLCB3aGVuIHNjcm9sbGluZyBhbmQgem9vbWluZy4gCisgICAg ICAgIEhvd2V2ZXIsIGF0IHRoaXMgdGltZSB0aGVyZSBpcyBvbmx5IGEgc3ViY2xhc3MgZm9yIHN0 aWNreS4gV2Ugc2hvdWxkIAorICAgICAgICBhZGQgYSBzdWItY2xhc3MgZm9yIGZpeGVkLiBUaGlz IGlzIHJlcXVpcmVkIHRvIGdldCBwYWdlcyB3aXRoIGZpeGVkIAorICAgICAgICBwb3NpdGlvbiBl bGVtZW50cyBzY3JvbGxpbmcgb24gdGhlIHNjcm9sbGluZyB0aHJlYWQuCisKKyAgICAgICAgKiBw YWdlL3Njcm9sbGluZy9TY3JvbGxpbmdDb25zdHJhaW50cy5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpGaXhlZFBvc2l0aW9uVmlld3BvcnRDb25zdHJhaW50czo6bGF5ZXJQb3NpdGlvbkZvclZpZXdw b3J0UmVjdCk6CisgICAgICAgIChXZWJDb3JlKToKKyAgICAgICAgKiBwYWdlL3Njcm9sbGluZy9T Y3JvbGxpbmdDb25zdHJhaW50cy5oOgorICAgICAgICAoV2ViQ29yZTo6Vmlld3BvcnRDb25zdHJh aW50czo6Vmlld3BvcnRDb25zdHJhaW50cyk6CisgICAgICAgIChWaWV3cG9ydENvbnN0cmFpbnRz KToKKyAgICAgICAgKFdlYkNvcmU6OlZpZXdwb3J0Q29uc3RyYWludHM6OnNldEFuY2hvckVkZ2Vz KToKKyAgICAgICAgKEZpeGVkUG9zaXRpb25WaWV3cG9ydENvbnN0cmFpbnRzKToKKyAgICAgICAg KFdlYkNvcmU6OkZpeGVkUG9zaXRpb25WaWV3cG9ydENvbnN0cmFpbnRzOjpGaXhlZFBvc2l0aW9u Vmlld3BvcnRDb25zdHJhaW50cyk6CisgICAgICAgIChXZWJDb3JlOjpGaXhlZFBvc2l0aW9uVmll d3BvcnRDb25zdHJhaW50czo6dmlld3BvcnRSZWN0QXRMYXN0TGF5b3V0KToKKyAgICAgICAgKFdl YkNvcmU6OkZpeGVkUG9zaXRpb25WaWV3cG9ydENvbnN0cmFpbnRzOjpzZXRWaWV3cG9ydFJlY3RB dExhc3RMYXlvdXQpOgorICAgICAgICAoV2ViQ29yZTo6Rml4ZWRQb3NpdGlvblZpZXdwb3J0Q29u c3RyYWludHM6OmxheWVyUG9zaXRpb25BdExhc3RMYXlvdXQpOgorICAgICAgICAoV2ViQ29yZTo6 Rml4ZWRQb3NpdGlvblZpZXdwb3J0Q29uc3RyYWludHM6OnNldExheWVyUG9zaXRpb25BdExhc3RM YXlvdXQpOgorICAgICAgICAoV2ViQ29yZSk6CisKIDIwMTItMTAtMzAgIE5pY28gV2ViZXIgIDx0 aGFraXNAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIEZpeCBwb3RlbnRpYWwgb3ZlcmZsb3cgaW4g anBlZyBleGlmIHJlYWRlci4gRm91bmQgYnkgYWVkbGFAZ29vZ2xlLmNvbS4KSW5kZXg6IFNvdXJj ZS9XZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5nL1Njcm9sbGluZ0NvbnN0cmFpbnRzLmNwcAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9wYWdlL3Njcm9sbGluZy9TY3JvbGxpbmdDb25zdHJhaW50 cy5jcHAJKHJldmlzaW9uIDEzMjk0NykKKysrIFNvdXJjZS9XZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5n L1Njcm9sbGluZ0NvbnN0cmFpbnRzLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjgsNiArMjgsMjMg QEAKIAogbmFtZXNwYWNlIFdlYkNvcmUgewogCitGbG9hdFBvaW50IEZpeGVkUG9zaXRpb25WaWV3 cG9ydENvbnN0cmFpbnRzOjpsYXllclBvc2l0aW9uRm9yVmlld3BvcnRSZWN0KGNvbnN0IEZsb2F0 UmVjdCYgdmlld3BvcnRSZWN0KSBjb25zdAoreworICAgIEZsb2F0U2l6ZSBvZmZzZXQ7CisKKyAg ICBpZiAoaGFzQW5jaG9yRWRnZShBbmNob3JFZGdlTGVmdCkpCisgICAgICAgIG9mZnNldC5zZXRX aWR0aCh2aWV3cG9ydFJlY3QueCgpIC0gbV92aWV3cG9ydFJlY3RBdExhc3RMYXlvdXQueCgpKTsK KyAgICBlbHNlIGlmIChoYXNBbmNob3JFZGdlKEFuY2hvckVkZ2VSaWdodCkpCisgICAgICAgIG9m ZnNldC5zZXRXaWR0aCh2aWV3cG9ydFJlY3QubWF4WCgpIC0gbV92aWV3cG9ydFJlY3RBdExhc3RM YXlvdXQubWF4WCgpKTsKKworICAgIGlmIChoYXNBbmNob3JFZGdlKEFuY2hvckVkZ2VUb3ApKQor ICAgICAgICBvZmZzZXQuc2V0SGVpZ2h0KHZpZXdwb3J0UmVjdC55KCkgLSBtX3ZpZXdwb3J0UmVj dEF0TGFzdExheW91dC55KCkpOworICAgIGVsc2UgaWYgKGhhc0FuY2hvckVkZ2UoQW5jaG9yRWRn ZUJvdHRvbSkpCisgICAgICAgIG9mZnNldC5zZXRIZWlnaHQodmlld3BvcnRSZWN0Lm1heFkoKSAt IG1fdmlld3BvcnRSZWN0QXRMYXN0TGF5b3V0Lm1heFkoKSk7CisKKyAgICByZXR1cm4gbV9sYXll clBvc2l0aW9uQXRMYXN0TGF5b3V0ICsgb2Zmc2V0OworfQorCiBGbG9hdFNpemUgU3RpY2t5UG9z aXRpb25WaWV3cG9ydENvbnN0cmFpbnRzOjpjb21wdXRlU3RpY2t5T2Zmc2V0KGNvbnN0IEZsb2F0 UmVjdCYgdmlld3BvcnRSZWN0KSBjb25zdAogewogICAgIEZsb2F0UmVjdCBib3hSZWN0ID0gbV9h YnNvbHV0ZVN0aWNreUJveFJlY3Q7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wYWdlL3Njcm9sbGlu Zy9TY3JvbGxpbmdDb25zdHJhaW50cy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3Bh Z2Uvc2Nyb2xsaW5nL1Njcm9sbGluZ0NvbnN0cmFpbnRzLmgJKHJldmlzaW9uIDEzMjk0NykKKysr IFNvdXJjZS9XZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5nL1Njcm9sbGluZ0NvbnN0cmFpbnRzLmgJKHdv cmtpbmcgY29weSkKQEAgLTUxLDYgKzUxLDExIEBAIHB1YmxpYzoKICAgICAgICAgOiBtX2FuY2hv ckVkZ2VzKDApCiAgICAgeyB9CiAKKyAgICBWaWV3cG9ydENvbnN0cmFpbnRzKFZpZXdwb3J0Q29u c3RyYWludHMqIGNvbnN0cmFpbnRzKQorICAgICAgICA6IG1fYWxpZ25tZW50T2Zmc2V0KGNvbnN0 cmFpbnRzLT5hbGlnbm1lbnRPZmZzZXQoKSkKKyAgICAgICAgLCBtX2FuY2hvckVkZ2VzKGNvbnN0 cmFpbnRzLT5hbmNob3JFZGdlcygpKQorICAgIHsgfQorCiAgICAgdmlydHVhbCB+Vmlld3BvcnRD b25zdHJhaW50cygpIHsgfQogICAgIAogICAgIHZpcnR1YWwgQ29uc3RyYWludFR5cGUgY29uc3Ry YWludFR5cGUoKSBjb25zdCA9IDA7CkBAIC01OCw2ICs2Myw3IEBAIHB1YmxpYzoKICAgICBBbmNo b3JFZGdlcyBhbmNob3JFZGdlcygpIGNvbnN0IHsgcmV0dXJuIG1fYW5jaG9yRWRnZXM7IH0KICAg ICBib29sIGhhc0FuY2hvckVkZ2UoQW5jaG9yRWRnZUZsYWdzIGZsYWcpIGNvbnN0IHsgcmV0dXJu IG1fYW5jaG9yRWRnZXMgJiBmbGFnOyB9CiAgICAgdm9pZCBhZGRBbmNob3JFZGdlKEFuY2hvckVk Z2VGbGFncyBlZGdlRmxhZykgeyBtX2FuY2hvckVkZ2VzIHw9IGVkZ2VGbGFnOyB9CisgICAgdm9p ZCBzZXRBbmNob3JFZGdlcyhBbmNob3JFZGdlcyBlZGdlcykgeyBtX2FuY2hvckVkZ2VzID0gZWRn ZXM7IH0KICAgICAKICAgICBGbG9hdFNpemUgYWxpZ25tZW50T2Zmc2V0KCkgY29uc3QgeyByZXR1 cm4gbV9hbGlnbm1lbnRPZmZzZXQ7IH0KICAgICB2b2lkIHNldEFsaWdubWVudE9mZnNldChjb25z dCBGbG9hdFNpemUmIG9mZnNldCkgeyBtX2FsaWdubWVudE9mZnNldCA9IG9mZnNldDsgfQpAQCAt NjcsNiArNzMsMzIgQEAgcHJvdGVjdGVkOgogICAgIEFuY2hvckVkZ2VzIG1fYW5jaG9yRWRnZXM7 CiB9OwogCitjbGFzcyBGaXhlZFBvc2l0aW9uVmlld3BvcnRDb25zdHJhaW50cyA6IHB1YmxpYyBW aWV3cG9ydENvbnN0cmFpbnRzIHsKK3B1YmxpYzoKKyAgICBGaXhlZFBvc2l0aW9uVmlld3BvcnRD b25zdHJhaW50cygpCisgICAgeyB9CisKKyAgICBGaXhlZFBvc2l0aW9uVmlld3BvcnRDb25zdHJh aW50cyhGaXhlZFBvc2l0aW9uVmlld3BvcnRDb25zdHJhaW50cyogY29uc3RyYWludHMpCisgICAg ICAgIDogVmlld3BvcnRDb25zdHJhaW50cyhjb25zdHJhaW50cykKKyAgICAgICAgLCBtX3ZpZXdw b3J0UmVjdEF0TGFzdExheW91dChjb25zdHJhaW50cy0+dmlld3BvcnRSZWN0QXRMYXN0TGF5b3V0 KCkpCisgICAgICAgICwgbV9sYXllclBvc2l0aW9uQXRMYXN0TGF5b3V0KGNvbnN0cmFpbnRzLT5s YXllclBvc2l0aW9uQXRMYXN0TGF5b3V0KCkpCisgICAgeyB9CisKKyAgICB2aXJ0dWFsIENvbnN0 cmFpbnRUeXBlIGNvbnN0cmFpbnRUeXBlKCkgY29uc3QgT1ZFUlJJREUgeyByZXR1cm4gRml4ZWRQ b3NpdGlvbkNvbnN0YWludDsgfTsKKworICAgIEZsb2F0UG9pbnQgbGF5ZXJQb3NpdGlvbkZvclZp ZXdwb3J0UmVjdChjb25zdCBGbG9hdFJlY3QmIHZpZXdwb3J0UmVjdCkgY29uc3Q7CisKKyAgICBj b25zdCBGbG9hdFJlY3QmIHZpZXdwb3J0UmVjdEF0TGFzdExheW91dCgpIGNvbnN0IHsgcmV0dXJu IG1fdmlld3BvcnRSZWN0QXRMYXN0TGF5b3V0OyB9CisgICAgdm9pZCBzZXRWaWV3cG9ydFJlY3RB dExhc3RMYXlvdXQoY29uc3QgRmxvYXRSZWN0JiByZWN0KSB7IG1fdmlld3BvcnRSZWN0QXRMYXN0 TGF5b3V0ID0gcmVjdDsgfQorCisgICAgY29uc3QgRmxvYXRQb2ludCYgbGF5ZXJQb3NpdGlvbkF0 TGFzdExheW91dCgpIGNvbnN0IHsgcmV0dXJuIG1fbGF5ZXJQb3NpdGlvbkF0TGFzdExheW91dDsg fQorICAgIHZvaWQgc2V0TGF5ZXJQb3NpdGlvbkF0TGFzdExheW91dChjb25zdCBGbG9hdFBvaW50 JiBwb2ludCkgeyBtX2xheWVyUG9zaXRpb25BdExhc3RMYXlvdXQgPSBwb2ludDsgfQorCitwcml2 YXRlOgorICAgIEZsb2F0UmVjdCBtX3ZpZXdwb3J0UmVjdEF0TGFzdExheW91dDsKKyAgICBGbG9h dFBvaW50IG1fbGF5ZXJQb3NpdGlvbkF0TGFzdExheW91dDsKK307CisKIGNsYXNzIFN0aWNreVBv c2l0aW9uVmlld3BvcnRDb25zdHJhaW50cyA6IHB1YmxpYyBWaWV3cG9ydENvbnN0cmFpbnRzIHsK IHB1YmxpYzoKICAgICBTdGlja3lQb3NpdGlvblZpZXdwb3J0Q29uc3RyYWludHMoKQo=