-
Notifications
You must be signed in to change notification settings - Fork 6k
Create blanket backdrop filter mutator #34408
Conversation
cyanglaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@dnfield Do you mind taking a secondary review? |
|
@JTKryptic I just realized that the check lists of the PR summary is gone. Do you mind adding them back and complete the check list? |
flow/embedded_views.h
Outdated
| clipRect, | ||
| clipRRect, | ||
| clipPath, | ||
| transform, | ||
| opacity, | ||
| backdropFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought the link I gave said this, but these should be
kClipRect,
kClipRRect,
kClipPath,
kTransform,
kOpacity,
kBackdropFilter(and elsewhere)
|
| SkMatrix matrix_; | ||
| SkPath* path_; | ||
| int alpha_; | ||
| const DlImageFilter* filter_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be stored as std::shared_ptr. The lifecycle is probably not going to fail with the current architecture, but we should be safe.
Also the SkPath object is supposed to be fast-copy in that it shares the underlying storage and marks the origin object as copy-on-write. Given that our paths never change, we could probably store an SkPath rather than a pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I will make that change for the next PR. Thanks!
| ASSERT_TRUE(iter->get()->GetAlpha() == 240); | ||
| } | ||
|
|
||
| TEST(MutatorsStack, PushBackdropFilter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It couldn't hurt to add a test to confirm that the == method on the mutators works for all types, including if they are fed copies of the original data (i.e. they compare by content rather than reference).
|
Ah, sorry, I guess I missed the merge on this. Can we address the shared ptr issue at least in a follow-up? |
Creates the mutator object for the backdrop filter effect for platform views. To be combined with #34355 (Create BackdropFilter Mutator) and #34596 (PlatformView Blur for Backdrop Filter).
Related issues: #43902
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.