Bug 23147 – Introduce Skia to WebKit

• Component: WebKit Misc.
• Status: RESOLVED DUPLICATE

[Dimitri Glazkov (Google)]

Adding Skia to WebCore/platform/graphics

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

Created attachment 26472 [details]
Add WebCore/svg/graphics/skia files

Here are the SVG-related bits.

[Eric Seidel (no email)]

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

Otherwise it looks fine.  I think you can delete the SVGPaintServerGradientSkia* files from the build and it should still work.

[Sam Weinig]

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

[Eric Seidel (no email)]

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 ==

[Dimitri Glazkov (Google)]

Created attachment 26476 [details]
Add WebCore/svg/graphics/skia files v2

Comments addressed on the svg side of things.

[Eric Seidel (no email)]

Comment on attachment 26476 [details]
Add WebCore/svg/graphics/skia files v2

Looks good.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

BTW, deg2rad wasn't used by anything, so just removed it.

[Dimitri Glazkov (Google)]

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 :)

[Dimitri Glazkov (Google)]

Created attachment 26494 [details]
Add platform/graphics/skia/Image* files

Adding another batch, this time the skia/Image* files.

[Dimitri Glazkov (Google)]

Created attachment 26497 [details]
Add a few IntPoint/IntRect, NativeImage, Path, Pattern skia files

A few more files in this batch.

[Eric Seidel (no email)]

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.

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

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

Style:
+                         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,

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

Spacing:
+ImageSource::ImageSource()
+  : m_decoder(0)
+{}

Nope:
+#define private protected
+#include "ImageSource.h"
That needs to be fixed before landing.

[Eric Seidel (no email)]

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:
+#ifdef CHECK_REASONABLE
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.

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

Style:
+    static SkBitmap* misspell_bitmap = 0;

(several in that function)

This is not needed:
+    paint.setColor(this->strokeColor().rgb());

[Dimitri Glazkov (Google)]

Created attachment 26507 [details]
Add remaining bits of platform/graphics/skia

This is the remainder of the platform/graphics/skia directory:

[Oliver Hunt]

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.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

"fixing minor" => "fixing major stylistic faux pas". Sorry, rapid typing syndrome strikes again.

[Eric Seidel (no email)]

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. :(

[Brett Wilson (Google)]

(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?

[Dimitri Glazkov (Google)]

Closing this bug, because 23200 is where it really went down. Or up? :)

*** This bug has been marked as a duplicate of 23200 ***

[Eric Seidel (no email)]

*** Bug 23217 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Comment on attachment 26493 [details]
Added WebCore/platform/graphics/skia/GraphicsContext* files

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