[Webkit-unassigned] [Bug 17483] Implement Scrollbars on Windows (Cairo)

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=17483


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #19280|review?                     |review-
               Flag|                            |




------- Comment #3 from darin at apple.com  2008-02-24 18:21 PDT -------
(From update of attachment 19280)
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;


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.