Bug 78028 - Decouple scrollbar painting from Scrollbar object
Summary: Decouple scrollbar painting from Scrollbar object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 78872 79137
  Show dependency treegraph
 
Reported: 2012-02-07 13:27 PST by Peter Kotwicz
Modified: 2012-03-01 15:36 PST (History)
13 users (show)

See Also:


Attachments
Patch (47.97 KB, patch)
2012-02-07 13:29 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (34.12 KB, patch)
2012-02-14 14:02 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (23.13 KB, patch)
2012-02-15 14:37 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (12.09 KB, patch)
2012-02-17 10:45 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (121.61 KB, patch)
2012-02-21 12:59 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (121.50 KB, patch)
2012-02-22 08:02 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (128.69 KB, patch)
2012-02-22 10:45 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (124.17 KB, patch)
2012-02-23 09:48 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (127.67 KB, patch)
2012-02-23 10:46 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (134.57 KB, patch)
2012-02-23 13:59 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Patch (130.01 KB, patch)
2012-02-23 16:54 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Should make patch build with Safari (131.69 KB, patch)
2012-02-24 15:51 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Fixes style bugs (131.65 KB, patch)
2012-02-25 13:29 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Changes as per jamesr (127.91 KB, patch)
2012-02-28 14:32 PST, Peter Kotwicz
no flags Details | Formatted Diff | Diff
Build files for Win, GTK, and QT (129.77 KB, patch)
2012-03-01 14:32 PST, Peter Kotwicz
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kotwicz 2012-02-07 13:27:49 PST
Decouple scrollbar painting from Scrollbar object
Comment 1 Peter Kotwicz 2012-02-07 13:29:31 PST
Created attachment 125913 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-07 13:31:51 PST
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.
Comment 3 Eric Seidel (no email) 2012-02-07 15:42:05 PST
Comment on attachment 125913 [details]
Patch

Title says "not for review", so I guess it's not.
Comment 4 Alexey Proskuryakov 2012-02-08 10:58:04 PST
This comes with a test case. What is the bug that's being fixed? The title sounds like refactoring.
Comment 5 Adrienne Walker 2012-02-10 17:48:13 PST
(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.
Comment 6 Peter Kotwicz 2012-02-14 14:02:58 PST
Created attachment 127039 [details]
Patch
Comment 7 Peter Kotwicz 2012-02-14 14:08:42 PST
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?
Comment 8 Early Warning System Bot 2012-02-14 14:50:26 PST
Comment on attachment 127039 [details]
Patch

Attachment 127039 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11521415
Comment 9 Gyuyoung Kim 2012-02-14 15:16:21 PST
Comment on attachment 127039 [details]
Patch

Attachment 127039 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11514460
Comment 10 Philippe Normand 2012-02-14 17:09:54 PST
Comment on attachment 127039 [details]
Patch

Attachment 127039 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11522465
Comment 11 Adrienne Walker 2012-02-15 09:02:03 PST
(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.
Comment 12 Peter Kotwicz 2012-02-15 14:37:00 PST
Created attachment 127239 [details]
Patch
Comment 13 Adrienne Walker 2012-02-15 16:00:57 PST
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.
Comment 14 Alexey Proskuryakov 2012-02-15 17:25:06 PST
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.
Comment 15 Early Warning System Bot 2012-02-15 18:19:35 PST
Comment on attachment 127239 [details]
Patch

Attachment 127239 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11528866
Comment 16 WebKit Review Bot 2012-02-15 18:52:12 PST
Comment on attachment 127239 [details]
Patch

Attachment 127239 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11530855
Comment 17 Gyuyoung Kim 2012-02-15 19:13:19 PST
Comment on attachment 127239 [details]
Patch

Attachment 127239 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11528880
Comment 18 Philippe Normand 2012-02-16 04:55:45 PST
Comment on attachment 127239 [details]
Patch

Attachment 127239 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11536169
Comment 19 WebKit Review Bot 2012-02-16 11:53:41 PST
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.
Comment 20 Peter Kotwicz 2012-02-17 10:45:31 PST
Created attachment 127611 [details]
Patch
Comment 21 Adrienne Walker 2012-02-17 13:03:07 PST
Comment on attachment 127611 [details]
Patch

Awesome.  Can you also then change all the ScrollbarTheme classes to use this interface instead?
Comment 22 Peter Kotwicz 2012-02-17 13:52:36 PST
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)
Comment 23 Adrienne Walker 2012-02-17 14:15:54 PST
(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.
Comment 24 Peter Kotwicz 2012-02-21 12:59:17 PST
Created attachment 128027 [details]
Patch
Comment 25 Peter Kotwicz 2012-02-21 13:07:34 PST
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()
Comment 26 Adrienne Walker 2012-02-21 13:53:11 PST
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
Comment 27 James Robinson 2012-02-21 16:09:16 PST
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?
Comment 28 Peter Kotwicz 2012-02-22 08:02:44 PST
Created attachment 128216 [details]
Patch
Comment 29 Peter Kotwicz 2012-02-22 10:45:07 PST
Created attachment 128245 [details]
Patch
Comment 30 Peter Kotwicz 2012-02-23 09:48:21 PST
Created attachment 128488 [details]
Patch
Comment 31 Philippe Normand 2012-02-23 10:25:33 PST
Comment on attachment 128488 [details]
Patch

Attachment 128488 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11603336
Comment 32 Early Warning System Bot 2012-02-23 10:29:09 PST
Comment on attachment 128488 [details]
Patch

Attachment 128488 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11574350
Comment 33 Peter Kotwicz 2012-02-23 10:46:45 PST
Created attachment 128502 [details]
Patch
Comment 34 Peter Kotwicz 2012-02-23 13:59:11 PST
Created attachment 128552 [details]
Patch
Comment 35 Peter Kotwicz 2012-02-23 16:54:58 PST
Created attachment 128605 [details]
Patch
Comment 36 Adrienne Walker 2012-02-23 16:57:13 PST
(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?
Comment 37 WebKit Commit Bot 2012-02-24 02:12:31 PST
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.
Comment 38 Peter Kotwicz 2012-02-24 15:51:41 PST
Created attachment 128821 [details]
Should make patch build with Safari
Comment 39 WebKit Review Bot 2012-02-24 15:55:33 PST
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.
Comment 40 Peter Kotwicz 2012-02-25 13:29:25 PST
Created attachment 128880 [details]
Fixes style bugs
Comment 41 Adrienne Walker 2012-02-27 14:02:25 PST
This unofficially looks good to me.
Comment 42 James Robinson 2012-02-28 12:31:27 PST
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.
Comment 43 Peter Kotwicz 2012-02-28 14:32:50 PST
Created attachment 129332 [details]
Changes as per jamesr
Comment 44 James Robinson 2012-02-28 15:05:00 PST
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
Comment 45 Peter Kotwicz 2012-03-01 14:32:33 PST
Created attachment 129747 [details]
Build files for Win, GTK, and QT
Comment 46 James Robinson 2012-03-01 15:16:34 PST
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.
Comment 47 James Robinson 2012-03-01 15:36:39 PST
Committed r109451: <http://trac.webkit.org/changeset/109451>