I'm going to tag @chinmaygarde as it looks like he originally authored this code, but I have to say that the original looks correct and the fix looks like it will have the wrong behavior.

The mutators stack was constructed from the top of the tree with transforms pushed as the tree was descended. Thus, child transforms appear later in the vector compared to parent transforms. In that ordering, the device pixel ratio and root transform must be before the root of the entire tree. The way the code was originally written looks like this is happening as intended, so you get:

root transform -> top-most transform layer transform -> some child transform -> some grandchild transform -> etc.

With the proposed fix, it looks like the ordering will be:

top-most transform layer transform -> some child transform -> some grandchild transform -> etc. -> root transform. I don't think that is right, but perhaps I am reading the history of how the mutators stack was constructed.

Thanks for adding Chinmay, I couldn't add him myself.

That's actually the issue. Based on the documentation of MutatorsStack[1], given a stack of transforms {T1, T2, T3} where T1 is the top and T3 is the bottom they need to be applied in T1(T2(T3(P))) order to the platform view layer (P). Now, if we add the root transform (RT) that would be applied after T1, not before T3, so RT(T1(T2(T3(P)))).

Now, the for-loop in line 129 goes from bottom to top, so |mutations_array| will look like {T3, T2, T1} without RT. If the root transform is pushed at the top, |mutations_array| will look like {RT, T3, T2, T1} which is incorrect.

I'll concede that I may have misunderstood the embedder.h[2] documentation and the reversal at line 168 isn't necessary, though it would still clash with the documentation: "... first mutation in the list will be a transformation mutation to make sure subsequent mutations are in the correct coordinate space". That essentially tells me that RT is at the top, so the list needs to be reversed. This also makes sense to me since the embedder gets the transformed platform view and would be applying the inverse transforms to get back to the platform view (P).

[1] https://github.com/flutter/engine/blob/master/flow/embedded_views.h#L123
[2] https://github.com/flutter/engine/blob/master/shell/platform/embedder/embedder.h#L731

I think the problem is that Bottom() is implemented to point to the most recently pushed item, but typical stack terminology is that Bottom() is the first item to have been pushed and the last item that would be popped. You are correct that the root transform should be closest to the first item pushed and it looks like that is Top() based on the implementation of the MutatorsStack, but that seems backwards to me and why I thought that the original code was correct.

Should the implementations of Bottom() and Top() be reversed?

Oh, interesting. Let me test. I'll likely want to fix this now rather than wait.

Sounds good. You might have to update an existing test expectation for this.

I'll leave this as a follow-up for Chinmay.

Sounds good.

LUCI presubmits are infra failures and build_and_test_linux_unopt_debug covers the changes here. Landing to unblock b/143612326.