Bug 17483 – Implement Scrollbars on Windows (Cairo)

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

If you attempt to use WebKit without the CoreGraphics and SafariTheme libraries, you do not have working scrollbars.

The attached patch provides support for themed scrollbars.  It is based on the PlatformScrollBarSafari.cpp and RenderThemeWin.cpp

[Brent Fulgham]

Created attachment 19267 [details]
Add scrollbar support to Windows (Cairo) build

[Brent Fulgham]

Created attachment 19280 [details]
Revised patch (paste error in text of patch)

This patch should be applied in the WebCore directory.

[Darin Adler]

Comment on attachment 19280 [details]
Revised patch (paste error in text of patch)

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;

[Brent Fulgham]

Created attachment 19333 [details]
Corrections based on Darin's suggestions

* Clean up formatting
* Replace missing ChangeLog entry
* Switch to "USE_SAFARI_THEME", and set it inside <wtf/Platform.h>

[Darin Adler]

Comment on attachment 19333 [details]
Corrections based on Darin's suggestions

If USE_SAFARI_THEME is going to be in Platform.h, then it should be:

    #if USE(SAFARI_THEME)

Look at the other WTF_USE_ stuff in Platform.h to see examples.

Or it could be in WebCore as USE_SAFARI_THEME and not be part of the Platform.h thing. I think it's a bit ugly that decisions about overall WebKit configuration are in wtf/Platform.h and that's something we'll have to fix eventually.

[Brent Fulgham]

(In reply to comment #5)
> (From update of attachment 19333 [details] [edit])
> If USE_SAFARI_THEME is going to be in Platform.h, then it should be:
> 
>     #if USE(SAFARI_THEME)

It's a little confusing (at least to me) what should go in wtf/Platform.h, and what belongs in WebCore/config.h.  Based on my (imperfect) understanding of things, it seems logical to put the THEME stuff in Platform.h because it's currently a 1:1 relationship with use of CG.  However it has no use inside the JavaScriptCore logic, so wtf/Platform.h might be at too high a visibility level.

For now, let's just leave things as-is and set to USE(SAFARI_THEME).

[Brent Fulgham]

Created attachment 19351 [details]
Revision based on Darin's latest comments

Revised version using #If USE(SAFARI_THEME)

[Brent Fulgham]

Created attachment 19390 [details]
Update based on Aroben/Bdash recommendations

* Use correct WTF_USE_SAFARI_THEME definition
* Switch to SOFT_LINK in RenderThemeWin and PlatformScrollbarWin.

[Brent Fulgham]

Created attachment 19394 [details]
Revision based on Bdash review.

* Get rid of left-over debugging "if (true)" code.
* Revise formatting (noticed a tab, and some single-line conditionals inside braces.

[Brent Fulgham]

Created attachment 19395 [details]
Revise to use proper 17-pixel width scrollbars

Correct scrollbar width to match platform-standard 17-pixel widths.

[Adam Roben (:aroben)]

Comment on attachment 19395 [details]
Revise to use proper 17-pixel width scrollbars

+        (WebCore::RenderThemeWin::determineState): Extend to recognize
+          scrollbar parts.
+        (WebCore::RenderThemeWin::paintButton): Add new implementation
+          for drawing buttons given a graphic context and bounding box.

I don't see any callers of this new RenderThemeWin method, so I don't think you should add it or change determineState. Maybe I'm missing something?

It would be good if the changes to make RenderThemeWin use the SOFT_LINK macros could be broken out into their own patch.

+// Note:  Copied from RenderThemeWin.  There should be some way to better
+// encapsulate/share this stuff.  See all RenderThemeSafari for more such stuff...
+//
+// After discussion with Adam Roben, we will make the ugly choice of copying logic
+// from RenderThemeWin until the Theme logic is refactored

I think a shorter comment would suffice, such as:

// FIXME: Refactor this soft-linking code in a way that can be shared with RenderThemeWin

Can you split up the patch as suggested above and re-upload it? That will make it much easier to review.

[Brent Fulgham]

Created attachment 19410 [details]
Scrollbars-only patch

Revised patch to only deal with Windows Scrollbar.  Moved RenderThemeWin change to a separate issue (#17576) for the revision to use softlink.

[Brent Fulgham]

Created attachment 19413 [details]
Scrollbars-only patch (oops)

Patch revised as follows:
1.  RenderThemeWin changes are now under bug http://bugs.webkit.org/show_bug.cgi?id=17576.
2.  Scrollbar-only changes here.
3.  Added back missing wtf/Platform.h changes.

[Adam Roben (:aroben)]

Comment on attachment 19413 [details]
Scrollbars-only patch (oops)

+#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.

[Brent Fulgham]

Created attachment 19440 [details]
Update based on aroben's comments

* Switch wtf/Platform.h logic to use #if PLATFORM(WIN) && !PLATFORM(CG)
* Switch win/PlatformScrollbar.h logic to use #if PLATFORM(WIN) && !USE(SAFARI_THEME)
* Clean up comment type (actually, delete comment)
* Correct formatting
* Change scrollbar to get dimensions from GetSystemMetrics calls.  Use only system metrics (not Safari-set 'controlSize') for scrollbar dimensions.
* Make theme handle a static entry in the implementation, since it's always the same thing.  No longer release this handle as it is always needed during a run of the browser.

[Adam Roben (:aroben)]

Comment on attachment 19440 [details]
Update based on aroben's comments

+static int cHorizontalWidth = 17;
+static int cHorizontalHeight = 17;
+static int cVerticalWidth = 17;
+static int cVerticalHeight = 17;
+static int cRealButtonLength = 28;
+static int cButtonInset = 14;
+static int cButtonHitInset = 3;

I don't think there's any reason to initialize these if you're just going to get the real values later.

+    , m_ButtonLength (14)

There shouldn't be a space before the parentheses here.

+    cHorizontalHeight = ::GetSystemMetrics(SM_CYHSCROLL);
+    cHorizontalWidth = ::GetSystemMetrics(SM_CXHSCROLL);
+    cVerticalHeight = ::GetSystemMetrics(SM_CYVSCROLL);
+    cVerticalWidth = ::GetSystemMetrics(SM_CXVSCROLL);
+    cThumbWidth = ::GetSystemMetrics(SM_CXHTHUMB);
+    cThumbHeight = ::GetSystemMetrics(SM_CYVTHUMB);

These should only need to be initialized once (though really we should listen for WM_SETTINGSCHANGE and refetch the values then, but that's for another day).

+    m_ButtonLength = ::GetSystemMetrics((orientation == VerticalScrollbar) ? SM_CYVSCROLL : SM_CXHSCROLL);

I think you can just have two more static variables, cHorizontalButtonLength and cVerticalButtonLength. Then you won't need this new member.

Once these are fixed I think this'll be good to go.

[Brent Fulgham]

Created attachment 19452 [details]
Update revised based on Adam's comments

* Zero-initialized System Metric-based values so they can be initialized on first scrollbar construction.
* Added note that we should (someday) check for WM_SETTINGSCHANGE.
* Use two new static variables to hold button size
* No longer change PlatformScrollbar.h.  No new class members.

[Adam Roben (:aroben)]

Comment on attachment 19452 [details]
Update revised based on Adam's comments

You're missing a ChangeLog for JavaScriptCore, but whomever lands this can make one.

+static int cHorizontalWidth = 0;
+static int cHorizontalHeight = 0;
+static int cVerticalWidth = 0;
+static int cVerticalHeight = 0;
+static int cHorizontalButtonWidth = 0;
+static int cVerticalButtonHeight = 0;

statics are automatically initialized to 0 in C++, so this explicit initialization isn't really necessary.

r=me

[Matt Lilek]

Landed in r30687.