[webkit-reviews] review denied: [Bug 17483] Implement Scrollbars on Windows (Cairo) : [Attachment 19413] Scrollbars-only patch (oops)

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben 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 19413: Scrollbars-only patch (oops)
http://bugs.webkit.org/attachment.cgi?id=19413&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
+#if PLATFORM(CG)
+#define WTF_USE_SAFARI_THEME 1
+#endif

The first line should probably be:

#if PLATFORM(WIN) && !PLATFORM(CG)

since SafariTheme only exists on Windows.

+#if !USE(SAFARI_THEME)
+    mutable HANDLE m_scrollBarTheme;
+#endif

Similarly, this should be:

#if PLATFORM(WIN) && !USE(SAFARI_THEME)

+const unsigned SP_ABS_HOT_MODIFIER = 1;
+const unsigned SP_ABS_PRESSED_MODIFIER = 2;
+const unsigned SP_ABS_DISABLE_MODIFIER = 3;

These should all be marked static.

+    // Cleanup theme stout.uff

Looks like a typo here. I'm not sure this comment is needed though.

+    if (!isEnabled()) {
+	 state += SP_ABS_DISABLE_MODIFIER;
+	 classicState |= DFCS_INACTIVE;
+    }
+    else if ((m_pressedPart == BackButtonPart && start)
+	   || (m_pressedPart == ForwardButtonPart && !start))
+    {
+	 state += SP_ABS_PRESSED_MODIFIER;
+	 classicState |= DFCS_PUSHED | DFCS_FLAT;
+    }
+    else if (m_client->isActive())
+    {
+	state += SP_ABS_HOT_MODIFIER;
+	classicState |= DFCS_HOT;
+    }

The braces here don't match our coding style.

+    if (!uxthemeLibrary() && !m_scrollBarTheme)
+	 m_scrollBarTheme = OpenThemeData(0, L"Scrollbar");

I think you have the wrong condition here. I think you want:

if (uxthemeLibrary() && !m_scrollBarTheme)

The other instances of this seem to have it right.

Maybe this initialization should be put into a helper function, since it's done
over and over?

r- so the above can be fixed, but overall it looks good.