Skip to content
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

Clean up from enabling lh/rlh units #8264

Conversation

litherum
Copy link
Contributor

@litherum litherum commented Jan 6, 2023

c7a1baf

Clean up from enabling lh/rlh units
https://bugs.webkit.org/show_bug.cgi?id=250163
rdar://103933593

Reviewed by Tim Nguyen.

I looked through all the places where the preference name was mentioned.

* LayoutTests/imported/w3c/web-platform-tests/css/css-values/lh-rlh-on-root-001.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-values/lh-unit-001.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-values/lh-unit-002.html:
* Source/WebCore/css/typedom/CSSUnitValue.cpp:
(WebCore::CSSUnitValue::parseUnit):
* Source/WebCore/page/DeprecatedGlobalSettings.h:
* Tools/DumpRenderTree/TestOptions.cpp:
(WTR::TestOptions::defaults):

Canonical link: https://commits.webkit.org/258603@main

accc257

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe   πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ›  gtk   πŸ›  wincairo
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2   πŸ§ͺ api-mac   πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1   πŸ§ͺ api-gtk
  πŸ›  tv βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge   πŸ›  watch-sim

@litherum litherum self-assigned this Jan 6, 2023
@litherum litherum added the CSS Cascading Style Sheets implementation label Jan 6, 2023
@@ -150,7 +150,7 @@ const TestFeatures& TestOptions::defaults()
{ "JavaScriptEnabled", true },
{ "KeygenElementEnabled", false },
{ "LayoutFormattingContextIntegrationEnabled", true },
{ "LineHeightUnitsEnabled", false },
{ "LineHeightUnitsEnabled", true },
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed if UnifiedWebPreferences.yaml sets the status to stable (which was the intent of the other commit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll update the patch to do that.

I'll also modify any layout tests which flip the flag individually.

@@ -365,9 +365,7 @@ class DeprecatedGlobalSettings {

bool m_CSSLogicalEnabled { false };

// False by default until https://bugs.webkit.org/show_bug.cgi?id=211351 /
// https://github.com/w3c/csswg-drafts/issues/3257 have been sorted out.
bool m_lineHeightUnitsEnabled { false };
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove the corresponding lineHeightUnitsEnabled() & setLineHeightUnitsEnabled and stop exposing it from DeprecatedGlobalSettings (remove the webcoreBinding field) and remove any corresponding usages as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think removing a flag is a different patch than flipping a flag.

Copy link
Member

Choose a reason for hiding this comment

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

I was talking about removing it from DeprecatedGlobalSettings (and keeping it on Settings), but glazing at the code, it's not trivial, so punting this seems fine.

@litherum litherum force-pushed the eng/Clean-up-from-enabling-lhrlh-units branch from d4b0f7f to accc257 Compare January 7, 2023 04:30
@litherum litherum added the merge-queue Applied to send a pull request to merge-queue label Jan 7, 2023
https://bugs.webkit.org/show_bug.cgi?id=250163
rdar://103933593

Reviewed by Tim Nguyen.

I looked through all the places where the preference name was mentioned.

* LayoutTests/imported/w3c/web-platform-tests/css/css-values/lh-rlh-on-root-001.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-values/lh-unit-001.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-values/lh-unit-002.html:
* Source/WebCore/css/typedom/CSSUnitValue.cpp:
(WebCore::CSSUnitValue::parseUnit):
* Source/WebCore/page/DeprecatedGlobalSettings.h:
* Tools/DumpRenderTree/TestOptions.cpp:
(WTR::TestOptions::defaults):

Canonical link: https://commits.webkit.org/258603@main
@webkit-commit-queue
Copy link
Collaborator

Committed 258603@main (c7a1baf): https://commits.webkit.org/258603@main

Reviewed commits have been landed. Closing PR #8264 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit c7a1baf into WebKit:main Jan 7, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 7, 2023
@litherum litherum deleted the eng/Clean-up-from-enabling-lhrlh-units branch January 7, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Cascading Style Sheets implementation
Projects
None yet
4 participants