RendererContextManager #17941
RendererContextManager #17941
Conversation
| RendererContextSwitch(RendererContextManager& manager, | ||
| std::shared_ptr<RendererContext> context) | ||
| : manager_(manager) { | ||
| bool result = manager_.PushContext(context); |
There was a problem hiding this comment.
Nit: Here and elsewhere, OOL these code blocks.
| ~RendererContextManager() = default; | ||
|
|
||
| //---------------------------------------------------------------------------- | ||
| /// @brief Make the flutter's context as current context. |
There was a problem hiding this comment.
the flutter's context is a bit ambiguous. Maybe, "the context Flutter uses on the raster thread for onscreen rendering.". Similarly, for the resource context, "the context Flutter uses for performing texture upload on the IO thread".
| set_current_context_block_(stored_.back()); | ||
| stored_.pop_back(); | ||
| } | ||
| }; |
|
|
||
| namespace flutter { | ||
|
|
||
| template <typename RendererContext> |
There was a problem hiding this comment.
It is unclear why this has to be templated. The render context can be an abstract class instead that the implementation can subclass. This will also allow you to OOL these methods.
There was a problem hiding this comment.
I was wondering how to test this, as the implementation in objc (with a EAGLContext) wasn't testable ASAIK. So I made this change so at least we can test the flow with some other type.
There was a problem hiding this comment.
If RenderContext was an abstract base class instead, that would solve your testability problem
There was a problem hiding this comment.
discussed offline:
excessive use of templating quickly leads to code bloat and binary size increases. If the only use case is testability, the abstract base class makes more sense.
I'm going to update this PR to make each platform override this.
| private: | ||
| RendererContextManager& manager_; | ||
| bool result_; | ||
| bool has_pushed_context_; |
There was a problem hiding this comment.
When will this ever be false? This seems unnecessary as it stands.
| /// | ||
| /// @return A `RendererContextSwitch` which ensures the current context is | ||
| /// the `context` that used in the constructor. | ||
| std::unique_ptr<RendererContextSwitch> MakeCurrent() { |
There was a problem hiding this comment.
Aren't these switches tied to scoped? In that case, it isn't clear why these are unique pointers and not just lightweight value objects.
| } | ||
|
|
||
| // Returns true if the context switching was successful. | ||
| bool GetResult() { return result_; } |
There was a problem hiding this comment.
Nit: Doxygen style comments please. Also, maybe "DidSwitch"? or something more indicative of what the result might be?
| std::shared_ptr<RendererContext> GetContext() { return context_; } | ||
|
|
||
| private: | ||
| std::shared_ptr<RendererContext> context_; |
There was a problem hiding this comment.
Sorry. Did not submit this one last comment in the previous review. Using a shared pointer here puts the added burden of making sure the render context can be destroyed on any thread. In reality, render context cannot are tied to specific threads.
|
@chinmaygarde Updated based on your reviews. Removed generic implementations. Will implement the actual logic in a following up PR. Edit: I missed some of your comments. Going to rework on this. Will ping you once ready. |
cyanglaz
force-pushed
the
gl_context_manager
branch
from
a8ca4a4 to
33cc616
Compare
|
@chinmaygarde Updated with all your review messages. PTAL! |
cyanglaz
force-pushed
the
gl_context_manager
branch
from
d405309 to
951635f
Compare
shell/common/test_renderer_context.h
Outdated
| /// Represents the current alive context. | ||
| /// | ||
| /// -1 represents no context has been set. | ||
| static int currentContext; |
There was a problem hiding this comment.
This reads like an ivar but is a static. Also, see the naming convention rules https://google.github.io/styleguide/cppguide.html#Variable_Names.
Also, you don't need to use a static variable for this. We use GTEST fixtures for this purpose. Here is how: You can make TestRendererContext inherit from ::testing::Test. This will allow you to use a TEST_F to write a test. Now each test gets its own context and is now isolated from sharing state with other tests and is more self contained.
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
This is awesome. Thanks!
3 participants
This is a part of #13946
This PR only contains setting up the RendererContextManager and RendererContextSwitch.
This PR doesn't include any usage of these 2 classes
A difference from #13946 is that I moved the switching logic from IOSRendererContextManager to RendererContextManager so it is easily unittestable.
This change also made implementing RendererContextManager easier. An embedder doesn't have to override/implement the RendererContextManager anymore. Instead, the embedder can construct a RendererContextManager, while providing Context types and code blocks to set and get current context.