[webkit-reviews] review granted: [Bug 23147] Introduce Skia to WebKit : [Attachment 26479] Add WebCore/platform/graphics/skia files v2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 7 13:08:46 PST 2009


Eric Seidel <eric at webkit.org> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 23147: Introduce Skia to WebKit
https://bugs.webkit.org/show_bug.cgi?id=23147

Attachment 26479: Add WebCore/platform/graphics/skia files v2
https://bugs.webkit.org/attachment.cgi?id=26479&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
We only name arguments when the name adds clarity:
+    static PassRefPtr<BitmapImageSingleFrameSkia> create(
+	 const SkBitmap& bitmap);
This one doesn't need a name, per the style guidelines.

I'm surprised there isn't just a m_nativeImage.size() accessor:
+	 return IntSize(m_nativeImage.width(), m_nativeImage.height());

Also, there is no reason for these virtual functions to be in the headers. 
They should just move to the .cpp file IMO (they're virtual, so won't be
inlined).

+    virtual void draw(GraphicsContext* ctxt, const FloatRect& dstRect,
+		       const FloatRect& srcRect, CompositeOperator
compositeOp);
compositeOp and ctxt don't need to be there.

Where's BitmapImageSingleFrameSkia.cpp?  :)  Looks fine.  Needs the above
changes and can be landed.  Marking r+ assuming you'll change before landing. 
If you're still sans commit-bit, then please post a new patch.


More information about the webkit-reviews mailing list