Bug 78028 – Decouple scrollbar painting from Scrollbar object

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Peter Kotwicz]

Decouple scrollbar painting from Scrollbar object

[Peter Kotwicz]

Created attachment 125913 [details]
Patch

[WebKit Review Bot]

Attachment 125913 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/platform/chromium/ScrollbarThemePainterChromiumLinux.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/platform/chromium/ScrollbarThemePainterChromiumLinux.h:50:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/WebCore.gypi:2950:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/WebCore.gypi:2951:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/WebCore.gypi:2952:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/WebCore.gypi:2953:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/platform/chromium/ScrollbarThemePainterChromium.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/chromium/ScrollbarThemePainterChromium.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/platform/chromium/ScrollbarThemePainterChromium.h:46:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/platform/chromium/ScrollbarThemePainterChromium.h:53:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/chromium/ScrollbarThemePainterChromium.h:68:  The parameter name "scrollbar" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/chromium/ScrollbarThemePainterChromium.h:69:  The parameter name "scrollbar" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/chromium/ScrollbarThemePainterChromiumLinux.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/chromium/ScrollbarPaintData.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/chromium/ScrollbarPaintData.cpp:41:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/chromium/ScrollbarThemeChromium.h:44:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/chromium/ScrollbarPaintData.h:29:  Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/chromium/ScrollbarPaintData.h:42:  The parameter name "orientation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/chromium/ScrollbarPaintData.h:42:  The parameter name "controlSize" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 19 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 125913 [details]
Patch

Title says "not for review", so I guess it's not.

[Alexey Proskuryakov]

This comes with a test case. What is the bug that's being fixed? The title sounds like refactoring.

[Adrienne Walker]

(CCing andersca, as I'm not sure how this intersects with the ScrollTree work and also wonder if "drawing scrollbars from another thread" is a concern for Safari too.)

Peter, sorry for not giving you feedback on this sooner.

Separating out painting code at the ScrollbarThemeChromium level doesn't feel like the right place to put this abstraction.  It's right in the middle of a virtual inheritance hierarchy, so it's awkward to have some functions branch off into the *Painter class that takes ScrollbarData, but still have a whole bunch of virtual functions in base classes that take Scrollbar*.

Talking out loud, the goal here is to be able to draw scrollbars (with adjustable scroll positions) from a separate thread, sharing as much painting logic from ScrollbarTheme as possible.

Just holding a reference back to an existing ScrollBar isn't quite right, because we want to be able to have that other thread 
manipulate the scroll position independently and draw it, without messing with the actual Scrollbar.  Duplicating the Scrollbar itself seems like too much to reconstruct.  Scrollbar itself has-a ScrollableArea and is-a Widget which has-a ScrollView.  That'd be a lot to try to duplicate.

I wonder if a better approach would be to go a little bit deeper than you're going and get all ScrollbarTheme-derived classes to operate on some other abstraction than Scrollbar.  The theme classes are already functional classes, so they just need something that conforms to a Scrollbar-like interface rather than directly using Scrollbar.  In other words, create some interface that both Scrollbar and ScrollbarData could implement and have all the Scrollbar theme code operate on that instead.

[Peter Kotwicz]

Created attachment 127039 [details]
Patch

[Peter Kotwicz]

I decided to split this up into the create of the ScrollbarThemeClient interfaces and their use.
The ScrollbarThemeClient class should have all the member functions required for using the interface in the ScrollbarTheme class on all platforms.

I added m_client as a member variable of ScrollbarPainter because mac registers the ScrollbarThemeClient* during ScrollbarTheme::register() and keeps a pointer inside the ScrollbarTheme class.

I know that ScrollbarThemeClientImpl is a terrible name, do you have any suggestions?

[Early Warning System Bot]

Comment on attachment 127039 [details]
Patch

Attachment 127039 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11521415

[Gyuyoung Kim]

Comment on attachment 127039 [details]
Patch

Attachment 127039 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11514460

[Philippe Normand]

Comment on attachment 127039 [details]
Patch

Attachment 127039 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11522465

[Adrienne Walker]

(In reply to comment #7)

> I know that ScrollbarThemeClientImpl is a terrible name, do you have any suggestions?

Is it possible to drop the class entirely and have Scrollbar implement the ScrollbarThemeClient interface? I think this would also eliminate the need for m_client and a number of other code changes.

[Peter Kotwicz]

Created attachment 127239 [details]
Patch

[Adrienne Walker]

Comment on attachment 127239 [details]
Patch

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

> Source/WebCore/platform/Scrollbar.cpp:556
> +PassOwnPtr<Vector<IntRect> > Scrollbar::getTickmarks() const

Can you keep the same signature as ScrollableArea::getTickmarks? There's no need to wrap a Vector in an OwnPtr here.

> Source/WebCore/platform/Widget.h:131
> +    virtual int x() const { return frameRect().x(); }
> +    virtual int y() const { return frameRect().y(); }
> +    virtual int width() const { return frameRect().width(); }
> +    virtual int height() const { return frameRect().height(); }
> +    virtual IntSize size() const { return frameRect().size(); }
> +    virtual IntPoint location() const { return frameRect().location(); }

I think you should revert these and other virtual modifications you made to Widget.h.  They're not necessary if you're going to explicitly thunk back to them in Scrollbar.

> Source/WebCore/rendering/RenderScrollbar.h:42
> +class RenderScrollbar : public Scrollbar,
> +                        public RenderScrollbarThemeClient {

This is an awkward inheritance hierarchy that forces you to redeclare virtuals from Scrollbar because it includes ScrollbarThemeClient from two different ancestors.

Honestly, I would prefer to punt on RenderScrollbar.  ScrollbarThemeClient is a small bag of data and ScrollbarTheme is a set of functions to to process that.  To serialize Scrollbar into a new ScrollbarData class (that implements ScrollbarThemeClient) is trivial, as you had done in your previous patch.  Doing something like that for RenderScrollbar, which is made up of RenderScrollbarPart (which is-a RenderBlock) is much less so.

[Alexey Proskuryakov]

What does [Not For Review] in the title of the bug mean at this point? In fact, I'm never sure what it means when in bug title, not in patch name.

[Early Warning System Bot]

Comment on attachment 127239 [details]
Patch

Attachment 127239 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11528866

[WebKit Review Bot]

Comment on attachment 127239 [details]
Patch

Attachment 127239 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11530855

[Gyuyoung Kim]

Comment on attachment 127239 [details]
Patch

Attachment 127239 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11528880

[Philippe Normand]

Comment on attachment 127239 [details]
Patch

Attachment 127239 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11536169

[WebKit Review Bot]

Attachment 127239 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: dataTransfer.types (HTML5 drag & drop) should return DOMStringList
Using index info to reconstruct a base tree...
<stdin>:84: trailing whitespace.
        
<stdin>:333: trailing whitespace.
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
<stdin>:334: trailing whitespace.
 width="300" height="300" onload="runRepaintTest()">
<stdin>:335: trailing whitespace.
<script xlink:href="../../fast/repaint/resources/repaint.js" />
<stdin>:336: trailing whitespace.
<script><![CDATA[
warning: squelched 16 whitespace errors
warning: 21 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 dataTransfer.types (HTML5 drag & drop) should return DOMStringList

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kotwicz]

Created attachment 127611 [details]
Patch

[Adrienne Walker]

Comment on attachment 127611 [details]
Patch

Awesome.  Can you also then change all the ScrollbarTheme classes to use this interface instead?

[Peter Kotwicz]

Definitely, however, I would rather change all the ScrollbarThemes to use the interface in a separate patch. (To make the addition of new methods and a simple rename really clear)

[Adrienne Walker]

(In reply to comment #22)
> Definitely, however, I would rather change all the ScrollbarThemes to use the interface in a separate patch. (To make the addition of new methods and a simple rename really clear)

If you don't mind, I'd prefer it to be the same patch, so you could be sure that everything you needed to wrap in ScrollbarThemeClient was actually there.

[Peter Kotwicz]

Created attachment 128027 [details]
Patch

[Peter Kotwicz]

Changes to ScrollbarTheme*.h / ScrollbarTheme*.cpp

Replaced Scrollbar* with ScrollbarThemeClient*
Replaced ->scrollableArea()->getTickmarks( with ->getTickmarks(
Replaced ->scrollableArea()->isActive() with ->isScrollableAreaActive()
Replaced ->scrollableArea()->scrollbarOverlayStyle() with 
                                ->scrollbarOverlayStyle()

[Adrienne Walker]

pkotwicz: Can you fix the mac build errors?

cc1plus: warnings being treated as errors
In file included from /Volumes/Data/WebKit/Source/WebCore/platform/Scrollbar.h:29,
                 from /Volumes/Data/WebKit/Source/WebCore/platform/ScrollView.h:31,
                 from /Volumes/Data/WebKit/Source/WebCore/page/FrameView.h:32,
                 from /Volumes/Data/WebKit/Source/WebCore/dom/MouseEvent.cpp:29:
/Volumes/Data/WebKit/Source/WebCore/platform/ScrollbarThemeClient.h:40: warning: 'class WebCore::ScrollbarThemeClient' has virtual functions but non-virtual destructor

[James Robinson]

Comment on attachment 128027 [details]
Patch

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

> Source/WebCore/platform/Scrollbar.h:64
> +    virtual ScrollbarOrientation orientation() const { return m_orientation; }

please group the virtual overrides for ScrollbarThemeClient together and document what interface they are implementing (this file is not a great example of this pattern, but other files have a comment saying something like "// ScrollbarThemeClient implementation." at the top of a block of virtuals).

> Source/WebCore/platform/ScrollbarThemeClient.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

it's 2012 now

> Source/WebCore/platform/mac/ScrollbarThemeMac.h:29
> +#include "Scrollbar.h"

is this #include needed?

[Peter Kotwicz]

Created attachment 128216 [details]
Patch

[Peter Kotwicz]

Created attachment 128245 [details]
Patch

[Peter Kotwicz]

Created attachment 128488 [details]
Patch

[Philippe Normand]

Comment on attachment 128488 [details]
Patch

Attachment 128488 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11603336

[Early Warning System Bot]

Comment on attachment 128488 [details]
Patch

Attachment 128488 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11574350

[Peter Kotwicz]

Created attachment 128502 [details]
Patch

[Peter Kotwicz]

Created attachment 128552 [details]
Patch

[Peter Kotwicz]

Created attachment 128605 [details]
Patch

[Adrienne Walker]

(In reply to comment #35)
> Created an attachment (id=128605) [details]
> Patch

If you're going to upload a series of patches in a row, can you leave a comment as to why you're doing it or pass -m to webkit-patch upload with a patchset message to differentiate them?

[WebKit Commit Bot]

Attachment 128605 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:39:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderScrollbarTheme.h:59:  The parameter name "scrollbar" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/efl/ScrollbarThemeEfl.h:42:  The parameter name "scrollbar" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/efl/ScrollbarThemeEfl.h:43:  The parameter name "scrollbar" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/mac/ScrollbarThemeMac.h:45:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:35:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:61:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:62:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:63:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:64:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:65:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:66:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/Scrollbar.h:96:  The parameter name "e" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/ScrollbarTheme.h:84:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarTheme.h:109:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarTheme.h:110:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/Scrollbar.cpp:117:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 17 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kotwicz]

Created attachment 128821 [details]
Should make patch build with Safari

[WebKit Review Bot]

Attachment 128821 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:39:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderScrollbarTheme.h:59:  The parameter name "scrollbar" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/efl/ScrollbarThemeEfl.h:42:  The parameter name "scrollbar" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/efl/ScrollbarThemeEfl.h:43:  The parameter name "scrollbar" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/mac/ScrollbarThemeMac.h:45:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:35:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:61:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:62:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:63:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:64:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:65:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarThemeComposite.h:66:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/Scrollbar.h:96:  The parameter name "e" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/ScrollbarTheme.h:84:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarTheme.h:109:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/ScrollbarTheme.h:110:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/Scrollbar.cpp:117:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 17 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kotwicz]

Created attachment 128880 [details]
Fixes style bugs

[Adrienne Walker]

This unofficially looks good to me.

[James Robinson]

Comment on attachment 128880 [details]
Fixes style bugs

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

I think this looks pretty good.  Two comments below that will make this easier to understand fully.

> Source/WebCore/platform/Scrollbar.cpp:115
> +void Scrollbar::setFrameRect(const IntRect& rect)

nit: would you mind not moving the defintions of this functions around in this .cpp in this patch? it makes it very hard to tell what code is actually changing and what is not

> Source/WebCore/platform/ScrollbarThemeClient.h:42
> +    virtual ~ScrollbarThemeClient() { }

Is it possible to make this d'tor protected?  In other words, are there bits of code in the ScrollbarTheme classes relying on ScrollbarThemeClient pointers for lifetime management / ownership or do they simply need a view into the Scrollbar's state?  I think if we can make this d'tor protected it would be a big improvement to the general readability of the Scrollbar code.

[Peter Kotwicz]

Created attachment 129332 [details]
Changes as per jamesr

[James Robinson]

Comment on attachment 129332 [details]
Changes as per jamesr

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

Awesome! This looks great, R=me.  There are two more issues to address before landing and I also think it's prudent to let the EWS bots go green just to be safe.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

this will trigger an SVN presubmit check and barf. since this is a refactor and not a change of behavior you aren't required to add new tests, but you do have to delete this line. it would be great if you could instead provide a brief summary of the changes

> Source/WebCore/ChangeLog:11
> +        * WebCore.gypi:
> +        * WebCore.xcodeproj/project.pbxproj:

can you also add this new header file to the other build systems we have? not all build systems list headers, but looking at references to ScrollbarTheme.h i think you will want to modify:

Source/WebCore/GNUmakefile.list.am
Source/WebCore/Target.pri
Source/WebCore/WebCore.vcproj/WebCore.vcproj

[Peter Kotwicz]

Created attachment 129747 [details]
Build files for Win, GTK, and QT

[James Robinson]

Comment on attachment 129747 [details]
Build files for Win, GTK, and QT

Looks great, R=me! I'll do some sanity checks locally and land by hand since EWS is pretty janked up right now.

[James Robinson]

Committed r109451: <http://trac.webkit.org/changeset/109451>