Bug 21269 – Make paint cross-platform on ScrollView

• Component: Platform
• Status: RESOLVED FIXED

[Dave Hyatt]

Make paint cross-platforn on ScrollView

[Dave Hyatt]

Created attachment 23969 [details]
Patch

[Dave Hyatt]

Created attachment 23970 [details]
Patch

[Darin Adler]

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]

"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]

BTW, you critiqued a lot of code that just moved (without changing), but I am game for fixing it anyway! :)

[Dave Hyatt]

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.