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

Refactor clip layers to share common code #32889

Merged
fluttergithubbot merged 3 commits into from
May 12, 2022
Merged

Refactor clip layers to share common code #32889

fluttergithubbot merged 3 commits into from
May 12, 2022

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Apr 24, 2022 •
edited
Loading

Added a new template class ClipShapeLayer to share the common code of clip layers, which also fixes the issue that ClipRectLayer uses clip rect for saveLayer bounds.

fix flutter/flutter#102341
fix (Partially) flutter/flutter#70406 (This PR only refactors the clip layers, not the clip layers unit tests. I've tried refactoring those unit tests, but haven't found a good way to do that.)

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.

@ColdPaleLight ColdPaleLight requested a review from flar April 24, 2022 13:44
flar
flar reviewed May 12, 2022
View reviewed changes
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

LGTM, just formatting and a cleanup suggestion.

(Not sure if there is a way to macro-ify the sub-class names in the TRACE_EVENT strings at compile time to avoid the overrides, but also don't think it is worth it to try to do something that might impact the runtime for Release builds)

@ColdPaleLight ColdPaleLight requested a review from flar May 12, 2022 03:09
flar
flar approved these changes May 12, 2022
View reviewed changes
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

LGTM.

Note that there is a slight behavior change for rect where it used to use the entire rect for the saveLayer and checkerboard, but now uses the clipped child bounds - as you discovered when you had to modify a test case. This should be a good thing and should be consistent across the clip types, but may cause some off-by-1 saveLayer anomalies and we'll have to be on the lookout for potential golden file changes down the line.

@ColdPaleLight ColdPaleLight added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label May 12, 2022
@fluttergithubbot fluttergithubbot merged commit d2a53d6 into flutter:main May 12, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 12, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 12, 2022
@engine-flutter-autoroll
d2a53d6 Refactor clip layers to share common code (flutter/engine#32889)
db900dd
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 12, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 12, 2022
@engine-flutter-autoroll
d2a53d6 Refactor clip layers to share common code (flutter/engine#32889)
26440ed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 12, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 12, 2022
@engine-flutter-autoroll
d2a53d6 Refactor clip layers to share common code (flutter/engine#32889)
d86d1f4
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 12, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 12, 2022
@engine-flutter-autoroll
d2a53d6 Refactor clip layers to share common code (flutter/engine#32889)
9a645f0
@wangying3426 wangying3426 mentioned this pull request May 16, 2022
Merged
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@flar flar flar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ClipRectLayer uses clip rect for saveLayer bounds

3 participants

@ColdPaleLight @flar @fluttergithubbot