Repository navigation
Add ClipPath optimizations to backdrop clips as well - #189118
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates ClipPathLayer to inspect its clip shape and push the most specific clip type (Rect, Oval, or RoundRect) to the view embedder instead of always pushing a generic path. It also updates MockViewEmbedder and adds corresponding unit tests to verify that the correct clip types are pushed for various clip layers. The feedback suggests refactoring the nested conditional checks in ClipPathLayer::PushClipToEmbeddedNativeViewMutatorStack by caching the clip shape reference and using C++17 init-statements to improve readability.
| void ClipPathLayer::PushClipToEmbeddedNativeViewMutatorStack( | ||
| ExternalViewEmbedder* view_embedder) const { | ||
| view_embedder->PushClipPathToVisitedPlatformViews(clip_shape()); | ||
| DlRect rect; | ||
| if (clip_shape().IsRect(&rect)) { | ||
| view_embedder->PushClipRectToVisitedPlatformViews(rect); | ||
| } else if (clip_shape().IsOval(&rect)) { | ||
| view_embedder->PushClipRRectToVisitedPlatformViews( | ||
| DlRoundRect::MakeOval(rect)); | ||
| } else { | ||
| DlRoundRect rrect; | ||
| if (clip_shape().IsRoundRect(&rrect)) { | ||
| view_embedder->PushClipRRectToVisitedPlatformViews(rrect); | ||
| } else { | ||
| view_embedder->PushClipPathToVisitedPlatformViews(clip_shape()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation calls clip_shape() multiple times and uses nested if-else blocks, which increases nesting and reduces readability.\n\nWe can improve this by:\n1. Caching the result of clip_shape() in a local const DlPath& reference.\n2. Flattening the control flow using a C++17 init-statement in the else if block to declare DlRoundRect rrect only where it is needed.\n\nThis simplifies the logic and makes the code much cleaner and easier to read.
void ClipPathLayer::PushClipToEmbeddedNativeViewMutatorStack(\n ExternalViewEmbedder* view_embedder) const {\n const DlPath& path = clip_shape();\n DlRect rect;\n if (path.IsRect(&rect)) {\n view_embedder->PushClipRectToVisitedPlatformViews(rect);\n } else if (path.IsOval(&rect)) {\n view_embedder->PushClipRRectToVisitedPlatformViews(\n DlRoundRect::MakeOval(rect));\n } else if (DlRoundRect rrect; path.IsRoundRect(&rrect)) {\n view_embedder->PushClipRRectToVisitedPlatformViews(rrect);\n } else {\n view_embedder->PushClipPathToVisitedPlatformViews(path);\n }\n}There was a problem hiding this comment.
clip_shape() just returns the instance so doesn't matter much. No strong opinion on the C++17 syntax, I wanted to keep the shape consistent with ApplyClip above
hellohuanlin
left a comment
hellohuanlin
left a comment
There was a problem hiding this comment.
sorry for the delay - just one nit
| ExternalViewEmbedder* view_embedder) const { | ||
| view_embedder->PushClipPathToVisitedPlatformViews(clip_shape()); | ||
| DlRect rect; | ||
| if (clip_shape().IsRect(&rect)) { |
There was a problem hiding this comment.
can you call clip_shape() once upfront to avoid calling it multiple times?
There was a problem hiding this comment.
I thought about doing that but clip_shape() is just a getter and I wanted to match the pattern in ApplyClip but down to change. Should I update both spots?
There was a problem hiding this comment.
oh if it's just a getter, then im fine either way.
There was a problem hiding this comment.
Yeah its just const ClipShape& clip_shape() const { return clip_shape_; }
|
autosubmit label was removed for flutter/flutter/189118, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
auto-submit
Bot
removed
the
autosubmit
Hari-07
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
clip_path_layer.cc,ApplyClip()has optimizations to convert to more efficient shapes if applicable. This PR extends that behaviour to backdrop clips as wellFixes #188971
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.