WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
21269
Make paint cross-platform on ScrollView
https://bugs.webkit.org/show_bug.cgi?id=21269
Summary
Make paint cross-platform on ScrollView
Dave Hyatt
Reported
2008-09-30 23:52:16 PDT
Make paint cross-platforn on ScrollView
Attachments
Patch
(45.31 KB, patch)
2008-09-30 23:55 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Patch
(45.38 KB, patch)
2008-10-01 00:02 PDT
,
Dave Hyatt
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Dave Hyatt
Comment 1
2008-09-30 23:55:01 PDT
Created
attachment 23969
[details]
Patch
Dave Hyatt
Comment 2
2008-10-01 00:02:35 PDT
Created
attachment 23970
[details]
Patch
Darin Adler
Comment 3
2008-10-01 09:35:51 PDT
Comment on
attachment 23970
[details]
Patch #include "ScrollView.h" + #include "IntSize.h" +#include "RenderLayer.h" #include <wtf/Forward.h> #include <wtf/OwnPtr.h> These should all be sorted, and ScrollView shouldn't be in a separate paragraph. I appreciate the thought that the base class include might be a different category than the others, but that's not our coding style approach to includes -- we just sort them all. + virtual void paintContents(GraphicsContext* context, const IntRect& damageRect); You can leave out the argument name "context" here. + static double sCurrentPaintTimeStamp; // used for detecting decoded resource thrash in the cache Two thoughts: 1) Why not just use a normal global variable with internal linkage instead of a static data member? There's no real reason to have this in the FrameView.h header at all. 2) If it is going to be a static data member, what is our naming scheme? It's annoying to have data members be "m_name" and static data members be "sName". I'd love it if we could be consistent, at least for new code. NSImage* Frame::selectionImage(bool forceBlackText) const { - d->m_paintRestriction = forceBlackText ? PaintRestrictionSelectionOnlyBlackText : PaintRestrictionSelectionOnly; + d->m_view->setPaintRestriction(forceBlackText ? PaintRestrictionSelectionOnlyBlackText : PaintRestrictionSelectionOnly); Does this function need a FrameView null check? What about Frame::snapshotDragImage, Frame::nodeImage, and imageFromSelection? + static RefPtr<Image> panScrollIcon; + if (m_drawPanScrollIcon) { + if (!panScrollIcon) + panScrollIcon = Image::loadPlatformResource("panIcon"); + context->drawImage(panScrollIcon.get(), m_panScrollIconPoint); + } I'd write it like this: if (m_drawPanScrollIcon) { static Image* panScrollIcon = Image::loadPlatformResource("panIcon").release().releaseRef(); context->drawImage(panScrollIcon, m_panScrollIconPoint); } I think it's nicer to scope the panScrollIcon variable tighter, to just let the compiler do the one-time initialization rather than writing it out explicitly, and to use a raw pointer so we don't waste time decrementing the reference count when quitting. + virtual void paint(GraphicsContext*, const IntRect&); + protected: virtual void repaintContentRectangle(const IntRect&, bool now = false); + virtual void paintContents(GraphicsContext* context, const IntRect& damageRect) = 0; I think you can omit the argument name "context" in paintContents too; and I also suggest being consistent about whether you give the name "damageRect" or not; both or neither would be better. Also, you could consider making paintContents private instead of protected. It should only be protected if derived classes need to call it. In ScrollbarTheme.h if you're going to add the include of GraphicsContext, then you should remove the forward declaration of the GraphicsContext class. +void ScrollbarThemeComposite::paintScrollCorner(ScrollView* view, GraphicsContext* context, const IntRect& cornerRect) +{ + FrameView* frameView = static_cast<FrameView*>(view); + Page* page = frameView->frame() ? frameView->frame()->page() : 0; Weren't you planning to get rid of these casts to FrameView using your new HostWindow class? + _private->coreFrame->view()->paintContents(&context, enclosingIntRect(rect)); Do we need a null check of FrameView here? + coreFrame->view()->paintContents(&spoolCtx, pageRect); What about here? r=me
Dave Hyatt
Comment 4
2008-10-01 10:23:36 PDT
"Weren't you planning to get rid of these casts to FrameView using your new HostWindow class?" The custom scrollbar painting API for Windows is something I'm just going to ditch completely once scrollbars can be styled with CSS, so I'm not bothering to create a new host window API for it.
Dave Hyatt
Comment 5
2008-10-01 10:24:31 PDT
BTW, you critiqued a lot of code that just moved (without changing), but I am game for fixing it anyway! :)
Dave Hyatt
Comment 6
2008-10-01 10:51:55 PDT
Your suggestion for how to change the pan scroll image initialization did not compile, so I ended up just leaving it alone for now. Thanks for the review! Fixed in
r37146
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug