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

Avoid code duplication between ComputedStylePropertyMapReadOnly and MainThreadStylePropertyMapReadOnly #7006

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Dec 1, 2022

95de65d

Avoid code duplication between ComputedStylePropertyMapReadOnly and MainThreadStylePropertyMapReadOnly
https://bugs.webkit.org/show_bug.cgi?id=248583

Reviewed by Antti Koivisto.

Avoid code duplication between ComputedStylePropertyMapReadOnly and
MainThreadStylePropertyMapReadOnly by having ComputedStylePropertyMapReadOnly
subclass MainThreadStylePropertyMapReadOnly instead of the more generic
StylePropertyMapReadOnly.

Doing so exposed a bug in the way we serialize shorthand property values in
MainThreadStylePropertyMapReadOnly and caused some test failures. To address
the issue, we now avoid doing the serialization ourselves and rely on
StyleProperties::getPropertyValue() instead. Fixing this actually caused quite
a few tests to pass.

* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/background-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/border-radius-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/flex-flow-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/font-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/font-synthesis-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/font-variant-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/grid-area-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/grid-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/grid-template-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/marker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/mask-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/offset-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/text-decoration-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/text-decoration-skip-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/transition-expected.txt:
* Source/WebCore/css/typedom/CSSStyleValueFactory.cpp:
(WebCore::CSSStyleValueFactory::constructStyleValueForShorthandSerialization):
(WebCore::CSSStyleValueFactory::extractShorthandCSSValues):
(WebCore::CSSStyleValueFactory::constructStyleValueForShorthandProperty): Deleted.
* Source/WebCore/css/typedom/CSSStyleValueFactory.h:
* Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:
(WebCore::ComputedStylePropertyMapReadOnly::propertyValue const):
(WebCore::ComputedStylePropertyMapReadOnly::shorthandPropertySerialization const):
(WebCore::ComputedStylePropertyMapReadOnly::customPropertyValue const):
(WebCore::ComputedStylePropertyMapReadOnly::get const): Deleted.
(WebCore::ComputedStylePropertyMapReadOnly::getAll const): Deleted.
(WebCore::ComputedStylePropertyMapReadOnly::has const): Deleted.
* Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.h:
* Source/WebCore/css/typedom/DeclaredStylePropertyMap.cpp:
(WebCore::DeclaredStylePropertyMap::shorthandPropertySerialization const):
* Source/WebCore/css/typedom/DeclaredStylePropertyMap.h:
* Source/WebCore/css/typedom/MainThreadStylePropertyMapReadOnly.cpp:
(WebCore::MainThreadStylePropertyMapReadOnly::get const):
(WebCore::MainThreadStylePropertyMapReadOnly::getAll const):
(WebCore::MainThreadStylePropertyMapReadOnly::shorthandPropertyValue const): Deleted.
* Source/WebCore/css/typedom/MainThreadStylePropertyMapReadOnly.h:
* Source/WebCore/dom/StyledElement.cpp:
* Source/WebCore/html/CustomPaintImage.cpp:

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

083f7c7

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ❌ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🛠 mac-AS-debug ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 api-mac ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk1
✅ 🛠 tv-sim ✅ 🧪 mac-wk2
🛠 🧪 merge ✅ 🛠 watch ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 mac-wk2-stress

@cdumez cdumez requested a review from rniwa as a code owner December 1, 2022 03:48
@cdumez cdumez self-assigned this Dec 1, 2022
@cdumez cdumez added the CSS Cascading Style Sheets implementation label Dec 1, 2022
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 1, 2022
@cdumez cdumez removed the merging-blocked Applied to prevent a change from being merged label Dec 1, 2022
@cdumez cdumez added merge-queue Applied to send a pull request to merge-queue unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merge-queue Applied to send a pull request to merge-queue labels Dec 2, 2022
…ainThreadStylePropertyMapReadOnly

https://bugs.webkit.org/show_bug.cgi?id=248583

Reviewed by Antti Koivisto.

Avoid code duplication between ComputedStylePropertyMapReadOnly and
MainThreadStylePropertyMapReadOnly by having ComputedStylePropertyMapReadOnly
subclass MainThreadStylePropertyMapReadOnly instead of the more generic
StylePropertyMapReadOnly.

Doing so exposed a bug in the way we serialize shorthand property values in
MainThreadStylePropertyMapReadOnly and caused some test failures. To address
the issue, we now avoid doing the serialization ourselves and rely on
StyleProperties::getPropertyValue() instead. Fixing this actually caused quite
a few tests to pass.

* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/background-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/border-radius-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/flex-flow-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/font-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/font-synthesis-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/font-variant-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/grid-area-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/grid-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/grid-template-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/marker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/mask-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/offset-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/text-decoration-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/text-decoration-skip-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/transition-expected.txt:
* Source/WebCore/css/typedom/CSSStyleValueFactory.cpp:
(WebCore::CSSStyleValueFactory::constructStyleValueForShorthandSerialization):
(WebCore::CSSStyleValueFactory::extractShorthandCSSValues):
(WebCore::CSSStyleValueFactory::constructStyleValueForShorthandProperty): Deleted.
* Source/WebCore/css/typedom/CSSStyleValueFactory.h:
* Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:
(WebCore::ComputedStylePropertyMapReadOnly::propertyValue const):
(WebCore::ComputedStylePropertyMapReadOnly::shorthandPropertySerialization const):
(WebCore::ComputedStylePropertyMapReadOnly::customPropertyValue const):
(WebCore::ComputedStylePropertyMapReadOnly::get const): Deleted.
(WebCore::ComputedStylePropertyMapReadOnly::getAll const): Deleted.
(WebCore::ComputedStylePropertyMapReadOnly::has const): Deleted.
* Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.h:
* Source/WebCore/css/typedom/DeclaredStylePropertyMap.cpp:
(WebCore::DeclaredStylePropertyMap::shorthandPropertySerialization const):
* Source/WebCore/css/typedom/DeclaredStylePropertyMap.h:
* Source/WebCore/css/typedom/MainThreadStylePropertyMapReadOnly.cpp:
(WebCore::MainThreadStylePropertyMapReadOnly::get const):
(WebCore::MainThreadStylePropertyMapReadOnly::getAll const):
(WebCore::MainThreadStylePropertyMapReadOnly::shorthandPropertyValue const): Deleted.
* Source/WebCore/css/typedom/MainThreadStylePropertyMapReadOnly.h:
* Source/WebCore/dom/StyledElement.cpp:
* Source/WebCore/html/CustomPaintImage.cpp:

Canonical link: https://commits.webkit.org/257284@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the ComputedStylePropertyMapReadOnly_simplify branch from 083f7c7 to 95de65d Compare December 2, 2022 16:05
@webkit-commit-queue
Copy link
Collaborator

Committed 257284@main (95de65d): https://commits.webkit.org/257284@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit 95de65d into WebKit:main Dec 2, 2022
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Dec 2, 2022
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
5 participants