[ASDisplayNode] -didEnterPreloadState does not need to call -layoutIfNeeded #trivial

[appleguy]

This was originally added for ASCollectionNode and ASTableNode preloading to work
as intended when nested inside of another ASRangeController-powered node. Indeed,
it is still necessary to trigger layout on these UIKit-backed components in order
for their own ASRangeControllers to start preparing content within them.

However, unlike the comment suggests, it is not necessary to do this for image
nodes. ASNetworkImageNode has only one .URL, and does not use the .bounds or
.calculatedLayout at all during loading. Even the ASMultiplexImageNode does not
use the .bounds, and the ASMapNode uses .calculatedLayout instead of .bounds.

This change has important performance benefits. In particular, it avoids
layouts that would occur on the main thread, often including text sizing,
and also can result in redundant layout passes (even during a layout pass that
triggers a node to enter the preload range, it may force its own layout early).

It would be great to test this change with Pinterest to confirm its safety, but
based on a full audit of the framework codebase, the only possibility that I
see for a regression is if app implementations of -didEnterPreloadState make
direct use of .bounds instead of .calculatedLayout (which is easy to fix).

[appleguy]

Note that a layout pass will be forced when reaching the Display range anyway, via recursivelyEnsureDisplaySynchronously:NO which is called on the ASCellNode at that time. If the layout hasn't changed by the time it is Visible, then no additional work should be performed.

[maicki]

@appleguy Just had a small question while browsing over the change. But if this question turns out to be not really relevant, it's really awesome you found unnecessary work we can safely remove.

[maicki]

If we remove layoutIfNeeded in this case would user with custom ASDisplayNode subclass that relied on the fact that the size was available via bounds / calculatedSize suddenly get an undefined behavior?

They would have to basically do the same as ASTableNode and ASCollectionNode and explicitly allocating the view and trigger a layout if needed if they would like to have the old behavior?

[appleguy]

@maicki Since this sets it in a downward propagation, it wouldn't necessarily have set the bounds on self, unless the parent had done so first.

Assuming the parent had done so, then yes this could be a behavior change. However, using .calculatedSize actually will not change in behavior.

I think it is quite rare to override this method, and even more rare to use .bounds in it. One potential option could be to throw an assertion if a client DOES access .bounds during this call, and to tell them "this probably won't do what you want, use .calculatedLayout.size instead".

[nguyenhuy]

Good point on calculatedSize being available and should be used by subclasses.

Approving because this behavior was introduced just 3 months ago (facebookarchive/AsyncDisplayKit#3263). So we're talking about a 3-month window in which developers has been allowed to use the bounds in - (void)didEnterPreloadState, which I reckon very few (if any) did. I did a quick check on Pinterest code base and we don't do it.

[nguyenhuy]

Would love to have input from @Adlai-Holler before merging.

[Adlai-Holler]

I think this change is acceptable. Clients who depend on self.bounds to be up-to-date can either trigger the early layout themselves or rely on the size of the calculated layout.