Bug 32534

Summary: Confusing, possibly buggy code in RenderLayer::updateScrollInfoAfterLayout()
Product: WebKit Reporter: Peter Kasting <pkasting>
Component: WebCore Misc.Assignee: Peter Kasting <pkasting>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, hyatt, mitz, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Add comment
none
Add comment, v2 none

Description Peter Kasting 2009-12-14 17:01:09 PST
(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.
Comment 1 Darin Adler 2009-12-14 17:14:59 PST
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.
Comment 2 Peter Kasting 2009-12-14 17:18:04 PST
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
Comment 3 Darin Adler 2009-12-14 17:20:40 PST
(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.
Comment 4 mitz 2009-12-14 17:36:34 PST
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).
Comment 5 mitz 2009-12-14 17:42:01 PST
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>
Comment 6 Darin Adler 2009-12-14 17:42:35 PST
Setting a resolution based on Mitz’s comments.

Or we could add a comment explaining this.
Comment 7 Peter Kasting 2009-12-14 17:49:24 PST
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.
Comment 8 Peter Kasting 2009-12-18 17:19:21 PST
Created attachment 45213 [details]
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.
Comment 9 WebKit Review Bot 2009-12-18 17:20:28 PST
Attachment 45213 [details] 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
Comment 10 Peter Kasting 2009-12-18 17:22:25 PST
Created attachment 45214 [details]
Add comment, v2

Fix apostrophe
Comment 11 WebKit Review Bot 2009-12-18 17:25:47 PST
style-queue ran check-webkit-style on attachment 45214 [details] without any errors.
Comment 12 WebKit Commit Bot 2009-12-18 22:40:31 PST
Comment on attachment 45214 [details]
Add comment, v2

Clearing flags on attachment: 45214

Committed r52379: <http://trac.webkit.org/changeset/52379>
Comment 13 WebKit Commit Bot 2009-12-18 22:40:37 PST
All reviewed patches have been landed.  Closing bug.