Taking.

Created attachment 107427 [details] Add ScrollbarThemeMock

Created attachment 107433 [details] Plumb through a setting for mock scrollbars

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?

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 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

The second patch seems very mac-specific, so I won't attempt to review it.

(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.

(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 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 on attachment 107427 [details] Add ScrollbarThemeMock http://trac.webkit.org/changeset/95245

This is so cool!

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

(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

Created attachment 107695 [details] Add a setting to enable mock scrollbars

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.

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

Created attachment 107752 [details] Use the mock theme when the setting is set

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 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 on attachment 107695 [details] Add a setting to enable mock scrollbars http://trac.webkit.org/changeset/95347

(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 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

> 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 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

Created attachment 110599 [details] Patch

Comment on attachment 110599 [details] Patch Attachment 110599 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10029666

Created attachment 110607 [details] Patch

Comment on attachment 110607 [details] Patch Seems a little strange that these functions return a pointer to a theme rather than a reference.

This patch appears to have broken SL build: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/34679/steps/compile-webkit/logs/stdio

http://trac.webkit.org/changeset/97227 (In reply to comment #30) > This patch appears to have broken SL build: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/34679/steps/compile-webkit/logs/stdio Fixed in http://trac.webkit.org/changeset/97229