81038 2012-03-13 14:49:00 -0700 Update LayoutUnit usage in RenderListBox 2012-03-16 12:07:47 -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 tkent webkit.review.bot oldest_to_newest 577833 0 leviw 2012-03-13 14:49:00 -0700 Rounding paint offsets, snapping painted rects, and rounding mouse coordinates, as laid out in https://trac.webkit.org/wiki/LayoutUnit. 577889 1 131727 leviw 2012-03-13 15:33:15 -0700 Created attachment 131727 Patch 580515 2 131727 jchaffraix 2012-03-16 07:57:09 -0700 Comment on attachment 131727 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131727&action=review > Source/WebCore/ChangeLog:19 > + (WebCore::RenderListBox::panScroll): EventHandler::currentMousePosition is an IntPoint, > + so it doesn't require additional rounding in RenderListBox. It would be nice to make such conversion fail at compile time when you change the type of LayoutUnit to catch such unneeded copying. > Source/WebCore/rendering/RenderListBox.cpp:425 > + paintInfo.context->drawBidiText(itemFont, textRun, roundedIntPoint(r.location())); I was going to ask about why rounding and then I saw that's what pixelSnappedIntRect does. I wonder if we could not make it more obvious that pixelSnappedIntRect is doing a roundedIntPoint on the location... 580702 3 131727 leviw 2012-03-16 11:20:15 -0700 Comment on attachment 131727 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131727&action=review >> Source/WebCore/ChangeLog:19 >> + so it doesn't require additional rounding in RenderListBox. > > It would be nice to make such conversion fail at compile time when you change the type of LayoutUnit to catch such unneeded copying. Making IntPoint->LayoutPoint an explicit conversion would be a good idea, you're right. >> Source/WebCore/rendering/RenderListBox.cpp:425 >> + paintInfo.context->drawBidiText(itemFont, textRun, roundedIntPoint(r.location())); > > I was going to ask about why rounding and then I saw that's what pixelSnappedIntRect does. I wonder if we could not make it more obvious that pixelSnappedIntRect is doing a roundedIntPoint on the location... We could call it pixelSnappedIntPoint, but I feel like that doesn't actually make things clearer... 580770 4 131727 webkit.review.bot 2012-03-16 12:07:42 -0700 Comment on attachment 131727 Patch Clearing flags on attachment: 131727 Committed r111040: <http://trac.webkit.org/changeset/111040> 580771 5 webkit.review.bot 2012-03-16 12:07:47 -0700 All reviewed patches have been landed. Closing bug. 131727 2012-03-13 15:33:15 -0700 2012-03-16 12:07:42 -0700 Patch bug-81038-20120313153314.patch text/plain 3410 leviw SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMDYxNSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIzIEBACisyMDEyLTAzLTEzICBMZXZpIFdl aW50cmF1YiAgPGxldml3QGNocm9taXVtLm9yZz4KKworICAgICAgICBVcGRhdGUgTGF5b3V0VW5p dCB1c2FnZSBpbiBSZW5kZXJMaXN0Qm94CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD04MTAzOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIEJyaW5naW5nIFJlbmRlckxpc3RCb3ggaW4gbGluZSB3aXRoIHRoZSBy dWxlcyBsaXN0ZWQgaW4KKyAgICAgICAgaHR0cHM6Ly90cmFjLndlYmtpdC5vcmcvd2lraS9MYXlv dXRVbml0CisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiBObyBjaGFuZ2UgaW4gYmVoYXZpb3IuCisK KyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyTGlzdEJveC5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpSZW5kZXJMaXN0Qm94OjpwYWludFNjcm9sbGJhcik6IFBpeGVsIHNuYXBwaW5nIGJlZm9yZSBo YW5kaW5nIG9mZiBzY3JvbGxiYXIKKyAgICAgICAgcGFpbnRpbmcgdG8gdGhlIHBsYXRmb3JtLgor ICAgICAgICAoV2ViQ29yZTo6UmVuZGVyTGlzdEJveDo6cGFpbnRJdGVtRm9yZWdyb3VuZCk6IFJv dW5kaW5nIHRoZSBvZmZzZXQgZm9yIHRleHQgcGFpbnRpbmcuCisgICAgICAgIChXZWJDb3JlOjpS ZW5kZXJMaXN0Qm94OjpwYWludEl0ZW1CYWNrZ3JvdW5kKTogUGl4ZWwgc25hcHBpbmcgdGhlIGJh Y2tncm91bmQgcmVjdC4KKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlckxpc3RCb3g6OnBhblNjcm9s bCk6IEV2ZW50SGFuZGxlcjo6Y3VycmVudE1vdXNlUG9zaXRpb24gaXMgYW4gSW50UG9pbnQsCisg ICAgICAgIHNvIGl0IGRvZXNuJ3QgcmVxdWlyZSBhZGRpdGlvbmFsIHJvdW5kaW5nIGluIFJlbmRl ckxpc3RCb3guCisKIDIwMTItMDMtMTMgIE5hdGUgQ2hhcGluICA8amFwaGV0QGNocm9taXVtLm9y Zz4KIAogICAgICAgICBSZXdvcmsgaG93IGEgQ2FjaGVkUmF3UmVzb3VyY2UgZGVjaWRlcyBpZiBp dCBjYW4gYmUgcmV1c2VkCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGlz dEJveC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxp c3RCb3guY3BwCShyZXZpc2lvbiAxMTA2MTIpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcv UmVuZGVyTGlzdEJveC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTM0NSw3ICszNDUsNyBAQCB2b2lk IFJlbmRlckxpc3RCb3g6OmFkZEZvY3VzUmluZ1JlY3RzKFZlCiB2b2lkIFJlbmRlckxpc3RCb3g6 OnBhaW50U2Nyb2xsYmFyKFBhaW50SW5mbyYgcGFpbnRJbmZvLCBjb25zdCBMYXlvdXRQb2ludCYg cGFpbnRPZmZzZXQpCiB7CiAgICAgaWYgKG1fdkJhcikgewotICAgICAgICBMYXlvdXRSZWN0IHNj cm9sbFJlY3QocGFpbnRPZmZzZXQueCgpICsgd2lkdGgoKSAtIGJvcmRlclJpZ2h0KCkgLSBtX3ZC YXItPndpZHRoKCksCisgICAgICAgIEludFJlY3Qgc2Nyb2xsUmVjdCA9IHBpeGVsU25hcHBlZElu dFJlY3QocGFpbnRPZmZzZXQueCgpICsgd2lkdGgoKSAtIGJvcmRlclJpZ2h0KCkgLSBtX3ZCYXIt PndpZHRoKCksCiAgICAgICAgICAgICBwYWludE9mZnNldC55KCkgKyBib3JkZXJUb3AoKSwKICAg ICAgICAgICAgIG1fdkJhci0+d2lkdGgoKSwKICAgICAgICAgICAgIGhlaWdodCgpIC0gKGJvcmRl clRvcCgpICsgYm9yZGVyQm90dG9tKCkpKTsKQEAgLTQyMiw3ICs0MjIsNyBAQCB2b2lkIFJlbmRl ckxpc3RCb3g6OnBhaW50SXRlbUZvcmVncm91bmQoCiAgICAgfQogCiAgICAgLy8gRHJhdyB0aGUg aXRlbSB0ZXh0Ci0gICAgcGFpbnRJbmZvLmNvbnRleHQtPmRyYXdCaWRpVGV4dChpdGVtRm9udCwg dGV4dFJ1biwgci5sb2NhdGlvbigpKTsKKyAgICBwYWludEluZm8uY29udGV4dC0+ZHJhd0JpZGlU ZXh0KGl0ZW1Gb250LCB0ZXh0UnVuLCByb3VuZGVkSW50UG9pbnQoci5sb2NhdGlvbigpKSk7CiB9 CiAKIHZvaWQgUmVuZGVyTGlzdEJveDo6cGFpbnRJdGVtQmFja2dyb3VuZChQYWludEluZm8mIHBh aW50SW5mbywgY29uc3QgTGF5b3V0UG9pbnQmIHBhaW50T2Zmc2V0LCBpbnQgbGlzdEluZGV4KQpA QCAtNDQ0LDcgKzQ0NCw3IEBAIHZvaWQgUmVuZGVyTGlzdEJveDo6cGFpbnRJdGVtQmFja2dyb3Vu ZCgKICAgICAgICAgQ29sb3JTcGFjZSBjb2xvclNwYWNlID0gZWxlbWVudC0+cmVuZGVyU3R5bGUo KSA/IGVsZW1lbnQtPnJlbmRlclN0eWxlKCktPmNvbG9yU3BhY2UoKSA6IHN0eWxlKCktPmNvbG9y U3BhY2UoKTsKICAgICAgICAgTGF5b3V0UmVjdCBpdGVtUmVjdCA9IGl0ZW1Cb3VuZGluZ0JveFJl Y3QocGFpbnRPZmZzZXQsIGxpc3RJbmRleCk7CiAgICAgICAgIGl0ZW1SZWN0LmludGVyc2VjdChj b250cm9sQ2xpcFJlY3QocGFpbnRPZmZzZXQpKTsKLSAgICAgICAgcGFpbnRJbmZvLmNvbnRleHQt PmZpbGxSZWN0KGl0ZW1SZWN0LCBiYWNrQ29sb3IsIGNvbG9yU3BhY2UpOworICAgICAgICBwYWlu dEluZm8uY29udGV4dC0+ZmlsbFJlY3QocGl4ZWxTbmFwcGVkSW50UmVjdChpdGVtUmVjdCksIGJh Y2tDb2xvciwgY29sb3JTcGFjZSk7CiAgICAgfQogfQogCkBAIC00OTAsNyArNDkwLDcgQEAgdm9p ZCBSZW5kZXJMaXN0Qm94OjpwYW5TY3JvbGwoY29uc3QgSW50UAogICAgIC8vIEZJWE1FOiBUaGlz IGRvZXNuJ3Qgd29yayBjb3JyZWN0bHkgd2l0aCB0cmFuc2Zvcm1zLgogICAgIEZsb2F0UG9pbnQg YWJzT2Zmc2V0ID0gbG9jYWxUb0Fic29sdXRlKCk7CiAKLSAgICBJbnRQb2ludCBjdXJyZW50TW91 c2VQb3NpdGlvbiA9IHJvdW5kZWRJbnRQb2ludChmcmFtZSgpLT5ldmVudEhhbmRsZXIoKS0+Y3Vy cmVudE1vdXNlUG9zaXRpb24oKSk7CisgICAgSW50UG9pbnQgY3VycmVudE1vdXNlUG9zaXRpb24g PSBmcmFtZSgpLT5ldmVudEhhbmRsZXIoKS0+Y3VycmVudE1vdXNlUG9zaXRpb24oKTsKICAgICAv LyBXZSBuZWVkIHRvIGNoZWNrIGlmIHRoZSBjdXJyZW50IG1vdXNlIHBvc2l0aW9uIGlzIG91dCBv ZiB0aGUgd2luZG93LiBXaGVuIHRoZSBtb3VzZSBpcyBvdXQgb2YgdGhlIHdpbmRvdywgdGhlIHBv c2l0aW9uIGlzIGluY29oZXJlbnQKICAgICBzdGF0aWMgSW50UG9pbnQgcHJldmlvdXNNb3VzZVBv c2l0aW9uOwogICAgIGlmIChjdXJyZW50TW91c2VQb3NpdGlvbi55KCkgPCAwKQo=