Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@JTKryptic
Copy link
Contributor

@JTKryptic JTKryptic commented Jun 30, 2022

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added embedder Related to the embedder API platform-ios labels Jun 30, 2022
@JTKryptic JTKryptic requested a review from cyanglaz July 8, 2022 16:19
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyanglaz cyanglaz requested a review from dnfield July 13, 2022 03:25
@cyanglaz
Copy link
Contributor

@dnfield Do you mind taking a secondary review?
This is part of the iOS intern project to add backdrop filter (blur) for PlatformViews.

@cyanglaz
Copy link
Contributor

cyanglaz commented Jul 13, 2022

@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?

Comment on lines 24 to 29
clipRect,
clipRRect,
clipPath,
transform,
opacity,
backdropFilter
Copy link
Contributor

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)

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 13, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 13, 2022

  • The status or check suite Linux Fuchsia FEMU has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 13, 2022
@JTKryptic JTKryptic added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 13, 2022
@auto-submit auto-submit bot merged commit 495ed03 into flutter:main Jul 13, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2022
betrevisan pushed a commit to betrevisan/engine that referenced this pull request Jul 15, 2022
SkMatrix matrix_;
SkPath* path_;
int alpha_;
const DlImageFilter* filter_;
Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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).

@flar
Copy link
Contributor

flar commented Jul 16, 2022

Ah, sorry, I guess I missed the merge on this. Can we address the shared ptr issue at least in a follow-up?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App embedder Related to the embedder API platform-android platform-fuchsia platform-ios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants