[webkit-reviews] review granted: [Bug 23147] Introduce Skia to WebKit : [Attachment 26497] Add a few IntPoint/IntRect, NativeImage, Path, Pattern skia files

[bugzilla-daemon at webkit.org]

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 26497: Add a few IntPoint/IntRect, NativeImage, Path, Pattern skia
files
https://bugs.webkit.org/attachment.cgi?id=26497&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I'm not sure why SkIntToScalar is needed here:
+    SkPoint p = { SkIntToScalar(m_x), SkIntToScalar(m_y) };

Then again, I think I wrote that code, so maybe I should remember better. :( 
IIRC the conversion functions are used for sanity checking to keep skia from
barfing on itself?  Either way, if the code works for the Chromium port I
shouldn't complain much.  It looks sane either way.

Style:
+    if (m_lastRequestSize.width() == w && m_lastRequestSize.height() == h) {
+	 m_resizeRequests++;
+    } else {

m_resizeRequests is a bad name for what that variable seems to do.  Also, it
seems that resizedBitmap called w/o first calling hasResizedBitmap never
increments the m_resizeRequests which may or may not be intentional.

Style:
+    if (m_resizedImage.width() != w || m_resizedImage.height() != h) {
+	 m_resizedImage = skia::ImageOperations::Resize(*this,
+	     skia::ImageOperations::RESIZE_LANCZOS3, w, h);
+    }

Style:
+bool NativeImageSkia::shouldCacheResampling(int dest_width,
+					     int dest_height,
+					     int dest_subset_width,
+					     int dest_subset_height) const
+{

Spacing:
+int NativeImageSkia::decodedSize() const
+{
+  return getSize() + m_resizedImage.getSize();
+}

It seems like these should possibly change to IntSize instead of a width/height
pair:
bool hasResizedBitmap(int width, int height) const;

Same here:
+    bool shouldCacheResampling(int dest_width,
+				int dest_height,
+				int dest_subset_width,
+				int dest_subset_height) const;

Is NativeImageSkia intentionally outside of the WebCore namespace?  That seems
odd.

I wonder why m_path can't be a OwnPtr.	Probably due to using "PlatformPath" as
the storage type.  We may consider adding a OwnedPalformPath or some such
typedef later, to allow CG to use RetainPtr<PlatformPath> and us to use
OwnPtr<PlatformPath>, etc.

Spacing:
+    return SkPathContainsPoint(
+      m_path,
+      point, 
+      rule == RULE_NONZERO ? SkPath::kWinding_FillType :
SkPath::kEvenOdd_FillType);
I'd probably have written that as:
SkFillRule fillRule = rule == RULE_NONZERO ? SkPath::kWinding_FillType :
SkPath::kEvenOdd_FillType;
return SkPathContainsPoint(m_path, point, fillRule);
if I were writing it (I know you didn't write it).

Extra spaces?
+    return FloatRect(	 SkScalarToFloat(r.fLeft),
+			 SkScalarToFloat(r.fTop),
+			 SkScalarToFloat(r.width()),
+			 SkScalarToFloat(r.height()));

Also, we don't need to do manual conversion to FloatRect there.  Just returning
"r" should convert transparently due to FloatRectSkia.

If Skia has SkPoint versions of these functions:
+    m_path->moveTo(WebCoreFloatToSkScalar(point.x()),
WebCoreFloatToSkScalar(point.y()));
+    m_path->lineTo(WebCoreFloatToSkScalar(p.x()),
WebCoreFloatToSkScalar(p.y()));

Then no manual conversion is needed there either.

Extra spaces?
+    SkScalar	 cx = WebCoreFloatToSkScalar(p.x());
+    SkScalar	 cy = WebCoreFloatToSkScalar(p.y());
+    SkScalar	 radius = WebCoreFloatToSkScalar(r);

Style:
+    if (sweep >= 2*gPI || sweep <= -2*gPI) {
+	 m_path->addOval(oval);
+    } else {

Style:
+	 if (anticlockwise && sweepDegrees > 0) {
+	     sweepDegrees -= SkIntToScalar(360);
+	 } else if (!anticlockwise && sweepDegrees < 0) {
+	     sweepDegrees += SkIntToScalar(360);
+	 }

We don't generally commit commented out code:
+//	   SkDebugf("addArc sa=%g ea=%g cw=%d start=%g sweep=%g\n", sa, ea,
clockwise,
+//		    SkScalarToFloat(startDegrees),
SkScalarToFloat(sweepDegrees));
+

Style:
+    for (int i = 0; i < count; i++)
+    {
+	 dst[i].setX(SkScalarToFloat(src[i].fX));

Could use a clearer name:
+static FloatPoint* setfpts(FloatPoint dst[], const SkPoint src[], int count)

We should probably use something other than String for result in debugString().
 StringBuilder or Vector<UChar> would be fine.	I'm not sure we really care
about perf of debugString, but in general String has awful append perf.

In general this patch looks great.  It could just use a little cleanup before
landing.  I don't need to see it again.  Marking r+ assuming you're going to
land it yourself after cleanup.