Make CanvasResourceProvider::IsOriginTopLeft virtual [chromium/src : main]
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 1 comment![]( "Open in Gerrit")
Related details
- Jean-Philippe Gravel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jean-Philippe Gravel
| Code-Review | +1 |
1 comment
This CL makes it virtual and override in Passthrough, so it's easier
to eliminate call-sites and the function all together.Do you mean that in a follow up, you'll remove the IsOriginTopLeft method?
As a general thought on this, I find that we often abuse inheritance and virtual calls. Adding a virtual method to a base class when that feature is only ever needed by one child class seems more like and unfortunate consequence of how the code is structured (e.g. I need to use this feature of child class but all I have is a pointer to the parent class) than a correct use of an API interface.
But maybe that's not the case here: the position of the origin is a trait that all child classes care about, irrespective of the default. It remains that by replacing the member attribute with a virtual call, we are adding virtual dispatches in places where we originally had direct access to the value, which complexifies data flow and could impact performance. I've been bitten many times by thinking that a certain method is being used, when in reality, an unexpected remotely defined override is actually being used. All that to say that there is advantages to having a member attribute in the parent class. But if all this is going away, either option works.
Related details
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Commit-Queue | +2 |
2 comments
Thanks for the review.
This CL makes it virtual and override in Passthrough, so it's easier
to eliminate call-sites and the function all together.Do you mean that in a follow up, you'll remove the IsOriginTopLeft method?
As a general thought on this, I find that we often abuse inheritance and virtual calls. Adding a virtual method to a base class when that feature is only ever needed by one child class seems more like and unfortunate consequence of how the code is structured (e.g. I need to use this feature of child class but all I have is a pointer to the parent class) than a correct use of an API interface.
But maybe that's not the case here: the position of the origin is a trait that all child classes care about, irrespective of the default. It remains that by replacing the member attribute with a virtual call, we are adding virtual dispatches in places where we originally had direct access to the value, which complexifies data flow and could impact performance. I've been bitten many times by thinking that a certain method is being used, when in reality, an unexpected remotely defined override is actually being used. All that to say that there is advantages to having a member attribute in the parent class. But if all this is going away, either option works.
Yes, I plan to remove IsOriginTopLeft from everywhere eventually.
I agree that it has theoretical performance impact, but unless whole chrome would try to avoid virtual calls and memory allocations on the critical path there is no practical difference.
And I agree that from architectural point of view it might be better to have variable and non-virtual getter, but tooling-wise it's much more complicated to trace all places where variable comes from, especially if it's plumbed through few functions vs all overrides of the function. And to easier reasoning for the follow-up CLs that are based on "This code place doesn't use CanvasReourceProviderPassthrough, so origin can't be bottom left" I want to land this as prerequisite.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 1 comment![]( "Open in Gerrit")
This CL makes it virtual and override in Passthrough, so it's easier
to eliminate call-sites and the function all together.Vasiliy TelezhnikovDo you mean that in a follow up, you'll remove the IsOriginTopLeft method?
As a general thought on this, I find that we often abuse inheritance and virtual calls. Adding a virtual method to a base class when that feature is only ever needed by one child class seems more like and unfortunate consequence of how the code is structured (e.g. I need to use this feature of child class but all I have is a pointer to the parent class) than a correct use of an API interface.
But maybe that's not the case here: the position of the origin is a trait that all child classes care about, irrespective of the default. It remains that by replacing the member attribute with a virtual call, we are adding virtual dispatches in places where we originally had direct access to the value, which complexifies data flow and could impact performance. I've been bitten many times by thinking that a certain method is being used, when in reality, an unexpected remotely defined override is actually being used. All that to say that there is advantages to having a member attribute in the parent class. But if all this is going away, either option works.
Yes, I plan to remove IsOriginTopLeft from everywhere eventually.
I agree that it has theoretical performance impact, but unless whole chrome would try to avoid virtual calls and memory allocations on the critical path there is no practical difference.
And I agree that from architectural point of view it might be better to have variable and non-virtual getter, but tooling-wise it's much more complicated to trace all places where variable comes from, especially if it's plumbed through few functions vs all overrides of the function. And to easier reasoning for the follow-up CLs that are based on "This code place doesn't use CanvasReourceProviderPassthrough, so origin can't be bottom left" I want to land this as prerequisite.
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Make CanvasResourceProvider::IsOriginTopLeft virtual
Only CanvasResourceProviderPassthrough can have bottom left origin.
This CL makes it virtual and override in Passthrough, so it's easier
to eliminate call-sites and the function all together.
- M third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d_test.cc
- M third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
- M third_party/blink/renderer/platform/graphics/canvas_resource_provider.h
Code-Review: +1 by Jean-Philippe Gravel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |