Made MetalSDF a printed name when sdf's are enabled.#185504
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request updates the logging in embedder_surface_metal_impeller.mm to reflect the use of Signed Distance Fields (SDFs). A review comment points out that the variable impeller_flags is undefined in the scope, which will lead to a compilation error, and suggests refactoring the log statement to eliminate string duplication.
| if (impeller_flags.use_sdfs) { | ||
| FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (MetalSDF)."; | ||
| } else { | ||
| FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (Metal)."; | ||
| } |
There was a problem hiding this comment.
The variable impeller_flags is not defined in this scope, which will cause a compilation error. Additionally, the log message can be simplified using a ternary operator to avoid duplicating the string literal, which improves maintainability and adheres to the style guide's recommendation to avoid duplicating state (Rule 35).
FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (Metal" << (impeller_flags.use_sdfs ? "SDF" : "") << ").";
There was a problem hiding this comment.
Ummm... gem? You having a bad day?
| if (impeller_flags.use_sdfs) { | ||
| FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (MetalSDF)."; | ||
| } else { | ||
| FML_LOG(IMPORTANT) << "Using the Impeller rendering backend (Metal)."; | ||
| } |
There was a problem hiding this comment.
Ummm... gem? You having a bad day?
flutter/flutter@aeb9623...3100188 2026-04-24 engine-flutter-autoroll@skia.org Roll Skia from 1c9b0b9141e9 to 63dbf37c3700 (1 revision) (flutter/flutter#185533) 2026-04-24 30870216+gaaclarke@users.noreply.github.com Made MetalSDF a printed name when sdf's are enabled. (flutter/flutter#185504) 2026-04-24 30870216+gaaclarke@users.noreply.github.com Made the MIT matcher not greedy and added a boost file matcher (flutter/flutter#185491) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…#11579) flutter/flutter@aeb9623...3100188 2026-04-24 engine-flutter-autoroll@skia.org Roll Skia from 1c9b0b9141e9 to 63dbf37c3700 (1 revision) (flutter/flutter#185533) 2026-04-24 30870216+gaaclarke@users.noreply.github.com Made MetalSDF a printed name when sdf's are enabled. (flutter/flutter#185504) 2026-04-24 30870216+gaaclarke@users.noreply.github.com Made the MIT matcher not greedy and added a boost file matcher (flutter/flutter#185491) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This makes it easier to know when you are using SDFs or not.
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.