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

Remove getPropertyShorthand from the web exposed API. #6456

Conversation

darinadler
Copy link
Member

@darinadler darinadler commented Nov 14, 2022

80c7a3d

Remove getPropertyShorthand from the web exposed API.
https://bugs.webkit.org/show_bug.cgi?id=87129
rdar://problem/102303976

Reviewed by Tim Nguyen.

* LayoutTests/css3/color-filters/color-filter-exposed-if-disabled-expected.txt: Updated.
* LayoutTests/css3/color-filters/color-filter-exposed-if-disabled.html: Removed call to
getPropertyShorthand.

* LayoutTests/css3/color-filters/color-filter-exposed-if-enabled-expected.txt: Updated.
* LayoutTests/css3/color-filters/color-filter-exposed-if-enabled.html: Removed call to
getPropertyShorthand.

* LayoutTests/fast/backgrounds/repeat/background-repeat-shorthand-expected.txt: Removed.
* LayoutTests/fast/backgrounds/repeat/background-repeat-shorthand.html: Removed.
* LayoutTests/fast/backgrounds/repeat/resources/background-repeat-shorthand.js: Removed.

* LayoutTests/fast/css/style-enumerate-properties-expected.txt: Updated.
* LayoutTests/fast/css/style-enumerate-properties.html: Removed getPropertyShorthand from the list
of property names expected.

* Source/WebCore/css/CSSStyleDeclaration.idl: Removed getPropertyShorthand.

* Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js: Removed
getPropertyShorthand and isPropertyImplicit.

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

02a8ab0

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
βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress

@darinadler darinadler self-assigned this Nov 14, 2022
@darinadler darinadler added 528+ (Nightly build) DOM For bugs specific to XML/HTML DOM elements (including parsing). labels Nov 14, 2022
@darinadler darinadler requested a review from nt1m November 14, 2022 06:40
@darinadler darinadler added CSS Cascading Style Sheets implementation and removed 528+ (Nightly build) labels Nov 14, 2022
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

The version on CSSComputedStyleDeclaration seems pretty useless, I wonder if we could clean that up:

https://searchfox.org/wubkat/rev/5a9504aef82d929917a680a05a3fc17f7f6b4dde/Source/WebCore/css/CSSComputedStyleDeclaration.cpp#200-203

@darinadler
Copy link
Member Author

We can remove the silly comment. But more importantly we just need to remove the callers and get rid of the function completely. That’s the way to clean it up.

@darinadler darinadler added the merge-queue Applied to send a pull request to merge-queue label Nov 14, 2022
@nt1m
Copy link
Member

nt1m commented Nov 14, 2022

cc @karlcow here too, to check how web compatible it is (I suspect it is).

https://bugs.webkit.org/show_bug.cgi?id=87129
rdar://problem/102303976

Reviewed by Tim Nguyen.

* LayoutTests/css3/color-filters/color-filter-exposed-if-disabled-expected.txt: Updated.
* LayoutTests/css3/color-filters/color-filter-exposed-if-disabled.html: Removed call to
getPropertyShorthand.

* LayoutTests/css3/color-filters/color-filter-exposed-if-enabled-expected.txt: Updated.
* LayoutTests/css3/color-filters/color-filter-exposed-if-enabled.html: Removed call to
getPropertyShorthand.

* LayoutTests/fast/backgrounds/repeat/background-repeat-shorthand-expected.txt: Removed.
* LayoutTests/fast/backgrounds/repeat/background-repeat-shorthand.html: Removed.
* LayoutTests/fast/backgrounds/repeat/resources/background-repeat-shorthand.js: Removed.

* LayoutTests/fast/css/style-enumerate-properties-expected.txt: Updated.
* LayoutTests/fast/css/style-enumerate-properties.html: Removed getPropertyShorthand from the list
of property names expected.

* Source/WebCore/css/CSSStyleDeclaration.idl: Removed getPropertyShorthand.

* Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js: Removed
getPropertyShorthand and isPropertyImplicit.

Canonical link: https://commits.webkit.org/256636@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/Remove-getPropertyShorthand-from-the-web-exposed-API- branch from 02a8ab0 to 80c7a3d Compare November 14, 2022 08:18
@webkit-commit-queue
Copy link
Collaborator

Committed 256636@main (80c7a3d): https://commits.webkit.org/256636@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit 80c7a3d into WebKit:main Nov 14, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 14, 2022
Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

This will not break the Web indeed.

@darinadler darinadler deleted the eng/Remove-getPropertyShorthand-from-the-web-exposed-API- branch November 14, 2022 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Cascading Style Sheets implementation DOM For bugs specific to XML/HTML DOM elements (including parsing).
Projects
None yet
5 participants