Bug 23147

Summary: Introduce Skia to WebKit
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Severity: Normal CC: brettw, eric, feng, mike, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Bug Depends on: 23200    
Bug Blocks:    
Description Flags
Add WebCore/platform/graphics/skia files
eric: review-
Add WebCore/svg/graphics/skia files
eric: review-
Add WebCore/svg/graphics/skia files v2
eric: review+
Add WebCore/platform/graphics/skia files v2
eric: review+
Added WebCore/platform/graphics/skia/GraphicsContext* files
Add platform/graphics/skia/Image* files
eric: review-
Add a few IntPoint/IntRect, NativeImage, Path, Pattern skia files
eric: review+
Add remaining bits of platform/graphics/skia oliver: review-

Description Dimitri Glazkov (Google) 2009-01-06 13:17:26 PST
Adding Skia to WebCore/platform/graphics
Comment 1 Dimitri Glazkov (Google) 2009-01-06 13:44:01 PST
Created attachment 26470 [details]
Add WebCore/platform/graphics/skia files

If this is a bit too large, please let me know and I will split up. I didn't write this code, but will coordinate any style-related, etc. changes.
Comment 2 Dimitri Glazkov (Google) 2009-01-06 14:02:00 PST
Created attachment 26472 [details]
Add WebCore/svg/graphics/skia files

Here are the SVG-related bits.
Comment 3 Eric Seidel (no email) 2009-01-06 14:18:26 PST
Comment on attachment 26472 [details]
Add WebCore/svg/graphics/skia files

We don't use SVGPaintServerGradientSkia anymore.  GradientSkia.cpp should be taking care of this.  There is a x-platform implementation of SVGPaintServerGradient which uses Gradient from platform instead.

SVGPaintServerPatternSkia is slated to die (the same death as SVGPaintServerGradient*, but it looks like that patch hasn't landed yet).

I'm surprised we don't have a nicer way than this:
+        IntRect textBoundary = const_cast<RenderObject*>(object)->absoluteBoundingBoxRect();
+        targetRect = object->absoluteTransform().inverse().mapRect(textBoundary);
it seems CG does the same thing though. :(

We use 0 instead of NULL for c++ code according to the style guidelines:
+    context->platformContext()->setGradient(NULL);

We don't generally commit ifdef'd out code, so this should be removed:
+  // TODO(jhaas): implement me
+#if 0
+    cairo_t* cr = context->platformContext();
+    cairo_surface_t* surface = mask()->surface();
+    if (!surface)
+        return;
+    cairo_pattern_t* mask = cairo_pattern_create_for_surface(surface);
+    cairo_mask(cr, mask);
+    cairo_pattern_destroy(mask);

Otherwise it looks fine.  I think you can delete the SVGPaintServerGradientSkia* files from the build and it should still work.
Comment 4 Sam Weinig 2009-01-06 14:19:55 PST
Comment on attachment 26470 [details]
Add WebCore/platform/graphics/skia files

I have only reviewed up to GradientSkia.  I am not sure what GdkSkia is for, and therefore, it could use explanation in the ChangeLog.

> +static const double deg2rad = 0.017453292519943295769; // pi/180

This should already be defined in WTF/MathExtras.h

> +AffineTransform::AffineTransform(const SkMatrix& matrix) : m_transform(matrix)

The : m_transform(matrix) should be on the next line.

> +
> +void AffineTransform::map(double x, double y, double *x2, double *y2) const

* on the wrong side.

> +IntRect AffineTransform::mapRect(const IntRect& src) const
> +{
> +    SkRect  dst;

Extra space.

> +
> +AffineTransform &AffineTransform::scale(double sx, double sy)

& on the wrong side.

> +AffineTransform &AffineTransform::translate(double tx, double ty)

& on the wrong side.

> +AffineTransform &AffineTransform::shear(double sx, double sy)

& on the wrong side.

> +AffineTransform &AffineTransform::operator*=(const AffineTransform& m2)

& on the wrong side.

> +
> +double AffineTransform::a() const
> +{
> +    return SkScalarToDouble(m_transform.getScaleX());
> +}
> +void AffineTransform::setA(double a)

Missing newline.

> +double AffineTransform::b() const
> +{
> +    return SkScalarToDouble(m_transform.getSkewY());
> +}
> +void AffineTransform::setB(double b)

Missing newline.

> +double AffineTransform::c() const
> +{
> +    return SkScalarToDouble(m_transform.getSkewX());
> +}
> +void AffineTransform::setC(double c)

Missing newline.

> +double AffineTransform::d() const
> +{
> +    return SkScalarToDouble(m_transform.getScaleY());
> +}
> +void AffineTransform::setD(double d)

Missing newline.

> +double AffineTransform::e() const
> +{
> +    return SkScalarToDouble(m_transform.getTranslateX());
> +}
> +void AffineTransform::setE(double e)

Missing newline.

> +double AffineTransform::f() const
> +{
> +    return SkScalarToDouble(m_transform.getTranslateY());
> +}
> +void AffineTransform::setF(double f)

Missing newline.

> +
> +// Determine the total number of stops needed, including pseudo-stops at the
> +// ends as necessary.
> +static size_t total_stops_needed(const Gradient::ColorStop* stopData, size_t count)

Please use camelCase.

> +{
> +    const Gradient::ColorStop* stop = stopData;
> +    size_t count_used = count;
> +    if (count < 1 || stop->stop > 0.0)
> +      count_used++;
> +    stop += count - 1;
> +    if (count < 2 || stop->stop < 1.0)
> +      count_used++;
> +    return count_used;

Indentation seems wrong here.

> +}
> +
> +// Collect sorted stop position and color information into the pos and colors 
> +// buffers, ensuring stops at both 0.0 and 1.0.  The buffers must be large
> +// enough to hold information for all stops, including the new endpoints if
> +// stops at 0.0 and 1.0 aren't already included.
> +static void fill_stops(const Gradient::ColorStop* stopData,
> +                       size_t count, SkScalar* pos, SkColor* colors)

Please use camelCase.

> +SkShader* Gradient::platformGradient()
> +{
> +    if (m_gradient)
> +        return m_gradient;
> +
> +    // TODO: This and compareStops() are also in Gradient.cpp and

This should be a FIXME.

> +    // CSSGradientValue.cpp; probably should refactor in WebKit.
> +    if (!m_stopsSorted) {
> +        if (m_stops.size())
> +            std::stable_sort(m_stops.begin(), m_stops.end(), compareStops);
> +        m_stopsSorted = true;
> +    }
> +    size_t count_used = total_stops_needed(m_stops.data(), m_stops.size());

Please use camelCase.

> +    ASSERT(count_used >= 2);
> +    ASSERT(count_used >= m_stops.size());
> +
> +    // FIXME: Why is all this manual pointer math needed?!
> +    SkAutoMalloc storage(count_used * (sizeof(SkColor) + sizeof(SkScalar)));
> +    SkColor* colors = (SkColor*)storage.get();
> +    SkScalar* pos = (SkScalar*)(colors + count_used);

We prefer c++ style casts.

> +
> +    fill_stops(m_stops.data(), m_stops.size(), pos, colors);
> +
> +    if (m_radial) {
> +        // TODO(mmoss) CSS radial Gradients allow an offset focal point (the

This should be a FIXME, preferably with a bug #.
Comment 5 Eric Seidel (no email) 2009-01-06 14:27:41 PST
Comment on attachment 26470 [details]
Add WebCore/platform/graphics/skia files

AffineTransformSkia.cpp needs to be renamed to TransformationMatrixSkia.cpp (and the class too) to match recent renames upstream.

I don't understand what GdkSkia.cpp is for.  Seems it should be in its own separate patch.

WebKit style would use coundUsed instead of count_used

We don't use c-style casts in C++ code:
+    SkColor* colors = (SkColor*)storage.get();

GraphicsContextSkia needs its own patch.  It's too big to grok as part of this one.

Image buffer probably too.

== stopped reading ==
Comment 6 Dimitri Glazkov (Google) 2009-01-06 14:59:57 PST
Created attachment 26476 [details]
Add WebCore/svg/graphics/skia files v2

Comments addressed on the svg side of things.
Comment 7 Eric Seidel (no email) 2009-01-06 15:08:09 PST
Comment on attachment 26476 [details]
Add WebCore/svg/graphics/skia files v2

Looks good.
Comment 8 Dimitri Glazkov (Google) 2009-01-06 16:16:39 PST
Created attachment 26479 [details]
Add WebCore/platform/graphics/skia files v2

Comments addressed. I also truncated the patch at the point where you stopped. Will add the rest as multiple separate patches.
Comment 9 Dimitri Glazkov (Google) 2009-01-06 16:17:35 PST
BTW, deg2rad wasn't used by anything, so just removed it.
Comment 10 Dimitri Glazkov (Google) 2009-01-07 09:54:19 PST
Created attachment 26493 [details]
Added WebCore/platform/graphics/skia/GraphicsContext* files

Adding Skia GraphicsContext impl. Geoffrey, I am spreading the love, so that Sam doesn't feel too lonely :)
Comment 11 Dimitri Glazkov (Google) 2009-01-07 10:33:08 PST
Created attachment 26494 [details]
Add platform/graphics/skia/Image* files

Adding another batch, this time the skia/Image* files.
Comment 12 Dimitri Glazkov (Google) 2009-01-07 11:21:27 PST
Created attachment 26497 [details]
Add a few IntPoint/IntRect, NativeImage, Path, Pattern skia files

A few more files in this batch.
Comment 13 Eric Seidel (no email) 2009-01-07 12:14:11 PST
Comment on attachment 26497 [details]
Add a few IntPoint/IntRect, NativeImage, Path, Pattern skia files

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.

+    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.

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

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

+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.

+    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);

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

+        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));

+    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.
Comment 14 Eric Seidel (no email) 2009-01-07 13:08:46 PST
Comment on attachment 26479 [details]
Add WebCore/platform/graphics/skia files v2

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.
Comment 15 Eric Seidel (no email) 2009-01-07 13:30:09 PST
Comment on attachment 26494 [details]
Add platform/graphics/skia/Image* files

0 is the WebKit way:
+    , m_platformContext(NULL)  // Canvas is set in ImageBuffer constructor.

Not needed in Webkit Style:
+    : m_canvas()

Wow, strange to have a success parameter on a constructor.

Best line ever:
+    unsigned char* data = result->data()->data().data();

I think WebKit style would have these be true camelCase instead of lowercase:
+    int originx = rect.x();
+    int destx = 0;
originX, destX

+    if (srcWidth == destIWidth && srcHeight == destIHeight) {
+        // We don't need to resample if the source and destination are the same.
+        return RESAMPLE_NONE;
+    }

+                         float src_width,
+                         float src_height,
+                         float* dest_width,
+                         float* dest_height) {

I thought gfx:: was off-limits from WebCore code?
+        gfx::Rect destBitmapSubset(destBitmapSubsetSkI.fLeft,

+    else {
+      resampling = computeResamplingMode(*bitmap,
+                                         srcRect.width(), srcRect.height(),
+                                         dest_bitmap_width, dest_bitmap_height);
+    }

We should probably write out "bitmap" or "skImage" here:
+    const NativeImageSkia* bm = nativeImageForCurrentFrame();
+    if (!bm)
+        return;  // It's too early and we don't have an image yet.

+  : m_decoder(0)

+#define private protected
+#include "ImageSource.h"
That needs to be fixed before landing.
Comment 16 Eric Seidel (no email) 2009-01-07 13:47:05 PST
Comment on attachment 26493 [details]
Added WebCore/platform/graphics/skia/GraphicsContext* files

3 #include "wtf/MathExtras.h"
should be <wtf/MathExtras.h>

I hate these:
Sigh.  I know they're necessary for now.

+    if (fillColor().rgb() & 0xFF000000) {:
I prefer somthing like fillColor().hasAlpha() personally.  Or in this case I think it's making sure it's non-transparent.  There is more than one of these.

+    SkRegion exterior_region;
+    const SkScalar exterior_offset = WebCoreFloatToSkScalar(0.5);

+    static SkBitmap* misspell_bitmap = 0;

(several in that function)

This is not needed:
+    paint.setColor(this->strokeColor().rgb());
Comment 17 Dimitri Glazkov (Google) 2009-01-07 13:51:41 PST
Created attachment 26507 [details]
Add remaining bits of platform/graphics/skia

This is the remainder of the platform/graphics/skia directory:
Comment 18 Oliver Hunt 2009-01-07 14:05:52 PST
Comment on attachment 26507 [details]
Add remaining bits of platform/graphics/skia

I'd like this patch split up somewhat more.  Ideally basic graphic context stuff separate from the font stuff, for that reaosn r- (Font stuff is sufficiently complex for me to want someone else to review it, i don't know whether it should just be r+'d as a platform component for instance, or whether there are real rules that are necessary)

But other issues
WebCoreRectToSkiaRect, etc -> these should not be in SkieUtils, they should be in IntRectSkia, etc as with the other ports
i'm not sure i like WebCoreCompositeToSkiaComposite, but i feel GraphicsContextSkia would be best
I don't have a clue what the hell SkColor SkPMColorToColor(SkPMColor pm) things are :D

In general *Utils files are bad, functions should be placed in the files of appropriate relevance, eg. IntRectSkia, and so on and so forth.

Didn't look at style at all.
Comment 19 Dimitri Glazkov (Google) 2009-01-07 14:21:06 PST
I am not sure what the right approach is here. I am very hesitant to make any non-style-related changes to already working code, yet I can not really test the changes unless I first make them downstream, then copy upstream, because the port migration is obviously incomplete and I can't test the changes upstream. And that would take double or more amount of time.

I realize that I might have been overly optimistic when I signed up to do this :) but I think we should figure out some sort of an approach that allows us to do this more efficiently.

In my idealistic view, I imagined that we would break this up into phases:

a) get the port upstream, fixing minor 
b) make it build
c) continue improving testable, working code

Attempting to refactor in-flight just doesn't seem like a very prudent thing to do. Though I am pretty sure our port doesn't represent "perfect code', it is a working port.
Comment 20 Dimitri Glazkov (Google) 2009-01-07 14:22:30 PST
"fixing minor" => "fixing major stylistic faux pas". Sorry, rapid typing syndrome strikes again.
Comment 21 Eric Seidel (no email) 2009-01-07 14:25:06 PST
Comment on attachment 26493 [details]
Added WebCore/platform/graphics/skia/GraphicsContext* files

Bah, hit the <submit> button by accident, and now I've lost my place. :(
Comment 22 Brett Wilson (Google) 2009-01-07 15:51:59 PST
(In reply to comment #18)
> WebCoreRectToSkiaRect, etc -> these should not be in SkieUtils, they should be
> in IntRectSkia, etc as with the other ports

These are extra and can be deleted (they have no entry in the header file, and whoever deleted it forgot to delete it from the .cpp).

> i'm not sure i like WebCoreCompositeToSkiaComposite, but i feel
> GraphicsContextSkia would be best

I don't know what you're saying here. This function is used from both GraphicsContextSkia and from ImageSkia (anywhere we're converting composite types). Where would you like it to live?
Comment 23 Dimitri Glazkov (Google) 2009-01-08 20:45:53 PST
Closing this bug, because 23200 is where it really went down. Or up? :)

*** This bug has been marked as a duplicate of 23200 ***
Comment 24 Eric Seidel (no email) 2009-01-09 15:39:00 PST
*** Bug 23217 has been marked as a duplicate of this bug. ***
Comment 25 Eric Seidel (no email) 2009-01-09 15:43:20 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/svg/graphics/skia/RenderPathSkia.cpp
	A	WebCore/svg/graphics/skia/SVGPaintServerPatternSkia.cpp
	A	WebCore/svg/graphics/skia/SVGPaintServerSkia.cpp
	A	WebCore/svg/graphics/skia/SVGResourceFilterSkia.cpp
	A	WebCore/svg/graphics/skia/SVGResourceMaskerSkia.cpp
Committed r39768
Comment 26 Eric Seidel (no email) 2009-02-05 14:25:24 PST
Comment on attachment 26493 [details]
Added WebCore/platform/graphics/skia/GraphicsContext* files

Clearing review flag so it's out of the queue.