Revert "SceneBuilder.addPicture returns the layer (#26074)" (#26133)

dnfield · web-flow · commit 7e34f1ff00a1 · 2021-05-13T15:44:02.000-07:00
diff --git a/lib/ui/compositing.dart b/lib/ui/compositing.dart
@@ -188,15 +188,6 @@ class PhysicalShapeEngineLayer extends _EngineLayerWrapper {
   PhysicalShapeEngineLayer._(EngineLayer nativeLayer) : super._(nativeLayer);
 }
 
-/// An opaque handle to a picture engine layer.
-///
-/// Instances of this class are created by [SceneBuilder.addPicture].
-///
-/// {@macro dart.ui.sceneBuilder.oldLayerCompatibility}
-class PictureEngineLayer extends _EngineLayerWrapper {
-  PictureEngineLayer._(EngineLayer nativeLayer) : super._(nativeLayer);
-}
-
 /// Builds a [Scene] containing the given visuals.
 ///
 /// A [Scene] can then be rendered using [FlutterView.render].
@@ -708,29 +699,18 @@ class SceneBuilder extends NativeFieldWrapperClass2 {
   /// Adds a [Picture] to the scene.
   ///
   /// The picture is rasterized at the given offset.
-  ///
-  /// {@macro dart.ui.sceneBuilder.oldLayer}
-  PictureEngineLayer addPicture(
+  void addPicture(
     Offset offset,
     Picture picture, {
     bool isComplexHint = false,
     bool willChangeHint = false,
-    PictureEngineLayer? oldLayer,
   }) {
-    assert(_debugCheckCanBeUsedAsOldLayer(oldLayer, 'addPicture'));
-    final EngineLayer engineLayer = EngineLayer._();
     final int hints = (isComplexHint ? 1 : 0) | (willChangeHint ? 2 : 0);
-    _addPicture(engineLayer, offset.dx, offset.dy, picture, hints, oldLayer?._nativeLayer);
-    return PictureEngineLayer._(engineLayer);
+    _addPicture(offset.dx, offset.dy, picture, hints);
   }
 
-  void _addPicture(
-    EngineLayer outEngineLayer,
-    double dx,
-    double dy,
-    Picture picture,
-    int hints,
-    EngineLayer? oldLayer) native 'SceneBuilder_addPicture';
+  void _addPicture(double dx, double dy, Picture picture, int hints)
+      native 'SceneBuilder_addPicture';
 
   /// Adds a backend texture to the scene.
   ///
diff --git a/lib/ui/compositing/scene_builder.cc b/lib/ui/compositing/scene_builder.cc
@@ -272,20 +272,14 @@ void SceneBuilder::pop() {
   PopLayer();
 }
 
-void SceneBuilder::addPicture(Dart_Handle layer_handle,
-                              double dx,
+void SceneBuilder::addPicture(double dx,
                               double dy,
                               Picture* picture,
-                              int hints,
-                              fml::RefPtr<EngineLayer> oldLayer) {
-  auto layer = std::make_shared<flutter::PictureLayer>(
+                              int hints) {
+  auto layer = std::make_unique<flutter::PictureLayer>(
       SkPoint::Make(dx, dy), UIDartState::CreateGPUObject(picture->picture()),
       !!(hints & 1), !!(hints & 2));
-  AddLayer(layer);
-  EngineLayer::MakeRetained(layer_handle, layer);
-  if (oldLayer && oldLayer->Layer()) {
-    layer->AssignOldLayer(oldLayer->Layer().get());
-  }
+  AddLayer(std::move(layer));
 }
 
 void SceneBuilder::addTexture(double dx,
diff --git a/lib/ui/compositing/scene_builder.h b/lib/ui/compositing/scene_builder.h
@@ -101,12 +101,7 @@ class SceneBuilder : public RefCountedDartWrappable<SceneBuilder> {
                              double top,
                              double bottom);
 
-  void addPicture(Dart_Handle layer_handle,
-                  double dx,
-                  double dy,
-                  Picture* picture,
-                  int hints,
-                  fml::RefPtr<EngineLayer> oldLayer);
+  void addPicture(double dx, double dy, Picture* picture, int hints);
 
   void addTexture(double dx,
                   double dy,
diff --git a/lib/ui/painting/engine_layer.cc b/lib/ui/painting/engine_layer.cc
@@ -4,7 +4,6 @@
 
 #include "flutter/lib/ui/painting/engine_layer.h"
 
-#include "flutter/lib/ui/ui_dart_state.h"
 #include "third_party/tonic/converter/dart_converter.h"
 #include "third_party/tonic/dart_args.h"
 #include "third_party/tonic/dart_binding_macros.h"
@@ -14,14 +13,14 @@ using tonic::ToDart;
 
 namespace flutter {
 
-EngineLayer::EngineLayer(std::shared_ptr<flutter::Layer> layer)
+EngineLayer::EngineLayer(std::shared_ptr<flutter::ContainerLayer> layer)
     : layer_(layer) {}
 
 EngineLayer::~EngineLayer() = default;
 
 size_t EngineLayer::GetAllocationSize() const {
   // Provide an approximation of the total memory impact of this object to the
-  // Dart GC.  The Layer may hold references to a tree of other layers,
+  // Dart GC.  The ContainerLayer may hold references to a tree of other layers,
   // which in turn may contain Skia objects.
   return 3000;
 };
diff --git a/lib/ui/painting/engine_layer.h b/lib/ui/painting/engine_layer.h
@@ -24,21 +24,19 @@ class EngineLayer : public RefCountedDartWrappable<EngineLayer> {
 
   size_t GetAllocationSize() const override;
 
-  static fml::RefPtr<EngineLayer> MakeRetained(
-      Dart_Handle dart_handle,
-      std::shared_ptr<flutter::Layer> layer) {
-    auto engine_layer = fml::MakeRefCounted<EngineLayer>(std::move(layer));
+  static void MakeRetained(Dart_Handle dart_handle,
+                           std::shared_ptr<flutter::ContainerLayer> layer) {
+    auto engine_layer = fml::MakeRefCounted<EngineLayer>(layer);
     engine_layer->AssociateWithDartWrapper(dart_handle);
-    return engine_layer;
   }
 
   static void RegisterNatives(tonic::DartLibraryNatives* natives);
 
-  std::shared_ptr<flutter::Layer> Layer() const { return layer_; }
+  std::shared_ptr<flutter::ContainerLayer> Layer() const { return layer_; }
 
  private:
-  explicit EngineLayer(std::shared_ptr<flutter::Layer> layer);
-  std::shared_ptr<flutter::Layer> layer_;
+  explicit EngineLayer(std::shared_ptr<flutter::ContainerLayer> layer);
+  std::shared_ptr<flutter::ContainerLayer> layer_;
 
   FML_FRIEND_MAKE_REF_COUNTED(EngineLayer);
 };
diff --git a/lib/web_ui/lib/src/engine/canvaskit/layer.dart b/lib/web_ui/lib/src/engine/canvaskit/layer.dart
@@ -411,7 +411,7 @@ class ShaderMaskEngineLayer extends ContainerLayer implements ui.ShaderMaskEngin
 }
 
 /// A layer containing a [Picture].
-class PictureLayer extends Layer implements ui.PictureEngineLayer {
+class PictureLayer extends Layer {
   /// The picture to paint into the canvas.
   final CkPicture picture;
 
diff --git a/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder.dart b/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder.dart
@@ -45,21 +45,14 @@ class LayerSceneBuilder implements ui.SceneBuilder {
   }
 
   @override
-  ui.PictureEngineLayer addPicture(
+  void addPicture(
     ui.Offset offset,
     ui.Picture picture, {
     bool isComplexHint = false,
     bool willChangeHint = false,
-    ui.PictureEngineLayer? oldLayer,
   }) {
-    final PictureLayer layer = PictureLayer(
-      picture as CkPicture,
-      offset,
-      isComplexHint,
-      willChangeHint,
-    );
-    currentLayer.add(layer);
-    return layer;
+    currentLayer.add(PictureLayer(
+        picture as CkPicture, offset, isComplexHint, willChangeHint));
   }
 
   @override
diff --git a/lib/web_ui/lib/src/engine/html/picture.dart b/lib/web_ui/lib/src/engine/html/picture.dart
@@ -78,7 +78,7 @@ void _recycleCanvas(EngineCanvas? canvas) {
 
 /// A surface that uses a combination of `<canvas>`, `<div>` and `<p>` elements
 /// to draw shapes and text.
-class PersistedPicture extends PersistedLeafSurface implements ui.PictureEngineLayer {
+class PersistedPicture extends PersistedLeafSurface {
   PersistedPicture(this.dx, this.dy, this.picture, this.hints)
       : localPaintBounds = picture.recordingCanvas!.pictureBounds;
 
diff --git a/lib/web_ui/lib/src/engine/html/scene_builder.dart b/lib/web_ui/lib/src/engine/html/scene_builder.dart
@@ -359,12 +359,11 @@ class SurfaceSceneBuilder implements ui.SceneBuilder {
   ///
   /// The picture is rasterized at the given offset.
   @override
-  ui.PictureEngineLayer addPicture(
+  void addPicture(
     ui.Offset offset,
     ui.Picture picture, {
     bool isComplexHint = false,
     bool willChangeHint = false,
-    ui.PictureEngineLayer? oldLayer,
   }) {
     int hints = 0;
     if (isComplexHint) {
@@ -373,14 +372,8 @@ class SurfaceSceneBuilder implements ui.SceneBuilder {
     if (willChangeHint) {
       hints |= 2;
     }
-    final PersistedPicture layer = PersistedPicture(
-      offset.dx,
-      offset.dy,
-      picture as EnginePicture,
-      hints,
-    );
-    _addSurface(layer);
-    return layer;
+    _addSurface(PersistedPicture(
+        offset.dx, offset.dy, picture as EnginePicture, hints));
   }
 
   /// Adds a backend texture to the scene.
diff --git a/lib/web_ui/lib/src/ui/compositing.dart b/lib/web_ui/lib/src/ui/compositing.dart
@@ -31,10 +31,6 @@ abstract class ShaderMaskEngineLayer implements EngineLayer {}
 
 abstract class PhysicalShapeEngineLayer implements EngineLayer {}
 
-// TODO(yjbanov): Incorporate into DOM diffing algorithm.
-// https://github.com/flutter/flutter/issues/82287
-abstract class PictureEngineLayer implements EngineLayer {}
-
 abstract class SceneBuilder {
   factory SceneBuilder() {
     if (engine.useCanvasKit) {
@@ -103,12 +99,11 @@ abstract class SceneBuilder {
   void addRetained(EngineLayer retainedLayer);
   void pop();
   void addPerformanceOverlay(int enabledOptions, Rect bounds);
-  PictureEngineLayer addPicture(
+  void addPicture(
     Offset offset,
     Picture picture, {
     bool isComplexHint = false,
     bool willChangeHint = false,
-    PictureEngineLayer? oldLayer,
   });
   void addTexture(
     int textureId, {
diff --git a/testing/dart/compositing_test.dart b/testing/dart/compositing_test.dart
@@ -279,18 +279,16 @@ void main() {
     builder2.build();
   }
 
-  void testNoSharing(_TestNoSharingFunction pushFunction, {bool isLeafLayer = false}) {
+  void testNoSharing(_TestNoSharingFunction pushFunction) {
     testPushThenIllegalRetain(pushFunction);
     testAddRetainedThenIllegalPush(pushFunction);
     testDoubleAddRetained(pushFunction);
     testPushOldLayerTwice(pushFunction);
+    testPushChildLayerOfRetainedLayer(pushFunction);
+    testRetainParentLayerOfPushedChild(pushFunction);
     testRetainOldLayer(pushFunction);
     testPushOldLayer(pushFunction);
-    if (!isLeafLayer) {
-      testPushChildLayerOfRetainedLayer(pushFunction);
-      testRetainParentLayerOfPushedChild(pushFunction);
-      testRetainsParentOfOldLayer(pushFunction);
-    }
+    testRetainsParentOfOldLayer(pushFunction);
   }
 
   test('SceneBuilder does not share a layer between addRetained and push*', () {
@@ -379,17 +377,6 @@ void main() {
         oldLayer: oldLayer as ImageFilterEngineLayer,
       );
     });
-    testNoSharing((SceneBuilder builder, EngineLayer oldLayer) {
-      final PictureRecorder recorder = PictureRecorder();
-      final Canvas canvas = Canvas(recorder);
-      canvas.drawPaint(Paint());
-      final Picture picture = recorder.endRecording();
-      return builder.addPicture(
-        Offset.zero,
-        picture,
-        oldLayer: oldLayer as PictureEngineLayer,
-      );
-    }, isLeafLayer: true);
   });
 }
 

Original file line number	Diff line number	Diff line change
`@@ -411,7 +411,7 @@ class ShaderMaskEngineLayer extends ContainerLayer implements ui.ShaderMaskEngin`
`411`	`411`	`}`
`412`	`412`
`413`	`413`	`/// A layer containing a [Picture].`
`414`		`-class PictureLayer extends Layer implements ui.PictureEngineLayer {`
	`414`	`+class PictureLayer extends Layer {`
`415`	`415`	`/// The picture to paint into the canvas.`
`416`	`416`	`final CkPicture picture;`
`417`	`417`