[ui/compositor] Refactor ui::Layer to use subclasses [chromium/src : main]
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Commit-Queue | +1 |
2 comments
class COMPOSITOR_EXPORT LayerNotDrawn : public Layer {More type based functionality will be moved to these types.
Related details
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
static std::unique_ptr<Layer> Create(LayerType type);@oshima-san, let me know if you are okay with the idea. I will then proceed to fix the compilation and change a few more sematics.
Related details
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
static std::unique_ptr<Layer> Create(LayerType type);@oshima-san, let me know if you are okay with the idea. I will then proceed to fix the compilation and change a few more sematics.
- Zoraiz Naeem
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Commit-Queue | +1 |
2 comments
This is nessary, to keep the forward declar of Layer/LayerXXX classes declared. Otherwise, if we include ui/layer.h, it bloats the binary size by 2gb.
I can probably condense it by defining a macro.
Related details
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 3 comments![]( "Open in Gerrit")
just a generic comment about the type usage.
std::unique_ptr<ui::LayerSolidColor> viewport_background_layer_;If Layer interface is enough, just use Layer, and instantiate the specific class.
const ui::LayerSolidColor* GetViewportMagnifierLayerForTesting() const;do you need to expose the type for this API?
Related details
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
flackr@ PTAL. Let us know if you are okay making this change in ui::Layer API? I eventually plan to move most of the type specific logic into the subclasses.
I might also introduce a new layer type that handles embedding contents. (aka a standlone wrapper around cc::SurfaceLayer)
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
flackr@ PTAL. Let us know if you are okay making this change in ui::Layer API? I eventually plan to move most of the type specific logic into the subclasses.
I might also introduce a new layer type that handles embedding contents. (aka a standlone wrapper around cc::SurfaceLayer)
.
Robert Flack (Gerrit)
Robert Flack added 3 comments![]( "Open in Gerrit")
Zoraiz Naeemflackr@ PTAL. Let us know if you are okay making this change in ui::Layer API? I eventually plan to move most of the type specific logic into the subclasses.
I might also introduce a new layer type that handles embedding contents. (aka a standlone wrapper around cc::SurfaceLayer)
.
Yes, I think it's good for readability if we can reduce the size of the layer api and move the type-specific behaviors to subclasses.
// TODO(b:522627357): Add a templated version.Why does clone and mirror need to be templated? Aren't the specific types subclasses of Layer? I.e. isn't this just a plain virtual function that is implemented in each of the subclasses?
static std::unique_ptr<Layer> Create(LayerType type);Mitsuru Oshima@oshima-san, let me know if you are okay with the idea. I will then proceed to fix the compilation and change a few more sematics.
Yes
I'm not sure if I see the advantage of templating the create method over having callers call the specific create method for the layer type they want - e.g. `LayerTextured::Create()`. The latter would support having specific required parameters for some layer types if this made sense for some layer types.
Related details
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
SLSA-Policy-Verified
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 6 comments![]( "Open in Gerrit")
Zoraiz Naeemflackr@ PTAL. Let us know if you are okay making this change in ui::Layer API? I eventually plan to move most of the type specific logic into the subclasses.
I might also introduce a new layer type that handles embedding contents. (aka a standlone wrapper around cc::SurfaceLayer)
Robert Flack.
Yes, I think it's good for readability if we can reduce the size of the layer api and move the type-specific behaviors to subclasses.
Acknowledged
std::unique_ptr<ui::LayerSolidColor> viewport_background_layer_;If Layer interface is enough, just use Layer, and instantiate the specific class.
I will keep the the layer type explicit when defining member variables since it provides us better readability and intention of how these layer will be used.
However if a class is storing a general reference to a layer that it does not own, I will keep it as ui::Layer most of the time.
const ui::LayerSolidColor* GetViewportMagnifierLayerForTesting() const;do you need to expose the type for this API?
As discussed offline, methods like these should returns ui::Layer as long as the Layer interface is enough.
// TODO(b:522627357): Add a templated version.Why does clone and mirror need to be templated? Aren't the specific types subclasses of Layer? I.e. isn't this just a plain virtual function that is implemented in each of the subclasses?
I wanted it be a virtual method so that I clone in additional type specific properties. That was the main goal.
I think it was nice to have Clone/mirror method to return the subclass. Hence the comment (which might not be possible)
static std::unique_ptr<Layer> Create(LayerType type);Mitsuru Oshima@oshima-san, let me know if you are okay with the idea. I will then proceed to fix the compilation and change a few more sematics.
Robert FlackYes
I'm not sure if I see the advantage of templating the create method over having callers call the specific create method for the layer type they want - e.g. `LayerTextured::Create()`. The latter would support having specific required parameters for some layer types if this made sense for some layer types.
I agree that it is better to have a factory method defined in the subclasses than templating it, given the future flexibility of supplying type specific paramters.
Related details
- Mitsuru Oshima
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 3 comments![]( "Open in Gerrit")
LayerType type() const { return type_; }I wonder if it would be better to make this a pure virtual function overridden in the subclasses. There's technically no need to store this as a member variable since the specific subclass vtable determines the type and making this explicit would mean no chance of those two being out of sync.
// TODO(b:522627357): Add a templated version.Zoraiz NaeemWhy does clone and mirror need to be templated? Aren't the specific types subclasses of Layer? I.e. isn't this just a plain virtual function that is implemented in each of the subclasses?
I wanted it be a virtual method so that I clone in additional type specific properties. That was the main goal.
I think it was nice to have Clone/mirror method to return the subclass. Hence the comment (which might not be possible)
Clone and Mirror should be pure virtual in the base class and overridden in the subclasses which will allow for cloning and mirroring the subclass specific properties.
std::unique_ptr<Layer> Layer::Create(LayerType type) {We should remove this and require callers invoke the specific subclass create method.
Related details
- Mitsuru Oshima
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |