82185 2012-03-26 03:53:30 -0700 LayoutRepainter: Remove unused constructor parameter and update to LayoutUnits 2012-03-27 03:40:52 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 60318 1 leviw leviw eae eric jchaffraix oldest_to_newest 587587 0 leviw 2012-03-26 03:53:30 -0700 LayoutRepainter has a parameter for oldBounds that is never used, with the old bounds instead being assigned by default from clippedOverflowRectForRepaint. The patch removes this paramater and stores the old bounds and old outline box as LayoutUnits. 587591 1 133769 leviw 2012-03-26 04:00:04 -0700 Created attachment 133769 Patch 588058 2 133769 jchaffraix 2012-03-26 13:20:19 -0700 Comment on attachment 133769 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133769&action=review > Source/WebCore/ChangeLog:9 > + Removing an optional parameter for old bounds in LayoutRepainter's constructor that > + was never used. The old bounds were instead always gleaned from the renderer's That statement is wrong. The parameter was used by SVG when LayoutRepainter was introduced but got removed over time (at least that's what git blame is telling me). Please amend this statement to say that it is unused now. > Source/WebCore/rendering/LayoutRepainter.h:49 > + LayoutRect m_oldBounds; > + LayoutRect m_oldOutlineBox; It's weird to store them as LayoutRect even if they correspond to paint information (ie should be device pixels). Your ChangeLog entry seems to mention detecting sub-pixel layout change, could you explain me how you are going to handle those, especially with respect to the repaint logic? (Where will you do the rounding / snapping? Where will you actually use IntRect?...) 588179 3 133769 leviw 2012-03-26 15:12:07 -0700 Comment on attachment 133769 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133769&action=review >> Source/WebCore/ChangeLog:9 >> + was never used. The old bounds were instead always gleaned from the renderer's > > That statement is wrong. The parameter was used by SVG when LayoutRepainter was introduced but got removed over time (at least that's what git blame is telling me). Please amend this statement to say that it is unused now. Sorry, that was the point I intended to make. Incorrect choice of words. Either way, it's not unnecessary. >> Source/WebCore/rendering/LayoutRepainter.h:49 >> + LayoutRect m_oldOutlineBox; > > It's weird to store them as LayoutRect even if they correspond to paint information (ie should be device pixels). Your ChangeLog entry seems to mention detecting sub-pixel layout change, could you explain me how you are going to handle those, especially with respect to the repaint logic? (Where will you do the rounding / snapping? Where will you actually use IntRect?...) We currently evaluate whether or not to repaint after layout using sub-pixel bounds. This is the safest possible strategy, as we chose not to try and optimize this early. The rects passed to the embedder to repaint are pixel snapped, if that's the question you're asking. 588206 4 133769 eric 2012-03-26 15:40:10 -0700 Comment on attachment 133769 Patch OK. Obviously you should update per Julien's comments. 588297 5 jchaffraix 2012-03-26 16:38:13 -0700 > We currently evaluate whether or not to repaint after layout using sub-pixel bounds. This is the safest possible strategy, as we chose not to try and optimize this early. That sounds like a good comment to add to the LayoutRepainter declaration. I would say, repainting AFAICT only makes sense on a device pixel perspective bit it's better to be conservative here. > The rects passed to the embedder to repaint are pixel snapped, if that's the question you're asking. OK, I think I see what you mean here. Correct me if I am wrong: WebCore repainting logic will always use LayoutRect, snapped before calling our platform to do the invalidation. That sounds very similar to painting: it will be a lazily (late) conversion. I would love 1 line on the wiki about that :-) 588669 6 leviw 2012-03-27 02:16:41 -0700 (In reply to comment #5) > > We currently evaluate whether or not to repaint after layout using sub-pixel bounds. This is the safest possible strategy, as we chose not to try and optimize this early. > > That sounds like a good comment to add to the LayoutRepainter declaration. I would say, repainting AFAICT only makes sense on a device pixel perspective bit it's better to be conservative here. > > > The rects passed to the embedder to repaint are pixel snapped, if that's the question you're asking. > > OK, I think I see what you mean here. Correct me if I am wrong: WebCore repainting logic will always use LayoutRect, snapped before calling our platform to do the invalidation. That sounds very similar to painting: it will be a lazily (late) conversion. I would love 1 line on the wiki about that :-) That's a fantastic idea as usual :) I'll update the changelog and Wiki. Thanks guys! 588710 7 leviw 2012-03-27 03:07:59 -0700 (In reply to comment #5) > > We currently evaluate whether or not to repaint after layout using sub-pixel bounds. This is the safest possible strategy, as we chose not to try and optimize this early. > > That sounds like a good comment to add to the LayoutRepainter declaration. I would say, repainting AFAICT only makes sense on a device pixel perspective bit it's better to be conservative here. > > > The rects passed to the embedder to repaint are pixel snapped, if that's the question you're asking. > > OK, I think I see what you mean here. Correct me if I am wrong: WebCore repainting logic will always use LayoutRect, snapped before calling our platform to do the invalidation. That sounds very similar to painting: it will be a lazily (late) conversion. I would love 1 line on the wiki about that :-) No correction necessary as you're absolutely right. I'll add a comment before landing. 588720 8 133769 leviw 2012-03-27 03:40:39 -0700 Comment on attachment 133769 Patch Landed in http://trac.webkit.org/changeset/112243 133769 2012-03-26 04:00:04 -0700 2012-03-27 03:40:39 -0700 Patch bug-82185-20120326120002.patch text/plain 2930 leviw SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMjA3MykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI0IEBACisyMDEyLTAzLTI2ICBMZXZpIFdl aW50cmF1YiAgPGxldml3QGNocm9taXVtLm9yZz4KKworICAgICAgICBMYXlvdXRSZXBhaW50ZXI6 IFJlbW92ZSB1bnVzZWQgY29uc3RydWN0b3IgcGFyYW1ldGVyIGFuZCB1cGRhdGUgdG8gTGF5b3V0 VW5pdHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTgy MTg1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUmVt b3ZpbmcgYW4gb3B0aW9uYWwgcGFyYW1ldGVyIGZvciBvbGQgYm91bmRzIGluIExheW91dFJlcGFp bnRlcidzIGNvbnN0cnVjdG9yIHRoYXQKKyAgICAgICAgd2FzIG5ldmVyIHVzZWQuIFRoZSBvbGQg Ym91bmRzIHdlcmUgaW5zdGVhZCBhbHdheXMgZ2xlYW5lZCBmcm9tIHRoZSByZW5kZXJlcidzCisg ICAgICAgIGNsaXBwZWRPdmVyZmxvd1JlY3RGb3JSZXBhaW50LgorCisgICAgICAgIFRoZSByZW5k ZXJlcidzIGJvdW5kcyBhbmQgb3V0bGluZSByZWN0IGFsc28gc2hvdWxkIGJlIHN0b3JlZCBpbiBM YXlvdXRVbml0cyB0byBwcm9wZXJseQorICAgICAgICBkZXRlY3Qgc3ViLXBpeGVsIGNoYW5nZXMg ZHVyaW5nIGxheW91dC4gVXBkYXRpbmcgYXMgc3VjaC4KKworICAgICAgICBObyBuZXcgdGVzdHMu IE5vIGNoYW5nZSBpbiBiZWhhdmlvci4KKworICAgICAgICAqIHJlbmRlcmluZy9MYXlvdXRSZXBh aW50ZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6TGF5b3V0UmVwYWludGVyOjpMYXlvdXRSZXBh aW50ZXIpOgorICAgICAgICAqIHJlbmRlcmluZy9MYXlvdXRSZXBhaW50ZXIuaDoKKyAgICAgICAg KExheW91dFJlcGFpbnRlcik6CisKIDIwMTItMDMtMjYgIFl1cnkgU2VtaWtoYXRza3kgIDx5dXJ5 c0BjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgW0Nocm9taXVtXSBXZWIgSW5zcGVjdG9yOiBkZWRp Y2F0ZWQgd29ya2VyIGluc3BlY3RvciBpcyBlbXB0eQpJbmRleDogU291cmNlL1dlYkNvcmUvcmVu ZGVyaW5nL0xheW91dFJlcGFpbnRlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL0xheW91dFJlcGFpbnRlci5jcHAJKHJldmlzaW9uIDExMjA2MikKKysrIFNvdXJj ZS9XZWJDb3JlL3JlbmRlcmluZy9MYXlvdXRSZXBhaW50ZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBA IC0zMCwxNCArMzAsMTQgQEAKIAogbmFtZXNwYWNlIFdlYkNvcmUgewogCi1MYXlvdXRSZXBhaW50 ZXI6OkxheW91dFJlcGFpbnRlcihSZW5kZXJPYmplY3QmIG9iamVjdCwgYm9vbCBjaGVja0ZvclJl cGFpbnQsIGNvbnN0IEludFJlY3QqIG9sZEJvdW5kcykKK0xheW91dFJlcGFpbnRlcjo6TGF5b3V0 UmVwYWludGVyKFJlbmRlck9iamVjdCYgb2JqZWN0LCBib29sIGNoZWNrRm9yUmVwYWludCkKICAg ICA6IG1fb2JqZWN0KG9iamVjdCkKICAgICAsIG1fcmVwYWludENvbnRhaW5lcigwKQogICAgICwg bV9jaGVja0ZvclJlcGFpbnQoY2hlY2tGb3JSZXBhaW50KQogewogICAgIGlmIChtX2NoZWNrRm9y UmVwYWludCkgewogICAgICAgICBtX3JlcGFpbnRDb250YWluZXIgPSBtX29iamVjdC5jb250YWlu ZXJGb3JSZXBhaW50KCk7Ci0gICAgICAgIG1fb2xkQm91bmRzID0gb2xkQm91bmRzID8gKm9sZEJv dW5kcyA6IG1fb2JqZWN0LmNsaXBwZWRPdmVyZmxvd1JlY3RGb3JSZXBhaW50KG1fcmVwYWludENv bnRhaW5lcik7CisgICAgICAgIG1fb2xkQm91bmRzID0gbV9vYmplY3QuY2xpcHBlZE92ZXJmbG93 UmVjdEZvclJlcGFpbnQobV9yZXBhaW50Q29udGFpbmVyKTsKICAgICAgICAgbV9vbGRPdXRsaW5l Qm94ID0gbV9vYmplY3Qub3V0bGluZUJvdW5kc0ZvclJlcGFpbnQobV9yZXBhaW50Q29udGFpbmVy KTsKICAgICB9CiB9CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvTGF5b3V0UmVwYWlu dGVyLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL0xheW91dFJlcGFp bnRlci5oCShyZXZpc2lvbiAxMTIwNjIpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvTGF5 b3V0UmVwYWludGVyLmgJKHdvcmtpbmcgY29weSkKQEAgLTM1LDcgKzM1LDcgQEAgY2xhc3MgUmVu ZGVyQm94TW9kZWxPYmplY3Q7CiAKIGNsYXNzIExheW91dFJlcGFpbnRlciB7CiBwdWJsaWM6Ci0g ICAgTGF5b3V0UmVwYWludGVyKFJlbmRlck9iamVjdCYsIGJvb2wgY2hlY2tGb3JSZXBhaW50LCBj b25zdCBJbnRSZWN0KiBvbGRCb3VuZHMgPSAwKTsKKyAgICBMYXlvdXRSZXBhaW50ZXIoUmVuZGVy T2JqZWN0JiwgYm9vbCBjaGVja0ZvclJlcGFpbnQpOwogCiAgICAgYm9vbCBjaGVja0ZvclJlcGFp bnQoKSBjb25zdCB7IHJldHVybiBtX2NoZWNrRm9yUmVwYWludDsgfQogCkBAIC00NSw4ICs0NSw4 IEBAIHB1YmxpYzoKIHByaXZhdGU6CiAgICAgUmVuZGVyT2JqZWN0JiBtX29iamVjdDsKICAgICBS ZW5kZXJCb3hNb2RlbE9iamVjdCogbV9yZXBhaW50Q29udGFpbmVyOwotICAgIEludFJlY3QgbV9v bGRCb3VuZHM7Ci0gICAgSW50UmVjdCBtX29sZE91dGxpbmVCb3g7CisgICAgTGF5b3V0UmVjdCBt X29sZEJvdW5kczsKKyAgICBMYXlvdXRSZWN0IG1fb2xkT3V0bGluZUJveDsKICAgICBib29sIG1f Y2hlY2tGb3JSZXBhaW50OwogfTsKIAo=