32534 2009-12-14 17:01:09 -0800 Confusing, possibly buggy code in RenderLayer::updateScrollInfoAfterLayout() 2009-12-18 22:40:37 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 1 pkasting pkasting commit-queue darin hyatt mitz simon.fraser webkit.review.bot oldest_to_newest 171630 0 pkasting 2009-12-14 17:01:09 -0800 (Mentioned this to smfr on IRC, he didn't know what to make of it either and suggested filing this bug) At the bottom of RenderLayer::updateScrollInfoAfterLayout() is some code that resets the horizontal and vertical scrollbars. The blocks are identical, except that the horizontal scrollbar gets a setValue() call and the vertical scrollbar doesn't. Is this a bug? If not can we comment about why? This dates from 2005, when darin checked in r11521 to fix bug 5826. Before this change, neither block had a setValue(); the change added the setValue() call to just the horizontal scrollbar block. 171633 1 darin 2009-12-14 17:14:59 -0800 If we can find a symptom, then we can fix the bug. I don’t think we should change any code based on just seeing it’s different. I’ll look at the patch and see if I can remember anything from the distant past. 171635 2 pkasting 2009-12-14 17:18:04 -0800 Yes, I wish I knew what this code did so I could figure out how to test this codepath. Of course, if I knew that much, I could just say whether the code was right :P 171637 3 darin 2009-12-14 17:20:40 -0800 (In reply to comment #0) > This dates from 2005, when darin checked in r11521 to fix bug 5826. Note that the change r11521 was done by Mitz Pettel. I was just the committer. Back then I was the commit bot. 171641 4 mitz 2009-12-14 17:36:34 -0800 This code ensures that when a right-to-left scrollable area’s width (or content width) changes, the top right corner of the content doesn’t shift with respect to the top right corner of the area. Conceptually, right-to-left areas have their origin at the top-right, but RenderLayer is top-left oriented, so this is needed to keep everything working (see how scrollXOffset() differs from srollYOffsset() to get an idea of why the horizontal and vertical scrollbars need to be treated differently). 171644 5 mitz 2009-12-14 17:42:01 -0800 I’m pretty sure there are regression tests that cover this already, but here’s the simplest one: <div style="direction: rtl; width: 100px; overflow: auto;"> <div id="target" style="width: 150px; height: 100px;"></div> </div> 171645 6 darin 2009-12-14 17:42:35 -0800 Setting a resolution based on Mitz’s comments. Or we could add a comment explaining this. 171646 7 pkasting 2009-12-14 17:49:24 -0800 It'd be awesome to add a comment in the code. All my checkouts are in use at the moment, but if no one gets to it before me I'll add something like comment 4. 173430 8 45213 pkasting 2009-12-18 17:19:21 -0800 Created attachment 45213 Add comment This patch basically adds mitz' comment verbatim into the code. I think this is really useful when trying to figure out why the code does what it does. 173431 9 webkit.review.bot 2009-12-18 17:20:28 -0800 Attachment 45213 did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/rendering/RenderLayer.cpp:1895: Line contains invalid UTF-8 (or Unicode replacement character). [readability/utf8] [5] Total errors found: 1 173432 10 45214 pkasting 2009-12-18 17:22:25 -0800 Created attachment 45214 Add comment, v2 Fix apostrophe 173433 11 webkit.review.bot 2009-12-18 17:25:47 -0800 style-queue ran check-webkit-style on attachment 45214 without any errors. 173481 12 45214 commit-queue 2009-12-18 22:40:31 -0800 Comment on attachment 45214 Add comment, v2 Clearing flags on attachment: 45214 Committed r52379: <http://trac.webkit.org/changeset/52379> 173482 13 commit-queue 2009-12-18 22:40:37 -0800 All reviewed patches have been landed. Closing bug. 45213 2009-12-18 17:19:21 -0800 2009-12-18 17:22:25 -0800 Add comment patch text/plain 1834 pkasting SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9DaGFuZ2VM b2cJKHJldmlzaW9uIDUyMzY2KQorKysgV2ViQ29yZS9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkK QEAgLTEsMyArMSwxNCBAQAorMjAwOS0xMi0xOCAgUGV0ZXIgS2FzdGluZyAgPHBrYXN0aW5nQGdv b2dsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgQWRkIGEgY29tbWVudCBhYm91dCBzb21lIGNvbmZ1c2luZyBjb2RlIHRoYXQgb3RoZXJ3aXNl IHdvdWxkIGxvb2sgbGlrZQorICAgICAgICBpdCBtaWdodCBiZSBhIGJ1Zy4KKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTMyNTM0CisKKyAgICAgICAgKiBy ZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyTGF5ZXI6 OnVwZGF0ZVNjcm9sbEluZm9BZnRlckxheW91dCk6CisKIDIwMDktMTItMTggIEpvbiBIb25leWN1 dHQgIDxqaG9uZXljdXR0QGFwcGxlLmNvbT4KIAogICAgICAgICBSRUdSRVNTSU9OKHI1MjIzMyk6 IE1TQUE6IEFjY2Vzc2liaWxpdHkgcm9sZSBvZiBsaXN0cyBpcyB3cm9uZwpJbmRleDogV2ViQ29y ZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9yZW5k ZXJpbmcvUmVuZGVyTGF5ZXIuY3BwCShyZXZpc2lvbiA1MjM1NCkKKysrIFdlYkNvcmUvcmVuZGVy aW5nL1JlbmRlckxheWVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTg5MCw2ICsxODkwLDE0IEBA IFJlbmRlckxheWVyOjp1cGRhdGVTY3JvbGxJbmZvQWZ0ZXJMYXlvdXQKICAgICAgICAgaWYgKHBh Z2VTdGVwIDwgMCkgcGFnZVN0ZXAgPSBjbGllbnRXaWR0aDsKICAgICAgICAgbV9oQmFyLT5zZXRT dGVwcyhjU2Nyb2xsYmFyUGl4ZWxzUGVyTGluZVN0ZXAsIHBhZ2VTdGVwKTsKICAgICAgICAgbV9o QmFyLT5zZXRQcm9wb3J0aW9uKGNsaWVudFdpZHRoLCBtX3Njcm9sbFdpZHRoKTsKKyAgICAgICAg Ly8gRXhwbGljaXRseSBzZXQgdGhlIGhvcml6b250YWwgc2Nyb2xsIHZhbHVlLiAgVGhpcyBlbnN1 cmVzIHRoYXQgd2hlbiBhCisgICAgICAgIC8vIHJpZ2h0LXRvLWxlZnQgc2Nyb2xsYWJsZSBhcmVh J3Mgd2lkdGggKG9yIGNvbnRlbnQgd2lkdGgpIGNoYW5nZXMsIHRoZQorICAgICAgICAvLyB0b3Ag cmlnaHQgY29ybmVyIG9mIHRoZSBjb250ZW50IGRvZXNuknQgc2hpZnQgd2l0aCByZXNwZWN0IHRv IHRoZSB0b3AKKyAgICAgICAgLy8gcmlnaHQgY29ybmVyIG9mIHRoZSBhcmVhLiBDb25jZXB0dWFs bHksIHJpZ2h0LXRvLWxlZnQgYXJlYXMgaGF2ZQorICAgICAgICAvLyB0aGVpciBvcmlnaW4gYXQg dGhlIHRvcC1yaWdodCwgYnV0IFJlbmRlckxheWVyIGlzIHRvcC1sZWZ0IG9yaWVudGVkLAorICAg ICAgICAvLyBzbyB0aGlzIGlzIG5lZWRlZCB0byBrZWVwIGV2ZXJ5dGhpbmcgd29ya2luZyAoc2Vl IGhvdyBzY3JvbGxYT2Zmc2V0KCkKKyAgICAgICAgLy8gZGlmZmVycyBmcm9tIHNjcm9sbFlPZmZz ZXQoKSB0byBnZXQgYW4gaWRlYSBvZiB3aHkgdGhlIGhvcml6b250YWwgYW5kCisgICAgICAgIC8v IHZlcnRpY2FsIHNjcm9sbGJhcnMgbmVlZCB0byBiZSB0cmVhdGVkIGRpZmZlcmVudGx5KS4KICAg ICAgICAgbV9oQmFyLT5zZXRWYWx1ZShzY3JvbGxYT2Zmc2V0KCkpOwogICAgIH0KICAgICBpZiAo bV92QmFyKSB7Cg== 45214 2009-12-18 17:22:25 -0800 2009-12-18 22:40:31 -0800 Add comment, v2 patch text/plain 1834 pkasting SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9DaGFuZ2VM b2cJKHJldmlzaW9uIDUyMzY2KQorKysgV2ViQ29yZS9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkK QEAgLTEsMyArMSwxNCBAQAorMjAwOS0xMi0xOCAgUGV0ZXIgS2FzdGluZyAgPHBrYXN0aW5nQGdv b2dsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgQWRkIGEgY29tbWVudCBhYm91dCBzb21lIGNvbmZ1c2luZyBjb2RlIHRoYXQgb3RoZXJ3aXNl IHdvdWxkIGxvb2sgbGlrZQorICAgICAgICBpdCBtaWdodCBiZSBhIGJ1Zy4KKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTMyNTM0CisKKyAgICAgICAgKiBy ZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyTGF5ZXI6 OnVwZGF0ZVNjcm9sbEluZm9BZnRlckxheW91dCk6CisKIDIwMDktMTItMTggIEpvbiBIb25leWN1 dHQgIDxqaG9uZXljdXR0QGFwcGxlLmNvbT4KIAogICAgICAgICBSRUdSRVNTSU9OKHI1MjIzMyk6 IE1TQUE6IEFjY2Vzc2liaWxpdHkgcm9sZSBvZiBsaXN0cyBpcyB3cm9uZwpJbmRleDogV2ViQ29y ZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gV2ViQ29yZS9yZW5k ZXJpbmcvUmVuZGVyTGF5ZXIuY3BwCShyZXZpc2lvbiA1MjM1NCkKKysrIFdlYkNvcmUvcmVuZGVy aW5nL1JlbmRlckxheWVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTg5MCw2ICsxODkwLDE0IEBA IFJlbmRlckxheWVyOjp1cGRhdGVTY3JvbGxJbmZvQWZ0ZXJMYXlvdXQKICAgICAgICAgaWYgKHBh Z2VTdGVwIDwgMCkgcGFnZVN0ZXAgPSBjbGllbnRXaWR0aDsKICAgICAgICAgbV9oQmFyLT5zZXRT dGVwcyhjU2Nyb2xsYmFyUGl4ZWxzUGVyTGluZVN0ZXAsIHBhZ2VTdGVwKTsKICAgICAgICAgbV9o QmFyLT5zZXRQcm9wb3J0aW9uKGNsaWVudFdpZHRoLCBtX3Njcm9sbFdpZHRoKTsKKyAgICAgICAg Ly8gRXhwbGljaXRseSBzZXQgdGhlIGhvcml6b250YWwgc2Nyb2xsIHZhbHVlLiAgVGhpcyBlbnN1 cmVzIHRoYXQgd2hlbiBhCisgICAgICAgIC8vIHJpZ2h0LXRvLWxlZnQgc2Nyb2xsYWJsZSBhcmVh J3Mgd2lkdGggKG9yIGNvbnRlbnQgd2lkdGgpIGNoYW5nZXMsIHRoZQorICAgICAgICAvLyB0b3Ag cmlnaHQgY29ybmVyIG9mIHRoZSBjb250ZW50IGRvZXNuJ3Qgc2hpZnQgd2l0aCByZXNwZWN0IHRv IHRoZSB0b3AKKyAgICAgICAgLy8gcmlnaHQgY29ybmVyIG9mIHRoZSBhcmVhLiBDb25jZXB0dWFs bHksIHJpZ2h0LXRvLWxlZnQgYXJlYXMgaGF2ZQorICAgICAgICAvLyB0aGVpciBvcmlnaW4gYXQg dGhlIHRvcC1yaWdodCwgYnV0IFJlbmRlckxheWVyIGlzIHRvcC1sZWZ0IG9yaWVudGVkLAorICAg ICAgICAvLyBzbyB0aGlzIGlzIG5lZWRlZCB0byBrZWVwIGV2ZXJ5dGhpbmcgd29ya2luZyAoc2Vl IGhvdyBzY3JvbGxYT2Zmc2V0KCkKKyAgICAgICAgLy8gZGlmZmVycyBmcm9tIHNjcm9sbFlPZmZz ZXQoKSB0byBnZXQgYW4gaWRlYSBvZiB3aHkgdGhlIGhvcml6b250YWwgYW5kCisgICAgICAgIC8v IHZlcnRpY2FsIHNjcm9sbGJhcnMgbmVlZCB0byBiZSB0cmVhdGVkIGRpZmZlcmVudGx5KS4KICAg ICAgICAgbV9oQmFyLT5zZXRWYWx1ZShzY3JvbGxYT2Zmc2V0KCkpOwogICAgIH0KICAgICBpZiAo bV92QmFyKSB7Cg==