[webkit-reviews] review granted: [Bug 21269] Make paint cross-platform on ScrollView : [Attachment 23970] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dave Hyatt <hyatt at apple.com>'s
request for review:
Bug 21269: Make paint cross-platform on ScrollView
https://bugs.webkit.org/show_bug.cgi?id=21269

Attachment 23970: Patch
https://bugs.webkit.org/attachment.cgi?id=23970&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
 #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