[webkit-reviews] review granted: [Bug 66008] [chromium] Update WebScrollbar so that it works with overlay scrollbars on Lion : [Attachment 103554] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted John Abd-El-Malek
<jam at chromium.org>'s request for review:
Bug 66008: [chromium] Update WebScrollbar so that it works with overlay
scrollbars on Lion
https://bugs.webkit.org/show_bug.cgi?id=66008

Attachment 103554: Patch
https://bugs.webkit.org/attachment.cgi?id=103554&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103554&action=review


R=me

> Source/WebKit/chromium/src/ScrollbarGroup.cpp:8
> + *	  * Redistributions of source code must retain the above copyright

fyi, this is the wrong copyright header format.  we're supposed to use the
2-clause version for new stuff (like in
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/
TiledLayerChromium.cpp, for instance)

> Source/WebKit/chromium/src/ScrollbarGroup.cpp:46
> +    : m_page(page),
> +	 m_horizontalScrollbar(0)

nit: comma goes on the next line

> Source/WebKit/chromium/src/ScrollbarGroup.cpp:79
> +    } else {
> +	   willRemoveVerticalScrollbar(scrollbar->scrollbar());

might as well ASSERT() this is the vertical scrollbar

> Source/WebKit/chromium/src/ScrollbarGroup.h:48
> +    ScrollbarGroup(WebCore::Page*);

add 'explicit'