Summary: | Make paint cross-platform on ScrollView | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||||
Component: | Platform | Assignee: | Dave Hyatt <hyatt> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 21083 | ||||||||
Attachments: |
|
Description
Dave Hyatt
2008-09-30 23:52:16 PDT
Created attachment 23969 [details]
Patch
Created attachment 23970 [details]
Patch
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
"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. BTW, you critiqued a lot of code that just moved (without changing), but I am game for fixing it anyway! :) |