Decouple Canvas from DisplayList and map Picture and PictureRecorder more directly to their Skia co…

sky/engine/core/painting/Canvas.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef SKY_ENGINE_CORE_PAINTING_CANVAS_H_
 #define SKY_ENGINE_CORE_PAINTING_CANVAS_H_
 
 #include "sky/engine/core/painting/CanvasPath.h"
 #include "sky/engine/core/painting/Paint.h"
 #include "sky/engine/core/painting/Picture.h"
+#include "sky/engine/core/painting/PictureRecorder.h"
 #include "sky/engine/core/painting/RRect.h"
 #include "sky/engine/core/painting/Rect.h"
 #include "sky/engine/platform/graphics/DisplayList.h"
 #include "sky/engine/tonic/dart_wrappable.h"
 #include "sky/engine/tonic/float32_list.h"
 #include "sky/engine/wtf/PassRefPtr.h"
 #include "sky/engine/wtf/RefCounted.h"
+#include "third_party/skia/include/core/SkCanvas.h"
 
 namespace blink {
-class Element;
 class CanvasImage;
 
 class Canvas : public RefCounted<Canvas>, public DartWrappable {
     DEFINE_WRAPPERTYPEINFO();
 public:
-    Canvas(const FloatSize& size);
+    static PassRefPtr<Canvas> create(SkCanvas* skCanvas) {
+        ASSERT(skCanvas);
+        return adoptRef(new Canvas(skCanvas));
+    }
+
+    static PassRefPtr<Canvas> create(PassRefPtr<PictureRecorder> recorder,

[abarth-chromium, 2015/06/23 02:28:44]

I think what Eric is saying is that ideally |PassRefPtr<PictureRecorder>| would
be |PictureRecorder*| because we're not going to keep a reference to the
|PictureRecorder|.  It's possible that doesn't compile because of the IDL code
generator, in which case what you've got here is fine.

[iansf, 2015/06/23 22:46:09]

Confirmed that |PictureRecorder*| works, and updated accordingly.

+                                     double width,
+                                     double height) {
+        ASSERT(recorder);
+        PassRefPtr<Canvas> canvas = create(
+            recorder->beginRecording(width, height));
+        recorder->set_canvas(canvas.get());

[eseidel, 2015/06/23 00:41:32]

Who keeps the PictureRecorder alive? , Typically accepting a PassRefPtr<> means
that you're taking ownership of the object, or at least you're comfortable being
the last ref of said object. :)  It looks like this code may depend on someone
else holding onto the PictureRecorder?

[abarth-chromium, 2015/06/23 02:28:44]

Canvas effectively has a weak reference to the picture recorder.  We don't
actually use a base::WeakPtr because that would add overhead to every draw call.
 Instead, the PictureRecorder notifies the Canvas when it dies, which makes the
Canvas object ignore all subsequent draw calls.

+        return canvas;
+    }
+
     ~Canvas() override;
 
-    // Width/Height define a culling rect which Skia may use for optimizing
-    // out draw calls issued outside the rect.
-    float width() const { return m_size.width(); }
-    float height() const { return m_size.height(); }
-
     void save();
     void saveLayer(const Rect& bounds, const Paint* paint = nullptr);
     void restore();
 
     void translate(float dx, float dy);
     void scale(float sx, float sy);
     void rotate(float radians);
     void skew(float sx, float sy);
     void concat(const Float32List& matrix4);
 
     void clipRect(const Rect& rect);
     void clipRRect(const RRect* rrect);
     void clipPath(const CanvasPath* path);
 
     void drawLine(float x0, float y0, float x1, float y1, const Paint* paint);
     void drawPicture(Picture* picture);
     void drawPaint(const Paint* paint);
     void drawRect(const Rect& rect, const Paint* paint);
     void drawRRect(const RRect* rrect, const Paint* paint);
     void drawOval(const Rect& rect, const Paint* paint);
     void drawCircle(float x, float y, float radius, const Paint* paint);
     void drawPath(const CanvasPath* path, const Paint* paint);
     void drawImage(const CanvasImage* image,
                    float x,
                    float y,
                    const Paint* paint);
 
     SkCanvas* skCanvas() { return m_canvas; }
+    void clearSkCanvas() { m_canvas = nullptr; }
+    bool isRecording() const { return m_canvas != nullptr; }

[eseidel, 2015/06/23 00:41:32]

!!m_canvas works too.  This is fine though. :)

[iansf, 2015/06/23 22:46:09]

Acknowledged.

 
 protected:
-    PassRefPtr<DisplayList> finishRecording();
-
-    bool isRecording() const { return m_canvas; }
+    Canvas(SkCanvas* skCanvas);

[abarth-chromium, 2015/06/23 02:28:44]

Please mark one-argument constructors |explicit|.

[iansf, 2015/06/23 22:46:09]

Done.

 
 private:
-    FloatSize m_size;
-    RefPtr<DisplayList> m_displayList;
+    // The SkCanvas is supplied by a call to SkPictureRecorder::beginRecording,
+    // which does not transfer ownership.  For this reason, we hold a raw
+    // pointer and manually set the SkCanvas to null in clearSkCanvas.

[abarth-chromium, 2015/06/23 02:28:44]

@eseidel: Here's the comment that's intended to answer your question.

     SkCanvas* m_canvas;
 };
 
 } // namespace blink
 
 #endif  // SKY_ENGINE_CORE_PAINTING_CANVAS_H_