Bug 17483 - Implement Scrollbars on Windows (Cairo)
Summary: Implement Scrollbars on Windows (Cairo)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-02-21 16:41 PST by Brent Fulgham
Modified: 2008-02-29 16:30 PST (History)
0 users

See Also:


Attachments
Add scrollbar support to Windows (Cairo) build (44.96 KB, patch)
2008-02-21 17:16 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Revised patch (paste error in text of patch) (42.82 KB, patch)
2008-02-22 09:21 PST, Brent Fulgham
darin: review-
Details | Formatted Diff | Diff
Corrections based on Darin's suggestions (44.24 KB, patch)
2008-02-24 20:38 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Revision based on Darin's latest comments (44.23 KB, patch)
2008-02-25 10:34 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Update based on Aroben/Bdash recommendations (49.81 KB, patch)
2008-02-26 16:56 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Revision based on Bdash review. (49.79 KB, patch)
2008-02-26 20:03 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Revise to use proper 17-pixel width scrollbars (50.32 KB, patch)
2008-02-26 22:15 PST, Brent Fulgham
aroben: review-
Details | Formatted Diff | Diff
Scrollbars-only patch (37.35 KB, patch)
2008-02-27 15:08 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Scrollbars-only patch (oops) (37.74 KB, patch)
2008-02-27 15:21 PST, Brent Fulgham
aroben: review-
Details | Formatted Diff | Diff
Update based on aroben's comments (44.81 KB, patch)
2008-02-28 17:52 PST, Brent Fulgham
aroben: review-
Details | Formatted Diff | Diff
Update revised based on Adam's comments (44.74 KB, patch)
2008-02-29 11:09 PST, Brent Fulgham
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2008-02-21 16:41:30 PST
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
Comment 1 Brent Fulgham 2008-02-21 17:16:18 PST
Created attachment 19267 [details]
Add scrollbar support to Windows (Cairo) build
Comment 2 Brent Fulgham 2008-02-22 09:21:09 PST
Created attachment 19280 [details]
Revised patch (paste error in text of patch)

This patch should be applied in the WebCore directory.
Comment 3 Darin Adler 2008-02-24 18:21:50 PST
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;
Comment 4 Brent Fulgham 2008-02-24 20:38:18 PST
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>
Comment 5 Darin Adler 2008-02-24 22:55:50 PST
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.
Comment 6 Brent Fulgham 2008-02-25 09:32:01 PST
(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).

Comment 7 Brent Fulgham 2008-02-25 10:34:11 PST
Created attachment 19351 [details]
Revision based on Darin's latest comments

Revised version using #If USE(SAFARI_THEME)
Comment 8 Brent Fulgham 2008-02-26 16:56:58 PST
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.
Comment 9 Brent Fulgham 2008-02-26 20:03:40 PST
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.
Comment 10 Brent Fulgham 2008-02-26 22:15:03 PST
Created attachment 19395 [details]
Revise to use proper 17-pixel width scrollbars

Correct scrollbar width to match platform-standard 17-pixel widths.
Comment 11 Adam Roben (:aroben) 2008-02-27 13:51:18 PST
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.
Comment 12 Brent Fulgham 2008-02-27 15:08:50 PST
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.
Comment 13 Brent Fulgham 2008-02-27 15:21:43 PST
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.
Comment 14 Adam Roben (:aroben) 2008-02-28 16:14:25 PST
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.
Comment 15 Brent Fulgham 2008-02-28 17:52:58 PST
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.
Comment 16 Adam Roben (:aroben) 2008-02-29 09:24:33 PST
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.
Comment 17 Brent Fulgham 2008-02-29 11:09:59 PST
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.
Comment 18 Adam Roben (:aroben) 2008-02-29 12:42:32 PST
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
Comment 19 Matt Lilek 2008-02-29 16:30:38 PST
Landed in r30687.