Move Shell::Add/RemoveView to PlatformView and refine embedder API doc
#52003
Conversation
|
@chinmaygarde Can you take a look? I'm not sure whether this is a necessary change. I'm proposing this only because I supposed the |
| /// | ||
| class Delegate { | ||
| public: | ||
| using AddViewCallback = PlatformView::AddViewCallback; |
There was a problem hiding this comment.
Nit: Is this necessary? You can just use the typedef from the parent.
There was a problem hiding this comment.
The compiler requires PlatformView::AddViewCallback (for both the methods in Delegate and Shell) instead of simply AddViewCallback without it, which is definitely not necessary though, and I'm down to remove it. What do you think?
| /// | ||
| class PlatformView { | ||
| public: | ||
| using AddViewCallback = std::function<void(bool added)>; |
There was a problem hiding this comment.
What do the added and removed arguments to the callback mean? If added is false, why invoke the AddViewCallback? Perhaps document that?
There was a problem hiding this comment.
Both the add and the remove callbacks rely on the result. For example, in the Windows embedder as implemented in #51923
- After calling
AddView, the embedder blocks until a result is reported, which decides whether the returnedFlutterWindowsViewis null. (This eager blocking can be optimized in the future, but this is an easier approach for now.) - The embedder also pre-allocate resources after calling
AddViewbefore the callback, and if the result turns out a failure, then the resources are deallocated. The pre-allocation ensures that the framework can start operating the view as soon as they're aware of it. - After calling
RemoveView, embedder does not deallocate resources until the callback (successful or not). This allows handling any pending operations during this time.
|
I kept the statement "The embedder should prepare resources in advance but be ready to clean up on failure." since I think this is a very useful implementation suggestion. |
Shell::Add/RemoveView to PlatformViewShell::Add/RemoveView to PlatformView and refine embedder API doc
loic-sharma
left a comment
There was a problem hiding this comment.
LGTM, thanks for cleaning this up!
This PR moves the methods to add or remove views from
ShelltoPlatformView. By design, theShellis supposed to be a messenger that glues classes together, whilePlatformViewis the operator that embedders that do not use the embedder API should operate on. The current design was made due to lack of knowledge to this design.This also makes
PlatformViewaware of views, which might be a prerequisite to #51925.This PR also adds some details to embedder API
AddViewandRemoveView.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.