Summary: | Decouple scrollbar painting from Scrollbar object | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Kotwicz <pkotwicz> | ||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | andersca, ap, commit-queue, dglazkov, enne, gustavo, jamesr, pkotwicz, pnormand, rakuco, trchen, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||
Bug Blocks: | 78872, 79137 | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Peter Kotwicz
2012-02-07 13:27:49 PST
Created attachment 125913 [details]
Patch
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 on attachment 125913 [details]
Patch
Title says "not for review", so I guess it's not.
This comes with a test case. What is the bug that's being fixed? The title sounds like refactoring. (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. Created attachment 127039 [details]
Patch
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 on attachment 127039 [details] Patch Attachment 127039 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11521415 Comment on attachment 127039 [details] Patch Attachment 127039 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11514460 Comment on attachment 127039 [details] Patch Attachment 127039 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11522465 (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. Created attachment 127239 [details]
Patch
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. 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 on attachment 127239 [details] Patch Attachment 127239 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11528866 Comment on attachment 127239 [details] Patch Attachment 127239 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11530855 Comment on attachment 127239 [details] Patch Attachment 127239 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11528880 Comment on attachment 127239 [details] Patch Attachment 127239 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11536169 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. Created attachment 127611 [details]
Patch
Comment on attachment 127611 [details]
Patch
Awesome. Can you also then change all the ScrollbarTheme classes to use this interface instead?
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) (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. Created attachment 128027 [details]
Patch
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() 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 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? Created attachment 128216 [details]
Patch
Created attachment 128245 [details]
Patch
Created attachment 128488 [details]
Patch
Comment on attachment 128488 [details] Patch Attachment 128488 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11603336 Comment on attachment 128488 [details] Patch Attachment 128488 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11574350 Created attachment 128502 [details]
Patch
Created attachment 128552 [details]
Patch
Created attachment 128605 [details]
Patch
(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? 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.
Created attachment 128821 [details]
Should make patch build with Safari
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.
Created attachment 128880 [details]
Fixes style bugs
This unofficially looks good to me. 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. Created attachment 129332 [details]
Changes as per jamesr
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 Created attachment 129747 [details]
Build files for Win, GTK, and QT
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.
Committed r109451: <http://trac.webkit.org/changeset/109451> |