This is a breaking change. Some inherited classes may need to update their signatures.

I reverted the change. I only changed it because it didn't feel right. It puts the responsibility of setting the _engineLayer field on the caller of addToScene. This means that the caller always has to do this:

childLayer._engineLayer = childLayer.addToScene(builder);

There are two issues with this. One is that _engineLayer is private and so a custom layer implementation won't be able to do this correctly. The second is that the caller needs to remember to do this. They also need to make sure they do this correctly. I found a bug in _ProxyLayer that returned another layer's engine layer, which is wrong. It only manifested after my change because my change exercises the new assertions I added in the SceneBuilder API.

However, there's no rush to fix this. We can do this as a follow-up PR.

I remembered that we've had this conversation before. Maybe let's create a Github issue to track our discussions and decisions so we won't forget it again in a few months :-)

Update the document if the return type is changed to void.

Reverted the change.

Do we currently have use cases for engineLayer in Leader/FollowerLayers? I remember that I explicitly set engine layer to null because Leader/FollowerLayers break assumptions of the retained rendering. Hence I don't want someone to accidentally use retained rendering for them.

Yes, when a leader/follower layer is moving without changing its children, we want to reuse the engine layer DOM nodes associated with pushTransform.

I see. Maybe add a comment somewhere like "The engine layer's resources may be reused through oldLayer, but do not reuse the engine layer's rasterized pixels through addRetained directly because there's no guarantee that the pixels would stay the same even if the whole subtree is unchanged."

There's plenty of documentation on the oldLayer parameter about what it does. It might be overkill to document it again for every invocation of push* method, no?

This is specific to the Leader/FolllowerLayer. Previously, it's impossible to re-use the engineLayer of a Leader/FollowerLayer for addRetained because we return null.

Now we return the engineLayer for oldLayer, but we still don't want developers to use it for addRetained. I don't think that we need to document every push* method. Just documenting Leader/FollowerLayer.addToScene would be enough. Maybe something like "Only use the returned engineLayer for oldLayer in .... Do not use it for addRetained".

Shall we also invalidate the layer's engineLayer at this point?

I think that's layer-specific. The old engineLayer may still be needed to be passed as oldLayer during compositeFrame.

I see. So engineLayer could either be:

1. in a state that's synchronized with the layer so we can directly use it in addRetained,
2. in a stale state where one can only use it as oldLayer.

Is there some mechanism that we can enforce the developer to not misuse such two states of the engineLayer?

Yes, I added a bunch of assertions in the SceneBuilder in the engine code.

LeaderLayer shouldn't even have an engine layer, should it?

@yjbanov ^

LeaderLayer sometimes creates a TransformEngineLayer:

Oh, right.

This may be a breaking change as we're changing the return type. Maybe we can just assert(_layer is OffsetLayer) and return an OffsetLayer as this is just for backward compatibility.

Hmm. Interesting idea. Since I'm deprecating it, might as well keep the same type. I'll give it a shot.

Done.

this should be protected. Nobody outside this object should be reading it as far as I can tell.

There are 41 unprotected usages. Most are in tests (which can be fixed by making it @protected @visibleForTesting). However, there are 18 usages in package:flutter_test, widget_inspector.dart, and rendering/binding.dart, that need this to remain public.

How many of these are for debug purposes (for which we could use a debugLayer) vs production?

I was able to convert all but 5 to debugLayer. Here's the production code that uses layer:

rendering/binding.dart:

multiple places in widget_inspector.dart (although I'm not sure if widget inspector is considered debug code):

Looking the surrounding code in widget_inspector.dart it's all debug code, so I was able to convert it to debugLayer as well. So, the only remaining production usage of layer is the MouseTracker in rendering/binding.dart.

Let's move that closure into a method on RenderView, and then pass that method in rather than using a closure. That'll be generally cleaner and will allow us to make this protected.

why do we want to remove this?

Because any render object can create and keep a layer the normal layer getter no longer requires isRepaintBoundary and !_needsPaint. So this getter no longer adds any value to tests. They can as successfully use the layer getter (in fact, I moved all our existing tests to it).

It seems to me that this should still require needsPaint. All the more so now than before. Indeed the setter probably should have asserts too: we presumably should never change our layer except during paint.

In the past it required !_needsPaint:

With this change it no longer requires it, because, as you say we're producing layers during paint. Are you suggesting that we should flip the requirement to require _needsPaint?

Actually, we're clearing _needsPaint before calling paint, so requiring _needsPaint in the layer getter won't work:

I meant that layer should require !needsPaint, sorry for the confusion above.

If we make layer protected I guess I'm not as worried about this. Though it sounds like we still want the debugLayer for those places outside of RenderObject that want to read the layer during development.

here and in other places, might it be better to just create the layer using oldLayer ??= PhysicalModelLayer() and then set the attributes unconditionally? Do the constructors do anything more efficient than that?

I wasn't worried about efficiency as much as I was about API compatibility. Most layer constructors either have @required parameters, or they assume non-null arguments. I can change the constructors to allow blank objects. WDYT?

We can give them default values, or we can allow null and document they have to be non-null before adding to scene and add the relevant asserts (we have some cases like that already). I think the ergonomics will be nicer if we remove the @requireds and just have the one way of doing this, though. Having both ways is going to result in bugs when we accidentally only fix one code path.

Can we have a null layer here?

Done

Same here. It doesn't seem to me that layer could be null at this point.

Done.