[webkit-reviews] review granted: [Bug 46645] Make scrolling work properly with writing modes : [Attachment 75309] Part 1: Generalize scrollOrigin concept to work with both horizontal and vertical text.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 1 12:58:42 PST 2010
Darin Adler <darin at apple.com> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 46645: Make scrolling work properly with writing modes
https://bugs.webkit.org/show_bug.cgi?id=46645
Attachment 75309: Part 1: Generalize scrollOrigin concept to work with both
horizontal and vertical text.
https://bugs.webkit.org/attachment.cgi?id=75309&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=75309&action=review
> WebCore/platform/ScrollView.cpp:1043
> + IntPoint prevScrollOrigin = m_scrollOrigin;
>
> -void ScrollView::updateScrollbars()
> -{
> - if (!platformWidget())
> + m_scrollOrigin = origin;
> +
> + if (platformWidget() || !updatePosition)
> + return;
> +
> + // Update if the scroll origin changes, since our position will be
different if the content size did not change.
> + if (prevScrollOrigin != m_scrollOrigin)
> updateScrollbars(scrollOffset());
Seems like a cleaner way to write this is:
if (origin == m_scrollOrigin)
return;
m_scrollOrigin = origin;
...
No need to have the check specifically wrapped around the updateScrollbars
call.
> WebKit/mac/WebView/WebDynamicScrollBarsView.mm:68
> + // scrollOrigin is set for various combinations of writing mode and
direction. See the comment next to the
> + // corresponding member in ScrollView.h.
> + NSPoint scrollOrigin;
We normally use one space after a period. I also suggest moving that second
sentence to its own second line.
> WebKit/mac/WebView/WebDynamicScrollBarsView.mm:580
> + NSPoint docOrigin = [docView bounds].origin;
> + if (docOrigin.x != -scrollOrigin.x || docOrigin.y != -scrollOrigin.y) {
Early return would make this function read better, I think.
> WebKit/mac/WebView/WebDynamicScrollBarsView.mm:582
> +
Looks like there was an extra space added here.
More information about the webkit-reviews
mailing list