-
Notifications
You must be signed in to change notification settings - Fork 16.2k
fix: corner smoothing feature gate crash #47759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
} | ||
}) | ||
``` | ||
|
||
The CSS rule will still parse, but will have no visual effect. |
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.
This removal was intentional: the built-in runtime feature gating will prevent this rule from parsing, similar to how experimental CSS properties work.
+ // Since the Electron implementation of DrawSmoothRoundRect uses one radius | ||
+ // for both dimensions, we need to use the minimum of the two supplied. | ||
+ auto min_radius = [](const gfx::SizeF& radius) -> float { | ||
+ return std::min(radius.width(), radius.height()); | ||
+ }; | ||
+ |
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.
Drive-by fix for a bug I ran into while testing
--- a/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc | ||
+++ b/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc | ||
@@ -12325,5 +12325,25 @@ const CSSValue* InternalEmptyLineHeight::ParseSingleValue( | ||
@@ -12325,5 +12325,31 @@ const CSSValue* InternalEmptyLineHeight::ParseSingleValue( | ||
CSSValueID::kNone>(stream); | ||
} | ||
|
||
+const CSSValue* ElectronCornerSmoothing::ParseSingleValue( | ||
+ CSSParserTokenStream& stream, | ||
+ const CSSParserContext& context, | ||
+ const CSSParserLocalContext&) const { |
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.
With the blink runtime flag disabled, is this codepath no longer entered? Just want to confirm whether that's the reason why feature checking is no longer needed here.
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.
That's correct 👍 For comparison, other experimental CSS properties rely on the JSON definition file to supply the required flag, instead of the parsing code here.
Is there any public discussion that I could read through on why this non-standard Electron-only CSS property is enabled by default? It seems like an odd choice when |
@absidue First off, thanks for the feedback! That is a great question to raise. I've thought about this a lot during this feature's development. The design & rationale for this feature was captured in RFC 12 and its corresponding PR. The non-standard-ness was a point of discussion I proactively brought up in the RFC text and during the API Working Group meetings. Your question about consistency with other non-standard APIs is fair to ask. Our thinking was based on precedent, ergonomic design, and risk assessment:
To directly answer your last question: Electron's policy on avoiding non-standard APIs hasn't changed. We still weigh the purpose and utility of a feature against its drawbacks—security, privacy, and non-standard-ness. In this case, we felt the feature was an appropriate addition to the platform. All that said, we're open to revisiting this decision. You're one of the first people to reach out to me about this topic, so if you feel the trade-off isn't right and have a motivating explanation for why corner smoothing should be disabled by default, then please feel free to open a new discussion issue! Footnotes
|
Release Notes Persisted
|
I was unable to backport this PR to "37-x-y" cleanly; |
I have automatically backported this PR to "38-x-y", please check out #47785 |
* fix: corner smoothing feature gate crash * Fix ElectronCornerSmoothing::CSSValueFromComputedStyleInternal
fix: corner smoothing feature gate crash (#47759)
Description of Change
There was a crash in
third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
from parsing the-electron-corner-smoothing
CSS property when the property was added to a stylesheet with no associated document. To resolve this, I changed the feature gating to use Blink's built-in runtime feature gating instead. As a result, I was able to cut out a majority of the patch that was just for plumbing the web preference. There is an insignificant API change for how the feature is disabled.Checklist
npm test
passesRelease Notes
Notes: Fixed a crash when adding the
-electron-corner-smoothing
CSS rule to a stylesheet with no associated document.