[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