Bug 32534 – Confusing, possibly buggy code in RenderLayer::updateScrollInfoAfterLayout()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Peter Kasting]

(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.

[Darin Adler]

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.

[Peter Kasting]

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

[Darin Adler]

(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.

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).

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>

[Darin Adler]

Setting a resolution based on Mitz’s comments.

Or we could add a comment explaining this.

[Peter Kasting]

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.

[Peter Kasting]

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.

[WebKit Review Bot]

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

[Peter Kasting]

Created attachment 45214 [details]
Add comment, v2

Fix apostrophe

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45214 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 45214 [details]
Add comment, v2

Clearing flags on attachment: 45214

Committed r52379: <http://trac.webkit.org/changeset/52379>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.