Revert "Fixed constness of display list storage." #52724
Conversation
This reverts commit 1ccd0c3.
| const DisplayListStorage& GetStorage() const { return storage_; } | ||
|
|
There was a problem hiding this comment.
I think the crux of your argument is around exposing this immutable data ( #52705 (comment) ). That decision is tied to #52715 since it accesses it.
Everything else in this PR is correct since it statically asserts correct constness, so you can make the revert just the removal of this method.
There was a problem hiding this comment.
There is nothing wrong with the current implementation. Adding const in a few places is probably fine, but not needed since the storage is not exposed outside of the internal implementation.
Two questions I have is whether or not destructors are required to work on const objects (I supposed that they are or the PR would not have landed) and whether or not the internal const-ness (which isn't strictly necessary) would imply that the data in the buffer never changes (but it actually does - there are mutable fields in the data that provided required functions).
There was a problem hiding this comment.
But, yes, the rest of it doesn't hurt anything and might be considered cleaner other than the fact that the data stream actually contains at least one (and potentially more as we migrate the text stuff to be Skia/Impeller-agnostic) mutable chunk of data.
There was a problem hiding this comment.
If we need mutable storage in DisplayList the correct solution would to make DisplayList::storage_ not const. Previously it was declared const but had the ability to mutate it because there was a const method that returned a non-const pointer.
There was a problem hiding this comment.
It won't matter as the mutation happens on a const object contained within the DL that is passed to a Dispatch receiver - and it's the receiver that does the mutation.
My point was less "how are we going to do this" than "we do this and making this pointer const won't stop it, so are we sending the right message by making it const", but I think it is a minor point that doesn't matter...
There was a problem hiding this comment.
The fact that the field is marked "mutable" already acknowledges that it is allowing mutation inside what is otherwise a const set of data, so I think the point is moot.
There was a problem hiding this comment.
I talked to jim friday. He was looking at the patch backwards for the modifications to DisplayListStorage::get (#52705 (comment)). So I think we are on the same page about that being correct. As for the visibility of DisplayList::GetStorage, since it is now immutable I don't have a problem with it, but if you want to make it private again that's fine with me. It's just a minor annoyance for what I'm trying to do with the display list capture tool. I'm fine with approving that change.
|
I think the public exposure of the storage from the DisplayList object should go away. I think it serves a near-term purpose for some investigations that @gaaclarke is doing, but we should eventually replace it with something more supportable. The rest of the changes other than that one line are fine and I don't think a revert is really necessary for us to settle on a future more maintainable exposure of the relevant information... |
Reverts #52705
The PR exposed internal storage information which is neither useful nor supported.
View comments on the PR for more specific reasons.