Simplify ViewResolver type

Created by: lguychard

A ViewResolver is now simply a function that takes a container as input and returns Iterable<View>, instead of being defined with a selector and a resolveView() function.

Created by: lguychard

When we wrote trackViews(), we opted not to run the resolvers on removed nodes, to guard against cases where resolution depends on state outside the code view, and the needed state would be inaccessible because the node has been removed from the tree. It wasn't a bug we actually encountered, merely something that was raised during code review, and then accounted for, but it is definitely possible. The result would have been leaked code views. Reference: !2909 (merged)

The issue you raised in !3920 (merged) is a different bug, that leads to systematically leaked code views: if the resolver returns a different element than the selector, the code view will be leaked when removed from the DOM, because the resolver is not run on removed nodes.

This PR does reintroduce the first bug, in order to fix the second.

I do think that looking at the context of an element initially matched with a selector in order to return the correct code view element is a valid use case. On github.com, for instance, we can't come up with an unambiguous enough selector to match the whole code view on blob views (example), and the issue was the same for snippets.

One way to keep this possible while avoiding both bugs would be to simply check whether removed nodes are tracked views, or contain tracked views within their subtree (without running selectors or resolvers on removed nodes).

Created by: felixfbecker

That would be less performant though because we would have to iterate over all registered code views instead of doing a O(1) Map lookup.

Anything that speaks against my suggestion?

Created by: lguychard

Your suggestion is still potentially vulnerable to !2909 (merged) if findElements() does something akin to this code, isn't it?

Created by: felixfbecker

Hmm yeah I think you're right. Sounds like we need to iterate.

Created by: felixfbecker

FYI I think that refactor would conflict with native diff views

Created by: felixfbecker

Do you want to get this in for 3.5?

Created by: lguychard

If time, but it's really not the most critical thing now that #4371 is merged. I'll move this to the backlog milestone to reflect this, and rebase when I get the chance.

Created by: slimsag

This PR has been open for a very long time with no progress, closing. Reopen if desired.