59046 2011-04-20 17:17:19 -0700 With a non-1 page scale, scrolling to reveal selection fails 2011-04-20 20:02:17 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED InRadar P2 Normal --- 1 mitz mitz abarth eric webkit.review.bot oldest_to_newest 389664 0 mitz 2011-04-20 17:17:19 -0700 With a non-1 page scale, scrolling to reveal selection fails 389745 1 mitz 2011-04-20 18:55:07 -0700 <rdar://problem/9095366> 389748 2 90474 mitz 2011-04-20 18:57:04 -0700 Created attachment 90474 Patch 389752 3 90474 mjs 2011-04-20 19:06:10 -0700 Comment on attachment 90474 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90474&action=review r=me > Source/WebCore/rendering/RenderObject.cpp:1164 > if (!v->hasLayer() || !v->layer()->isComposited() || v->layer()->backing()->paintingGoesToWindow()) { > - v->repaintViewRectangle(r, immediate); > + IntRect repaintRectangle = r; > + if (v->hasLayer() && v->layer()->transform() && v->layer()->isComposited() && v->layer()->backing()->paintingGoesToWindow()) > + repaintRectangle = v->layer()->transform()->mapRect(r); > + v->repaintViewRectangle(repaintRectangle, immediate); The logic here is a bit squarely. You test some conditions and then similar or opposite looking ones right inside that if. I wonder if this can be refactored to be more clear, though I have no specific suggestions. 389755 4 mitz 2011-04-20 19:11:19 -0700 (In reply to comment #3) > (From update of attachment 90474 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90474&action=review > > r=me > > > Source/WebCore/rendering/RenderObject.cpp:1164 > > if (!v->hasLayer() || !v->layer()->isComposited() || v->layer()->backing()->paintingGoesToWindow()) { > > - v->repaintViewRectangle(r, immediate); > > + IntRect repaintRectangle = r; > > + if (v->hasLayer() && v->layer()->transform() && v->layer()->isComposited() && v->layer()->backing()->paintingGoesToWindow()) > > + repaintRectangle = v->layer()->transform()->mapRect(r); > > + v->repaintViewRectangle(repaintRectangle, immediate); > > The logic here is a bit squarely. You test some conditions and then similar or opposite looking ones right inside that if. I wonder if this can be refactored to be more clear, though I have no specific suggestions. I can (and will) remove the “ && v->layer()->backing()->paintingGoesToWindow()” part, because it’s redundant, and add a local boolean variable for v->hasLayer() && v->layer()->isComposited() 389757 5 mitz 2011-04-20 19:14:02 -0700 Fixed in r84454. <http://trac.webkit.org/changeset/84454> 389779 6 webkit.review.bot 2011-04-20 20:02:17 -0700 http://trac.webkit.org/changeset/84454 might have broken Qt Linux Release The following tests are not passing: fast/transforms/selection-bounds-in-transformed-view.html 90474 2011-04-20 18:57:04 -0700 2011-04-20 19:06:09 -0700 Patch bug-59046-20110420185703.patch text/plain 6018 mitz SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDg0NDQ5KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjIgQEAKKzIwMTEtMDQtMjAgIERhbiBCZXJu c3RlaW4gIDxtaXR6QGFwcGxlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICA8cmRhcjovL3Byb2JsZW0vOTA5NTM2Nj4gV2l0aCBhIG5vbi0xIHBh Z2Ugc2NhbGUsIHNjcm9sbGluZyB0byByZXZlYWwgc2VsZWN0aW9uIGZhaWxzCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01OTA0NgorCisgICAgICAgIFRl c3Q6IGZhc3QvdHJhbnNmb3Jtcy9zZWxlY3Rpb24tYm91bmRzLWluLXRyYW5zZm9ybWVkLXZpZXcu aHRtbAorCisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlck9iamVjdC5jcHA6CisgICAgICAgIChX ZWJDb3JlOjpSZW5kZXJPYmplY3Q6OnJlcGFpbnRVc2luZ0NvbnRhaW5lcik6IElmIHRoZSByZXBh aW50IGNvbnRhaW5lciBpcyB0aGUgUmVuZGVyVmlldywgYW5kCisgICAgICAgIGl0IGhhcyBhIGNv bXBvc2l0ZWQgbGF5ZXIgdGhhdCBwYWludHMgc3RyYWlnaHQgdG8gdGhlIHdpbmRvdywgdGhlbiB0 cmFuc2xhdGUgZnJvbSB2aWV3IGNvb3JkaW5hdGVzCisgICAgICAgIHRvIHdpbmRvdyBjb29yZGlu YXRlcyBoZXJlLgorICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJWaWV3LmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OlJlbmRlclZpZXc6Om1hcExvY2FsVG9Db250YWluZXIpOiBEbyBub3QgYXBwbHkg b3VyIHRyYW5zZm9ybSBpZiB3ZSBhcmUgdGhlIHBhaW50aW5nIHJvb3QuCisgICAgICAgIChXZWJD b3JlOjpSZW5kZXJWaWV3Ojpjb21wdXRlUmVjdEZvclJlcGFpbnQpOiBEaXR0by4gQXBwbHlpbmcg dGhlIHRyYW5zZm9ybSBoZXJlIHdhcyBjb21wZW5zYXRpbmcKKyAgICAgICAgZm9yIG5vdCBhcHBs eWluZyBpdCBpbiBSZW5kZXJPYmplY3Q6OnJlcGFpbnRVc2luZ0NvbnRhaW5lcigpLCBidXQgZm9y IHB1cnBvc2VzIG90aGVyIHRoYW4gcmVwYWludGluZywKKyAgICAgICAgc3VjaCBhcyBjb21wdXRp bmcgc2VsZWN0aW9uIGJvdW5kcywgdGhpcyBmdW5jdGlvbiB3YXMgcmV0dXJuaW5nIHRoZSB3cm9u ZyByZXN1bHRzLgorCiAyMDExLTA0LTIwICBBbGV4ZXkgTWFyaW5pY2hldiAgPGFtYXJpbmljaGV2 QGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBLZW5uZXRoIFJ1c3NlbGwuCklu ZGV4OiBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmNwcAkocmV2aXNpb24g ODQ0MjcpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmNwcAkod29y a2luZyBjb3B5KQpAQCAtMTE1OCw3ICsxMTU4LDEwIEBAIHZvaWQgUmVuZGVyT2JqZWN0OjpyZXBh aW50VXNpbmdDb250YWluZXIKICAgICBpZiAocmVwYWludENvbnRhaW5lci0+aXNSZW5kZXJWaWV3 KCkpIHsKICAgICAgICAgQVNTRVJUKHJlcGFpbnRDb250YWluZXIgPT0gdik7CiAgICAgICAgIGlm ICghdi0+aGFzTGF5ZXIoKSB8fCAhdi0+bGF5ZXIoKS0+aXNDb21wb3NpdGVkKCkgfHwgdi0+bGF5 ZXIoKS0+YmFja2luZygpLT5wYWludGluZ0dvZXNUb1dpbmRvdygpKSB7Ci0gICAgICAgICAgICB2 LT5yZXBhaW50Vmlld1JlY3RhbmdsZShyLCBpbW1lZGlhdGUpOworICAgICAgICAgICAgSW50UmVj dCByZXBhaW50UmVjdGFuZ2xlID0gcjsKKyAgICAgICAgICAgIGlmICh2LT5oYXNMYXllcigpICYm ICB2LT5sYXllcigpLT50cmFuc2Zvcm0oKSAmJiB2LT5sYXllcigpLT5pc0NvbXBvc2l0ZWQoKSAm JiB2LT5sYXllcigpLT5iYWNraW5nKCktPnBhaW50aW5nR29lc1RvV2luZG93KCkpCisgICAgICAg ICAgICAgICAgcmVwYWludFJlY3RhbmdsZSA9IHYtPmxheWVyKCktPnRyYW5zZm9ybSgpLT5tYXBS ZWN0KHIpOworICAgICAgICAgICAgdi0+cmVwYWludFZpZXdSZWN0YW5nbGUocmVwYWludFJlY3Rh bmdsZSwgaW1tZWRpYXRlKTsKICAgICAgICAgICAgIHJldHVybjsKICAgICAgICAgfQogICAgIH0K SW5kZXg6IFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJWaWV3LmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVmlldy5jcHAJKHJldmlzaW9uIDg0 NDI3KQorKysgU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclZpZXcuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC0xNDAsOSArMTQwLDkgQEAgdm9pZCBSZW5kZXJWaWV3OjptYXBMb2NhbFRvQ29u dGFpbmVyKFJlbgogewogICAgIC8vIElmIGEgY29udGFpbmVyIHdhcyBzcGVjaWZpZWQsIGFuZCB3 YXMgbm90IDAgb3IgdGhlIFJlbmRlclZpZXcsCiAgICAgLy8gdGhlbiB3ZSBzaG91bGQgaGF2ZSBm b3VuZCBpdCBieSBub3cuCi0gICAgQVNTRVJUX1VOVVNFRChyZXBhaW50Q29udGFpbmVyLCAhcmVw YWludENvbnRhaW5lciB8fCByZXBhaW50Q29udGFpbmVyID09IHRoaXMpOworICAgIEFTU0VSVF9B UkcocmVwYWludENvbnRhaW5lciwgIXJlcGFpbnRDb250YWluZXIgfHwgcmVwYWludENvbnRhaW5l ciA9PSB0aGlzKTsKIAotICAgIGlmICh1c2VUcmFuc2Zvcm1zICYmIHNob3VsZFVzZVRyYW5zZm9y bUZyb21Db250YWluZXIoMCkpIHsKKyAgICBpZiAoIXJlcGFpbnRDb250YWluZXIgJiYgdXNlVHJh bnNmb3JtcyAmJiBzaG91bGRVc2VUcmFuc2Zvcm1Gcm9tQ29udGFpbmVyKDApKSB7CiAgICAgICAg IFRyYW5zZm9ybWF0aW9uTWF0cml4IHQ7CiAgICAgICAgIGdldFRyYW5zZm9ybUZyb21Db250YWlu ZXIoMCwgSW50U2l6ZSgpLCB0KTsKICAgICAgICAgdHJhbnNmb3JtU3RhdGUuYXBwbHlUcmFuc2Zv cm0odCk7CkBAIC0yOTksNyArMjk5LDcgQEAgdm9pZCBSZW5kZXJWaWV3Ojpjb21wdXRlUmVjdEZv clJlcGFpbnQoUgogewogICAgIC8vIElmIGEgY29udGFpbmVyIHdhcyBzcGVjaWZpZWQsIGFuZCB3 YXMgbm90IDAgb3IgdGhlIFJlbmRlclZpZXcsCiAgICAgLy8gdGhlbiB3ZSBzaG91bGQgaGF2ZSBm b3VuZCBpdCBieSBub3cuCi0gICAgQVNTRVJUX1VOVVNFRChyZXBhaW50Q29udGFpbmVyLCAhcmVw YWludENvbnRhaW5lciB8fCByZXBhaW50Q29udGFpbmVyID09IHRoaXMpOworICAgIEFTU0VSVF9B UkcocmVwYWludENvbnRhaW5lciwgIXJlcGFpbnRDb250YWluZXIgfHwgcmVwYWludENvbnRhaW5l ciA9PSB0aGlzKTsKIAogICAgIGlmIChwcmludGluZygpKQogICAgICAgICByZXR1cm47CkBAIC0z MTcsNyArMzE3LDcgQEAgdm9pZCBSZW5kZXJWaWV3Ojpjb21wdXRlUmVjdEZvclJlcGFpbnQoUgog ICAgICAgICByZWN0Lm1vdmUobV9mcmFtZVZpZXctPnNjcm9sbFhGb3JGaXhlZFBvc2l0aW9uKCks IG1fZnJhbWVWaWV3LT5zY3JvbGxZRm9yRml4ZWRQb3NpdGlvbigpKTsKICAgICAgICAgCiAgICAg Ly8gQXBwbHkgb3VyIHRyYW5zZm9ybSBpZiB3ZSBoYXZlIG9uZSAoYmVjYXVzZSBvZiBmdWxsIHBh Z2Ugem9vbWluZykuCi0gICAgaWYgKG1fbGF5ZXIgJiYgbV9sYXllci0+dHJhbnNmb3JtKCkpCisg ICAgaWYgKCFyZXBhaW50Q29udGFpbmVyICYmIG1fbGF5ZXIgJiYgbV9sYXllci0+dHJhbnNmb3Jt KCkpCiAgICAgICAgIHJlY3QgPSBtX2xheWVyLT50cmFuc2Zvcm0oKS0+bWFwUmVjdChyZWN0KTsK IH0KIApJbmRleDogTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRl c3RzL0NoYW5nZUxvZwkocmV2aXNpb24gODQ0MzgpCisrKyBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJ KHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwxMyBAQAorMjAxMS0wNC0yMCAgRGFuIEJlcm5zdGVp biAgPG1pdHpAYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS85MDk1MzY2PiBXaXRoIGEgbm9uLTEgcGFnZSBz Y2FsZSwgc2Nyb2xsaW5nIHRvIHJldmVhbCBzZWxlY3Rpb24gZmFpbHMKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU5MDQ2CisKKyAgICAgICAgKiBmYXN0 L3RyYW5zZm9ybXMvc2VsZWN0aW9uLWJvdW5kcy1pbi10cmFuc2Zvcm1lZC12aWV3LWV4cGVjdGVk LnR4dDogQWRkZWQuCisgICAgICAgICogZmFzdC90cmFuc2Zvcm1zL3NlbGVjdGlvbi1ib3VuZHMt aW4tdHJhbnNmb3JtZWQtdmlldy5odG1sOiBBZGRlZC4KKwogMjAxMS0wNC0yMCAgRGlyayBQcmFu a2UgIDxkcHJhbmtlQGNocm9taXVtLm9yZz4KIAogICAgICAgICBVbnJldmlld2VkLCBleHBlY3Rh dGlvbnMgY2hhbmdlLiBQdWxsIGluIGV4cGVjdGF0aW9ucyBmcm9tCkluZGV4OiBMYXlvdXRUZXN0 cy9mYXN0L3RyYW5zZm9ybXMvc2VsZWN0aW9uLWJvdW5kcy1pbi10cmFuc2Zvcm1lZC12aWV3LWV4 cGVjdGVkLnR4dAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9mYXN0L3RyYW5zZm9ybXMvc2Vs ZWN0aW9uLWJvdW5kcy1pbi10cmFuc2Zvcm1lZC12aWV3LWV4cGVjdGVkLnR4dAkocmV2aXNpb24g MCkKKysrIExheW91dFRlc3RzL2Zhc3QvdHJhbnNmb3Jtcy9zZWxlY3Rpb24tYm91bmRzLWluLXRy YW5zZm9ybWVkLXZpZXctZXhwZWN0ZWQudHh0CShyZXZpc2lvbiAwKQpAQCAtMCwwICsxLDMgQEAK K1BBU1MKK3RhcmdldAorCkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L3RyYW5zZm9ybXMvc2VsZWN0 aW9uLWJvdW5kcy1pbi10cmFuc2Zvcm1lZC12aWV3Lmh0bWwKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0 VGVzdHMvZmFzdC90cmFuc2Zvcm1zL3NlbGVjdGlvbi1ib3VuZHMtaW4tdHJhbnNmb3JtZWQtdmll dy5odG1sCShyZXZpc2lvbiAwKQorKysgTGF5b3V0VGVzdHMvZmFzdC90cmFuc2Zvcm1zL3NlbGVj dGlvbi1ib3VuZHMtaW4tdHJhbnNmb3JtZWQtdmlldy5odG1sCShyZXZpc2lvbiAwKQpAQCAtMCww ICsxLDE2IEBACis8Ym9keSBzdHlsZT0iZm9udDogMjBweCBhaGVtOyBtYXJnaW46IDA7Ij4KKyAg ICA8ZGl2IGlkPSJyZXN1bHQiIHN0eWxlPSJvdXRsaW5lOiBzb2xpZCBibHVlOyBoZWlnaHQ6IDEw MHB4OyB3aWR0aDogMTAwcHg7IC13ZWJraXQtdHJhbnNmb3JtOiB0cmFuc2xhdGV6KDApIj48L2Rp dj4KKyAgICA8ZGl2IHN0eWxlPSJoZWlnaHQ6IDkwMHB4Ij48L2Rpdj4KKyAgICA8ZGl2IHN0eWxl PSJoZWlnaHQ6IDEwMHB4OyI+dGFyZ2V0PC9kaXY+CisgICAgPGRpdiBzdHlsZT0iaGVpZ2h0OiAx MDAwcHg7Ij48L2Rpdj4KKyAgICA8c2NyaXB0PgorICAgICAgICBpZiAod2luZG93LmxheW91dFRl c3RDb250cm9sbGVyKSB7CisgICAgICAgICAgICBsYXlvdXRUZXN0Q29udHJvbGxlci5kdW1wQXNU ZXh0KCk7CisKKyAgICAgICAgICAgIGV2ZW50U2VuZGVyLnNjYWxlUGFnZUJ5KDIsIDAsIDApOwor ICAgICAgICB9CisKKyAgICAgICAgZG9jdW1lbnQuZXhlY0NvbW1hbmQoIkZpbmRTdHJpbmciLCBm YWxzZSwgInRhcmdldCIpOworICAgICAgICBkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgicmVzdWx0 IikuaW5uZXJUZXh0ID0gZG9jdW1lbnQuYm9keS5zY3JvbGxUb3AgPT09IDg2NCA/ICJQQVNTIiA6 ICJGQUlMIjsKKyAgICA8L3NjcmlwdD4KKzwvYm9keT4K