[TreesInViz] Change T* Node() to const T& Node() in PropertyTree [chromium/src : main]
Tzarial (Gerrit)
Tzarial added 1 comment![]( "Open in Gerrit")
Related details
- Zhenyao Mo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zhenyao Mo (Gerrit)
Zhenyao Mo added 3 comments![]( "Open in Gerrit")
Thank you for working on this. It's a great effort to a more secure code base.
I haven't read through the entire CL, but I have some early questions on the design.
TransformNode* node = MutableNode(id);Same here, we expect the return to be non-null, so its a candidate to return reference.
TransformNode* node = MutableNode(id);Quickly scanning through this file, I noticed, for example, here apparently we expect the node and parent_node to be non-null. So should we switch it also to reference rather than pointer? i.e., have a non-constant version of Node() and Parent() as well?
Related details
- Tzarial
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. |
Tzarial (Gerrit)
Tzarial added 2 comments![]( "Open in Gerrit")
TransformNode* node = MutableNode(id);Same here, we expect the return to be non-null, so its a candidate to return reference.
We didn't used to allow non-const references, but this seems to be recommended by https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
I will update the CL to return references.
TransformNode* node = MutableNode(id);Quickly scanning through this file, I noticed, for example, here apparently we expect the node and parent_node to be non-null. So should we switch it also to reference rather than pointer? i.e., have a non-constant version of Node() and Parent() as well?
Yes. That will be a follow up CL; this is large enough that I want to limit the scope as much as possible.
Related details
- Zhenyao Mo
Tzarial (Gerrit)
Tzarial added 2 comments![]( "Open in Gerrit")
This CL should be ready for another review pass!
TzarialSame here, we expect the return to be non-null, so its a candidate to return reference.
We didn't used to allow non-const references, but this seems to be recommended by https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
I will update the CL to return references.
Done
Related details
- Zhenyao Mo
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. |
Zhenyao Mo (Gerrit)
Zhenyao Mo added 3 comments![]( "Open in Gerrit")
Again, I only looked at property_tree.h first and I have two questions.
It's OK this is by design and you intend to clean it up in a followup.
Let me know and I'll finish the review.
Thanks for taking on such a large but super valuable refactoring!
T* MutableFindNodeFromElementId(ElementId id) {Isn't this the same as T* FindNodeFromElementId() in #121?
T* MutableBack() { return size() ? &nodes_.back() : nullptr; }Isn't this the same as T* back() in #113?
Related details
- Tzarial
Tzarial (Gerrit)
Tzarial added 2 comments![]( "Open in Gerrit")
T* MutableFindNodeFromElementId(ElementId id) {Isn't this the same as T* FindNodeFromElementId() in #121?
Yes. Eliminated the old one.
T* MutableBack() { return size() ? &nodes_.back() : nullptr; }Isn't this the same as T* back() in #113?
- Zhenyao Mo
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. |
Zhenyao Mo (Gerrit)
Zhenyao Mo added 25 comments![]( "Open in Gerrit")
EffectNode& node = effect_tree.MutableNode(EffectTreeIndex());This can be Node instead
DCHECK(filter_node);Here can be const EffectNode& and remove the DCHECK.
const ClipNode* clip_node = &clip_tree.Node(local_clip_id);Can this be reference instead?
const EffectNode* target_node = &effect_tree.Node(target_id);Can this be reference instead?
const TransformNode* node = &tree.Node(layer->transform_tree_index());Can this be reference instead?
const TransformNode* parent = &tree.Node(node->parent_id);Can this be reference instead?
const EffectNode* node = &tree.Node(render_surface->EffectTreeIndex());Can this be reference instead?
const EffectNode* node = &tree.Node(layer->effect_tree_index());Can these two be reference instead?
const EffectNode* effect_node =Can this be reference instead?
&property_trees->clip_tree().Node(layer->clip_tree_index());Can this be reference instead?
const EffectNode* root_effect_node =Can this be reference instead?
const TransformNode* transform_node =Can this be reference instead?
const EffectNode* effect_node =Can this be reference instead?
&property_trees->transform_tree().Node(layer->transform_tree_index());Can this be reference instead?
&property_trees->effect_tree().Node(render_surface->EffectTreeIndex());Can this be reference instead?
DCHECK_NE(layer->scroll_tree_index(), kInvalidPropertyNodeId);I think you can just do Node as you did in #1124. It's more than this DCHECK_NE
node_id = node.parent_frame_id;Both this loop and the above are much more secure!
T* MutableFindNodeFromElementId(ElementId id) {TzarialIsn't this the same as T* FindNodeFromElementId() in #121?
Yes. Eliminated the old one.
Acknowledged
T* MutableBack() { return size() ? &nodes_.back() : nullptr; }TzarialIsn't this the same as T* back() in #113?
Yes. Eliminated the old one.
Acknowledged
TransformNode* node = MutableNode(id);Quickly scanning through this file, I noticed, for example, here apparently we expect the node and parent_node to be non-null. So should we switch it also to reference rather than pointer? i.e., have a non-constant version of Node() and Parent() as well?
Yes. That will be a follow up CL; this is large enough that I want to limit the scope as much as possible.
Acknowledged
auto& cc_node = effect_tree.MutableNode(effect_id);This can be const and Node
scroll_tree_.MutableFindNodeFromElementId(element_id)) {This can be const
transform_tree_.MutableFindNodeFromElementId(element_id);This can be const
DCHECK_NE(parent_id, cc::kInvalidPropertyNodeId);No need to DCHECK here because Node() below does the same CHECK
Related details
- Tzarial
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. |