Make CkVertices a ManagedSkiaObject (flutter#20421)

Author: yjbanov

lib/web_ui/lib/src/engine/compositor/canvas.dart

@@ -5,7 +5,10 @@
 // @dart = 2.10
 part of engine;
 
-/// A Dart wrapper around Skia's SKCanvas.
+/// A Dart wrapper around Skia's [SkCanvas].
+///
+/// This is intentionally not memory-managing the underlying [SkCanvas]. See
+/// the docs on [SkCanvas], which explain the reason.
 class CkCanvas {
   final SkCanvas skCanvas;
 
@@ -203,7 +206,7 @@ class CkCanvas {
       ui.Vertices vertices, ui.BlendMode blendMode, CkPaint paint) {
     CkVertices skVertices = vertices as CkVertices;
     skCanvas.drawVertices(
-      skVertices.skVertices,
+      skVertices.skiaObject,
       toSkBlendMode(blendMode),
       paint.skiaObject,
     );

lib/web_ui/lib/src/engine/compositor/canvaskit_api.dart

@@ -1246,6 +1246,11 @@ class SkPictureRecorder {
   external void delete();
 }
 
+/// We do not use the `delete` method (which may be removed in the future anyway).
+///
+/// By Skia coding convention raw pointers should always be treated as
+/// "borrowed", i.e. their memory is managed by other objects. In the case of
+/// [SkCanvas] it is managed by [SkPictureRecorder].
 @JS()
 class SkCanvas {
   external void clear(Float32List color);
@@ -1547,7 +1552,9 @@ class SkTextRange {
 }
 
 @JS()
-class SkVertices {}
+class SkVertices {
+  external void delete();
+}
 
 @JS()
 @anonymous

lib/web_ui/lib/src/engine/compositor/vertices.dart

@@ -5,17 +5,16 @@
 // @dart = 2.10
 part of engine;
 
-class CkVertices implements ui.Vertices {
-  late SkVertices skVertices;
-
-  CkVertices(
+class CkVertices extends ManagedSkiaObject<SkVertices> implements ui.Vertices {
+  factory CkVertices(
     ui.VertexMode mode,
     List<ui.Offset> positions, {
     List<ui.Offset>? textureCoordinates,
     List<ui.Color>? colors,
     List<int>? indices,
-  })  : assert(mode != null), // ignore: unnecessary_null_comparison
-        assert(positions != null) { // ignore: unnecessary_null_comparison
+  }) {
+    assert(mode != null); // ignore: unnecessary_null_comparison
+    assert(positions != null); // ignore: unnecessary_null_comparison
     if (textureCoordinates != null &&
         textureCoordinates.length != positions.length)
       throw ArgumentError(
@@ -27,7 +26,7 @@ class CkVertices implements ui.Vertices {
       throw ArgumentError(
           '"indices" values must be valid indices in the positions list.');
 
-    skVertices = canvasKit.MakeSkVertices(
+    return CkVertices._(
       toSkVertexMode(mode),
       toSkPoints2d(positions),
       textureCoordinates != null ? toSkPoints2d(textureCoordinates) : null,
@@ -36,14 +35,15 @@ class CkVertices implements ui.Vertices {
     );
   }
 
-  CkVertices.raw(
+  factory CkVertices.raw(
     ui.VertexMode mode,
     Float32List positions, {
     Float32List? textureCoordinates,
     Int32List? colors,
     Uint16List? indices,
-  })  : assert(mode != null), // ignore: unnecessary_null_comparison
-        assert(positions != null) { // ignore: unnecessary_null_comparison
+  }) {
+    assert(mode != null); // ignore: unnecessary_null_comparison
+    assert(positions != null); // ignore: unnecessary_null_comparison
     if (textureCoordinates != null &&
         textureCoordinates.length != positions.length)
       throw ArgumentError(
@@ -55,12 +55,47 @@ class CkVertices implements ui.Vertices {
       throw ArgumentError(
           '"indices" values must be valid indices in the positions list.');
 
-    skVertices = canvasKit.MakeSkVertices(
+    return CkVertices._(
       toSkVertexMode(mode),
       rawPointsToSkPoints2d(positions),
       textureCoordinates != null ? rawPointsToSkPoints2d(textureCoordinates) : null,
       colors != null ? encodeRawColorList(colors) : null,
       indices,
     );
   }
+
+  CkVertices._(
+    this._mode,
+    this._positions,
+    this._textureCoordinates,
+    this._colors,
+    this._indices,
+  );
+
+  final SkVertexMode _mode;
+  final List<Float32List> _positions;
+  final List<Float32List>? _textureCoordinates;
+  final List<Float32List>? _colors;
+  final Uint16List? _indices;
+
+  @override
+  SkVertices createDefault() {
+    return canvasKit.MakeSkVertices(
+      _mode,
+      _positions,
+      _textureCoordinates,
+      _colors,
+      _indices,
+    );
+  }
+
+  @override
+  SkVertices resurrect() {
+    return createDefault();
+  }
+
+  @override
+  void delete() {
+    rawSkiaObject?.delete();
+  }
 }

lib/web_ui/test/canvaskit/vertices_test.dart

@@ -0,0 +1,57 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// @dart = 2.6
+import 'package:test/test.dart';
+import 'package:ui/src/engine.dart';
+import 'package:ui/ui.dart' as ui;
+
+import 'common.dart';
+
+void main() {
+  group('Vertices', () {
+    setUpAll(() async {
+      await ui.webOnlyInitializePlatform();
+    });
+
+    test('can be constructed, drawn, and deleted', () {
+      final CkVertices vertices = _testVertices();
+      expect(vertices, isA<CkVertices>());
+      expect(vertices.createDefault(), isNotNull);
+      expect(vertices.resurrect(), isNotNull);
+
+      final recorder = CkPictureRecorder();
+      final canvas = recorder.beginRecording(const ui.Rect.fromLTRB(0, 0, 100, 100));
+      canvas.drawVertices(
+        vertices,
+        ui.BlendMode.srcOver,
+        ui.Paint(),
+      );
+      vertices.delete();
+    });
+  // TODO: https://github.com/flutter/flutter/issues/60040
+  }, skip: isIosSafari);
+}
+
+ui.Vertices _testVertices() {
+  return ui.Vertices(
+    ui.VertexMode.triangles,
+    <ui.Offset>[
+      ui.Offset(0, 0),
+      ui.Offset(10, 10),
+      ui.Offset(0, 20),
+    ],
+    textureCoordinates: <ui.Offset>[
+      ui.Offset(0, 0),
+      ui.Offset(10, 10),
+      ui.Offset(0, 20),
+    ],
+    colors: <ui.Color>[
+      ui.Color.fromRGBO(255, 0, 0, 1.0),
+      ui.Color.fromRGBO(0, 255, 0, 1.0),
+      ui.Color.fromRGBO(0, 0, 255, 1.0),
+    ],
+    indices: <int>[0, 1, 2],
+  );
+}