[webkit-reviews] review denied: [Bug 17483] Implement Scrollbars on Windows (Cairo) : [Attachment 19280] Revised patch (paste error in text of patch)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brent Fulgham <bfulgham at gmail.com>'s
request for review:
Bug 17483: Implement Scrollbars on Windows (Cairo)
http://bugs.webkit.org/show_bug.cgi?id=17483

Attachment 19280: Revised patch (paste error in text of patch)
http://bugs.webkit.org/attachment.cgi?id=19280&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Generally looks good. A few problems that make me say review-.

First, I had some trouble understanding the changes because there was no
ChangeLog included in the patch.

+#if PLATFORM(CAIRO)

This is the wrong #if for non-SafariTheme scrollbars. Maybe the right one is
USE_SAFARI_THEME. If not, we need to figure out a better one. Perhaps we need
to add a define just to control what theme is used. Maybe we could make this
use the machinery from <wtf/Platform.h>, even though what's being configured is
truly a WebCore option. We can have the default be set automatically based on
the presence or absence of CG, for now, but that rule about what theme to use
should appear in exactly one place, not at all the various sites in the code.

There are lots of minor formatting issues in the code (please read the style
guidelines). Here's an example:

+    if (!isEnabled()) {
+	 state += SP_ABS_DISABLE_MODIFIER;
+    }
+    else if (m_client->isActive()) {
+	state += SP_ABS_HOT_MODIFIER;
+    }

Single-line bodies should not have braces around them. And the else should not
be on a different line than that brace before it.

+   PlatformGraphicsContext* ctx = context->platformContext();
+   state = TS_ACTIVE;
+
+    // Now paint the button.

Why the different indenting here?

+	if (true) { //isFocused(o)) {

This commented out code is not the kind of thing we usually check in. You can
include a comment, but use English rather than a code fragment, please.

+	 DrawFrameControl(hdc, &widgetRect, DFC_BUTTON, 0);
//themeData.m_classicState);

Same thing here.

+    float proportion = (float)(m_visibleSize) / m_totalSize;