Bug 68134 - Make custom scrollbar theme for use in DRT, to reduce pixel diffs between platforms
Summary: Make custom scrollbar theme for use in DRT, to reduce pixel diffs between pla...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks: 67217 66504
  Show dependency treegraph
 
Reported: 2011-09-14 17:22 PDT by Simon Fraser (smfr)
Modified: 2011-10-11 21:47 PDT (History)
16 users (show)

See Also:


Attachments
Add ScrollbarThemeMock (17.07 KB, patch)
2011-09-14 17:39 PDT, Simon Fraser (smfr)
jamesr: review+
Details | Formatted Diff | Diff
Plumb through a setting for mock scrollbars (11.72 KB, patch)
2011-09-14 18:14 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Add a setting to enable mock scrollbars (13.08 KB, patch)
2011-09-16 11:56 PDT, Simon Fraser (smfr)
sam: review+
Details | Formatted Diff | Diff
Use the mock theme when the setting is set (43.01 KB, patch)
2011-09-16 17:35 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (49.63 KB, patch)
2011-10-11 16:18 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (49.50 KB, patch)
2011-10-11 16:51 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-09-14 17:22:28 PDT
We should use a custom scrollbar theme in DRT to reduce pixel differences between platforms.
Comment 1 Simon Fraser (smfr) 2011-09-14 17:22:46 PDT
Taking.
Comment 2 Simon Fraser (smfr) 2011-09-14 17:39:34 PDT
Created attachment 107427 [details]
Add ScrollbarThemeMock
Comment 3 Simon Fraser (smfr) 2011-09-14 18:14:32 PDT
Created attachment 107433 [details]
Plumb through a setting for mock scrollbars
Comment 4 James Robinson 2011-09-14 18:16:56 PDT
I'm very interested in this idea.  In chromium today we use a custom scrollbar theme on windows because there are so many variations across different windows versions / themes  We also spent a ton of effort getting scrollbars to render identically to the Mac port's on Snow Leopard to share pixel results.

Is the idea to use this in all layout tests, or to have some layout tests to test the actual native theming code for scrollbars and to use this for the rest?
Comment 5 Simon Fraser (smfr) 2011-09-14 18:20:40 PDT
The idea is to use it for all layout tests. If we find that there are tests that are testing native scrollbar behaviors, then we'll add a way to go back to to native scrollbars for that test.
Comment 6 James Robinson 2011-09-14 18:29:12 PDT
Comment on attachment 107427 [details]
Add ScrollbarThemeMock

View in context: https://bugs.webkit.org/attachment.cgi?id=107427&action=review

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:116
> +    if (1) {
> +        DEFINE_STATIC_LOCAL(ScrollbarThemeMock, theme, ());
> +        return &theme;
> +    }

This seems like a debugging thing - did you intend to check it in?  Won't this cause the actual mac scrollbar theming code to not be used?

> Source/WebCore/platform/mock/ScrollbarThemeMock.cpp:31
> +using namespace std;

this doesn't appear to be used
Comment 7 James Robinson 2011-09-14 18:29:59 PDT
The second patch seems very mac-specific, so I won't attempt to review it.
Comment 8 James Robinson 2011-09-14 18:34:29 PDT
(In reply to comment #5)
> The idea is to use it for all layout tests. If we find that there are tests that are testing native scrollbar behaviors, then we'll add a way to go back to to native scrollbars for that test.

OK.  That seems like a sound plan from the chromium side as well, although I think we may want to have a few tests to hit the native theme code as well just to make sure that logic is working as expected.  There are also some native theming capabilities (such as opacity) that are probably worth testing specifically.  I think these are a special case, however, and most tests should be using a mock scrollbar.
Comment 9 Simon Fraser (smfr) 2011-09-14 23:30:12 PDT
(In reply to comment #6)
> (From update of attachment 107427 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107427&action=review
> 
> > Source/WebCore/platform/mac/ScrollbarThemeMac.mm:116
> > +    if (1) {
> > +        DEFINE_STATIC_LOCAL(ScrollbarThemeMock, theme, ());
> > +        return &theme;
> > +    }
> 
> This seems like a debugging thing - did you intend to check it in?  Won't this cause the actual mac scrollbar theming code to not be used?

Oops, that was not supposed to be included. A later patch will enable the mock scrollbars based on a Setting.

> 
> > Source/WebCore/platform/mock/ScrollbarThemeMock.cpp:31
> > +using namespace std;
> 
> this doesn't appear to be used

Will fix.
Comment 10 James Robinson 2011-09-15 11:49:33 PDT
Comment on attachment 107427 [details]
Add ScrollbarThemeMock

OK, then R=me assuming the plan is to not land the changes to ScrollbarTheme::nativeTheme() and remove the using namespace std; declaration from ScrollbarThemeMock.cpp
Comment 11 Simon Fraser (smfr) 2011-09-15 17:17:43 PDT
Comment on attachment 107427 [details]
Add ScrollbarThemeMock

http://trac.webkit.org/changeset/95245
Comment 12 David Levin 2011-09-15 18:15:53 PDT
This is so cool!
Comment 13 Deepak Sherveghar 2011-09-16 00:19:08 PDT
>Source/WebCore/GNUmakefile.list.am
>@@webcore_sources += \
>Source/WebCore/platform/mock/GeolocationClientMock.h \
>Source/WebCore/platform/mock/GeolocationServiceMock.cpp \
>Source/WebCore/platform/mock/GeolocationServiceMock.h \
>Source/WebCore/platform/mock/ScrollbarThemeMock.cpp \
>Source/WebCore/platform/mock/ScrollbarThemeMock.cpp \
>Source/WebCore/platform/mock/SpeechInputClientMock.cpp \
>Source/WebCore/platform/mock/SpeechInputClientMock.h \
>Source/WebCore/platform/network/AuthenticationChallengeBase.cpp \

I just updated my Webkit code and my linker complains about multiple definitions in ScrollbarThemeMock.
Error:
=====
usr/bin/ld: error: ./.libs/libWebCore.a(lt1-libWebCore_la-ScrollbarThemeMock.o): multiple definition of 'WebCore::ScrollbarThemeMock::trackRect(WebCore::Scrollbar*, bool)'
/usr/bin/ld: ./.libs/libWebCore.a(libWebCore_la-ScrollbarThemeMock.o): previous definition here


ScrollbarThemeMock.cpp is included twice in Source/WebCore/GNUmakefile.list.am. Instead, one of them should be a header file Source/WebCore/platform/mock/ScrollbarThemeMock.h. Otherwise linker's will complain of multiple definition in gtk.
Comment 14 Wajahat Siddiqui 2011-09-16 01:16:26 PDT
(In reply to comment #13)
> >Source/WebCore/GNUmakefile.list.am
> >@@webcore_sources += \
> >Source/WebCore/platform/mock/GeolocationClientMock.h \
> >Source/WebCore/platform/mock/GeolocationServiceMock.cpp \
> >Source/WebCore/platform/mock/GeolocationServiceMock.h \
> >Source/WebCore/platform/mock/ScrollbarThemeMock.cpp \
> >Source/WebCore/platform/mock/ScrollbarThemeMock.cpp \
> >Source/WebCore/platform/mock/SpeechInputClientMock.cpp \
> >Source/WebCore/platform/mock/SpeechInputClientMock.h \
> >Source/WebCore/platform/network/AuthenticationChallengeBase.cpp \
> 
> I just updated my Webkit code and my linker complains about multiple definitions in ScrollbarThemeMock.
> Error:
> =====
> usr/bin/ld: error: ./.libs/libWebCore.a(lt1-libWebCore_la-ScrollbarThemeMock.o): multiple definition of 'WebCore::ScrollbarThemeMock::trackRect(WebCore::Scrollbar*, bool)'
> /usr/bin/ld: ./.libs/libWebCore.a(libWebCore_la-ScrollbarThemeMock.o): previous definition here
> 
> 
> ScrollbarThemeMock.cpp is included twice in Source/WebCore/GNUmakefile.list.am. Instead, one of them should be a header file Source/WebCore/platform/mock/ScrollbarThemeMock.h. Otherwise linker's will complain of multiple definition in gtk.


Added Patch that fixes this error in https://bugs.webkit.org/show_bug.cgi?id=68229
Comment 15 Simon Fraser (smfr) 2011-09-16 11:56:59 PDT
Created attachment 107695 [details]
Add a setting to enable mock scrollbars
Comment 16 Sam Weinig 2011-09-16 13:50:54 PDT
Comment on attachment 107695 [details]
Add a setting to enable mock scrollbars

View in context: https://bugs.webkit.org/attachment.cgi?id=107695&action=review

> Source/WebCore/page/Settings.h:474
> +        static void setMockScrollbarsEnabled(bool flag);
> +        static bool mockScrollbarsEnabled();

I would move the Safari theming static functions near here.
Comment 17 Simon Fraser (smfr) 2011-09-16 14:03:06 PDT
Sadly we can't have custom scrollbars on the main frame in WK1, so at some point we may have to declare that all pixel testing needs to be done in WK2
Comment 18 Simon Fraser (smfr) 2011-09-16 17:35:25 PDT
Created attachment 107752 [details]
Use the mock theme when the setting is set
Comment 19 WebKit Review Bot 2011-09-16 18:15:06 PDT
Comment on attachment 107752 [details]
Use the mock theme when the setting is set

Attachment 107752 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9723334
Comment 20 mitz 2011-09-16 18:17:00 PDT
Comment on attachment 107695 [details]
Add a setting to enable mock scrollbars

View in context: https://bugs.webkit.org/attachment.cgi?id=107695&action=review

> Source/WebKit/mac/WebView/WebPreferencesPrivate.h:224
> +// This is a global setting.
> +- (BOOL)mockScrollbarsEnabled;
> +- (void)setMockScrollbarsEnabled:(BOOL)flag;

It seems wrong to represent a global setting by a WebPreferences instance method. This would be better served by a class method on WebView.
Comment 21 Simon Fraser (smfr) 2011-09-16 18:18:05 PDT
Comment on attachment 107695 [details]
Add a setting to enable mock scrollbars

http://trac.webkit.org/changeset/95347
Comment 22 Simon Fraser (smfr) 2011-09-16 18:19:38 PDT
(In reply to comment #20)
> (From update of attachment 107695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107695&action=review
> 
> > Source/WebKit/mac/WebView/WebPreferencesPrivate.h:224
> > +// This is a global setting.
> > +- (BOOL)mockScrollbarsEnabled;
> > +- (void)setMockScrollbarsEnabled:(BOOL)flag;
> 
> It seems wrong to represent a global setting by a WebPreferences instance method. This would be better served by a class method on WebView.

True, although there's precedent for this (poor) technique.

Do we follow a similar pattern in WK2?
Comment 23 Gyuyoung Kim 2011-09-16 18:19:39 PDT
Comment on attachment 107752 [details]
Use the mock theme when the setting is set

Attachment 107752 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9729188
Comment 24 Simon Fraser (smfr) 2011-09-16 18:24:01 PDT
> It seems wrong to represent a global setting by a WebPreferences instance method. This would be better served by a class method on WebView.

I filed bug 68300.
Comment 25 WebKit Review Bot 2011-09-17 07:57:39 PDT
Comment on attachment 107752 [details]
Use the mock theme when the setting is set

Attachment 107752 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9734279
Comment 26 Simon Fraser (smfr) 2011-10-11 16:18:33 PDT
Created attachment 110599 [details]
Patch
Comment 27 WebKit Review Bot 2011-10-11 16:44:37 PDT
Comment on attachment 110599 [details]
Patch

Attachment 110599 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10029666
Comment 28 Simon Fraser (smfr) 2011-10-11 16:51:04 PDT
Created attachment 110607 [details]
Patch
Comment 29 Darin Adler 2011-10-11 17:12:30 PDT
Comment on attachment 110607 [details]
Patch

Seems a little strange that these functions return a pointer to a theme rather than a reference.