2009-02-05 Scott Violet <sky@google.com>

        Reviewed by Eric Seidel.

        https://bugs.webkit.org/show_bug.cgi?id=23625
        Additional fix: Skia platform doesn't render text to a canvas or support clipping to an image buffer

        Fixes three bugs in PlatformContextSkia:

        * When a new layer was started clipped to an image we used the
          assignment operator to copy the SkBitmap. If the SkBitmap owns it's
          pixels, this is not the right thing to do. Instead we need to create
          a copy of the image.
        * State holds an SkBitmap by value. State's copy constructor does a
          memcpy. This is confusing and subtle, I've converted to use a member
          initializer list which I think is clearer and less error prone.
        * When creating a new layer there is no need to copy the clip image.

        * platform/graphics/skia/PlatformContextSkia.cpp:
        (PlatformContextSkia::State::State):
        (PlatformContextSkia::save):
        (PlatformContextSkia::beginLayerClippedToImage):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@40689 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 23d6699e2efda8e06d83b53ae3ea2fa40e8e5309..99228e7343114d89cdc994139f87b3d4e6216909 100644 (file)
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,26 @@
+2009-02-05  Scott Violet  <sky@google.com>
+
+        Reviewed by Eric Seidel.
+
+        https://bugs.webkit.org/show_bug.cgi?id=23625
+        Additional fix: Skia platform doesn't render text to a canvas or support clipping to an image buffer
+
+        Fixes three bugs in PlatformContextSkia:
+
+        * When a new layer was started clipped to an image we used the
+          assignment operator to copy the SkBitmap. If the SkBitmap owns it's
+          pixels, this is not the right thing to do. Instead we need to create
+          a copy of the image.
+        * State holds an SkBitmap by value. State's copy constructor does a
+          memcpy. This is confusing and subtle, I've converted to use a member
+          initializer list which I think is clearer and less error prone.
+        * When creating a new layer there is no need to copy the clip image.
+
+        * platform/graphics/skia/PlatformContextSkia.cpp:
+        (PlatformContextSkia::State::State):
+        (PlatformContextSkia::save):
+        (PlatformContextSkia::beginLayerClippedToImage):
+

 2009-02-05  Scott Violet  <sky@google.com>
 
         Reviewed by George Staikos.
 2009-02-05  Scott Violet  <sky@google.com>
 
         Reviewed by George Staikos.

diff --git a/WebCore/platform/graphics/skia/PlatformContextSkia.cpp b/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
index 8b128474d0784f6c9c2de51c5131cee49d995a79..73256a22d3c7448168a2e32390d66d50d1e8fae6 100644 (file)
--- a/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
+++ b/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
@@ -121,9 +121,28 @@ PlatformContextSkia::State::State()
 }
 
 PlatformContextSkia::State::State(const State& other)
 }
 
 PlatformContextSkia::State::State(const State& other)

+    : m_alpha(other.m_alpha)
+    , m_porterDuffMode(other.m_porterDuffMode)
+    , m_gradient(other.m_gradient)
+    , m_pattern(other.m_pattern)
+    , m_useAntialiasing(other.m_useAntialiasing)
+    , m_looper(other.m_looper)
+    , m_fillColor(other.m_fillColor)
+    , m_strokeStyle(other.m_strokeStyle)
+    , m_strokeColor(other.m_strokeColor)
+    , m_strokeThickness(other.m_strokeThickness)
+    , m_dashRatio(other.m_dashRatio)
+    , m_miterLimit(other.m_miterLimit)
+    , m_lineCap(other.m_lineCap)
+    , m_lineJoin(other.m_lineJoin)
+    , m_dash(other.m_dash)
+    , m_textDrawingMode(other.m_textDrawingMode)
+#if defined(__linux__) || PLATFORM(WIN_OS)
+    , m_imageBufferClip(other.m_imageBufferClip)
+    , m_clip(other.m_clip)
+#endif

 {
 {

-    memcpy(this, &other, sizeof(State));
-
+    // Up the ref count of these. saveRef does nothing if 'this' is NULL.

     m_looper->safeRef();
     m_dash->safeRef();
     m_gradient->safeRef();
     m_looper->safeRef();
     m_dash->safeRef();
     m_gradient->safeRef();


@@ -199,6 +218,12 @@ void PlatformContextSkia::save()
     m_stateStack.append(*m_state);
     m_state = &m_stateStack.last();
 
     m_stateStack.append(*m_state);
     m_state = &m_stateStack.last();
 

+#if defined(__linux__) || PLATFORM(WIN_OS)
+    // The clip image only needs to be applied once. Reset the image so that we
+    // don't attempt to clip multiple times.
+    m_state->m_imageBufferClip.reset();
+#endif
+

     // Save our native canvas.
     canvas()->save();
 }
     // Save our native canvas.
     canvas()->save();
 }


@@ -217,9 +242,22 @@ void PlatformContextSkia::beginLayerClippedToImage(const WebCore::FloatRect& rec
     canvas()->saveLayerAlpha(&bounds, 255,
                              static_cast<SkCanvas::SaveFlags>(SkCanvas::kHasAlphaLayer_SaveFlag | SkCanvas::kFullColorLayer_SaveFlag));
     // Copy off the image as |imageBuffer| may be deleted before restore is invoked.
     canvas()->saveLayerAlpha(&bounds, 255,
                              static_cast<SkCanvas::SaveFlags>(SkCanvas::kHasAlphaLayer_SaveFlag | SkCanvas::kFullColorLayer_SaveFlag));
     // Copy off the image as |imageBuffer| may be deleted before restore is invoked.

-    m_state->m_imageBufferClip = *(imageBuffer->context()->platformContext()->bitmap());
+    const SkBitmap* bitmap = imageBuffer->context()->platformContext()->bitmap();
+    if (!bitmap->pixelRef()) {
+        // The bitmap owns it's pixels. This happens when we've allocated the
+        // pixels in some way and assigned them directly to the bitmap (as
+        // happens when we allocate a DIB). In this case the assignment operator
+        // does not copy the pixels, rather the copied bitmap ends up
+        // referencing the same pixels. As the pixels may not live as long as we
+        // need it to, we copy the image.
+        bitmap->copyTo(&m_state->m_imageBufferClip, SkBitmap::kARGB_8888_Config);
+    } else {
+        // If there is a pixel ref, we can safely use the assignment operator.
+        m_state->m_imageBufferClip = *bitmap;
+    }

 }
 #endif
 }
 #endif

+

 void PlatformContextSkia::restore()
 {
 #if defined(__linux__) || PLATFORM(WIN_OS)
 void PlatformContextSkia::restore()
 {
 #if defined(__linux__) || PLATFORM(WIN_OS)