From 0cc552e0eb416651c1d1cbf69dcccd32e0f4ac45 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 13 May 2021 12:17:39 -0700 Subject: [PATCH] Revert "SceneBuilder.addPicture returns the layer (#26074)" This reverts commit 8e72d7bf4fde4cf42082328b397d111ba48567d3. --- lib/ui/compositing.dart | 28 +++---------------- lib/ui/compositing/scene_builder.cc | 14 +++------- lib/ui/compositing/scene_builder.h | 7 +---- lib/ui/painting/engine_layer.cc | 5 ++-- lib/ui/painting/engine_layer.h | 14 ++++------ .../lib/src/engine/canvaskit/layer.dart | 2 +- .../engine/canvaskit/layer_scene_builder.dart | 13 ++------- lib/web_ui/lib/src/engine/html/picture.dart | 2 +- .../lib/src/engine/html/scene_builder.dart | 13 ++------- lib/web_ui/lib/src/ui/compositing.dart | 7 +---- testing/dart/compositing_test.dart | 21 +++----------- 11 files changed, 30 insertions(+), 96 deletions(-) diff --git a/lib/ui/compositing.dart b/lib/ui/compositing.dart index e8b4346064571..5ad4693597d5d 100644 --- 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 index 3646b57c9578c..4acf0a3a1515c 100644 --- 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 oldLayer) { - auto layer = std::make_shared( + int hints) { + auto layer = std::make_unique( 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 index 32d7d040ddbf9..70d37f6123559 100644 --- a/lib/ui/compositing/scene_builder.h +++ b/lib/ui/compositing/scene_builder.h @@ -101,12 +101,7 @@ class SceneBuilder : public RefCountedDartWrappable { double top, double bottom); - void addPicture(Dart_Handle layer_handle, - double dx, - double dy, - Picture* picture, - int hints, - fml::RefPtr 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 index 4ce8de877fe0f..4504ac278e618 100644 --- 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 layer) +EngineLayer::EngineLayer(std::shared_ptr 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 index 1638751f4abcf..58bfdecbfd00c 100644 --- a/lib/ui/painting/engine_layer.h +++ b/lib/ui/painting/engine_layer.h @@ -24,21 +24,19 @@ class EngineLayer : public RefCountedDartWrappable { size_t GetAllocationSize() const override; - static fml::RefPtr MakeRetained( - Dart_Handle dart_handle, - std::shared_ptr layer) { - auto engine_layer = fml::MakeRefCounted(std::move(layer)); + static void MakeRetained(Dart_Handle dart_handle, + std::shared_ptr layer) { + auto engine_layer = fml::MakeRefCounted(layer); engine_layer->AssociateWithDartWrapper(dart_handle); - return engine_layer; } static void RegisterNatives(tonic::DartLibraryNatives* natives); - std::shared_ptr Layer() const { return layer_; } + std::shared_ptr Layer() const { return layer_; } private: - explicit EngineLayer(std::shared_ptr layer); - std::shared_ptr layer_; + explicit EngineLayer(std::shared_ptr layer); + std::shared_ptr 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 index 970b3d1252a40..89532d31a4b39 100644 --- 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 index 933424b53fc2d..1a1c950b58b94 100644 --- 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 index e29915d453c04..6562dfa5c92f1 100644 --- 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 ``, `
` and `

` 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 index 312ce6d45d310..bb6801fa628b8 100644 --- 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 index ab5c09710d0d3..a177a15f41f84 100644 --- 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 index dde5adcd76378..08aa9b84f0822 100644 --- 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); }); }