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 ENABLE_CSS_PAINTING_API build flag #26627

Conversation

donny-dont
Copy link
Contributor

@donny-dont donny-dont commented Mar 29, 2024

4d1720e

Remove ENABLE_CSS_PAINTING_API build flag
https://bugs.webkit.org/show_bug.cgi?id=271913

Reviewed by Said Abou-Hallawa and Michael Catanzaro.

All ports are building with `ENABLE_CSS_PAINTING_API` as `ON`. Remove the
build option and use the runtime option for controlling use.

* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WTF/wtf/PlatformEnable.h:
* Source/WebCore/bindings/js/JSPaintRenderingContext2DCustom.cpp:
* Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:
* Source/WebCore/bindings/js/WebCoreJSClientData.cpp:
* Source/WebCore/bindings/js/WebCoreJSClientData.h:
* Source/WebCore/css/CSSPaintCallback.h:
* Source/WebCore/css/CSSPaintCallback.idl:
* Source/WebCore/css/CSSPaintImageValue.cpp:
* Source/WebCore/css/CSSPaintImageValue.h:
* Source/WebCore/css/CSSPaintSize.h:
* Source/WebCore/css/CSSPaintSize.idl:
* Source/WebCore/css/CSSValue.cpp:
* Source/WebCore/css/CSSValue.h:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/DOMCSSNamespace+CSSPainting.idl:
* Source/WebCore/css/DOMCSSPaintWorklet.cpp:
* Source/WebCore/css/DOMCSSPaintWorklet.h:
* Source/WebCore/css/parser/CSSParserContext.cpp:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
* Source/WebCore/css/typedom/MainThreadStylePropertyMapReadOnly.cpp:
* Source/WebCore/dom/Document.cpp:
* Source/WebCore/dom/Document.h:
* Source/WebCore/html/CustomPaintCanvas.cpp:
* Source/WebCore/html/CustomPaintCanvas.h:
* Source/WebCore/html/CustomPaintImage.cpp:
* Source/WebCore/html/CustomPaintImage.h:
* Source/WebCore/html/canvas/PaintRenderingContext2D.cpp:
* Source/WebCore/html/canvas/PaintRenderingContext2D.h:
* Source/WebCore/html/canvas/PaintRenderingContext2D.idl:
* Source/WebCore/rendering/style/RenderStyle.cpp:
* Source/WebCore/rendering/style/RenderStyle.h:
* Source/WebCore/rendering/style/StyleImage.h:
* Source/WebCore/rendering/style/StylePaintImage.cpp:
* Source/WebCore/rendering/style/StylePaintImage.h:
* Source/WebCore/style/StyleBuilder.cpp:
* Source/WebCore/style/StyleBuilderState.cpp:
* Source/WebCore/workers/WorkerOrWorkletScriptController.cpp:
* Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:
* Source/WebCore/worklets/PaintWorkletGlobalScope.h:
* Source/WebCore/worklets/PaintWorkletGlobalScope.idl:
* Source/WebCore/worklets/WorkletGlobalScope.h:
* Source/cmake/WebKitFeatures.cmake:
* Tools/Scripts/webkitperl/FeatureList.pm:

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

515baac

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

@donny-dont donny-dont self-assigned this Mar 29, 2024
@donny-dont donny-dont added the Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases label Mar 29, 2024
@donny-dont donny-dont marked this pull request as ready for review March 29, 2024 22:40
Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

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

"All ports are building with ENABLE_CSS_PAINTING_API as ON."

Warning klaxons: looks like this is completely wrong. It's disabled in all CMake ports except macOS (which doesn't matter because production macOS builds don't use CMake). ENABLE_EXPERIMENTAL_FEATURES does not get turned on in releases and it probably hasn't been tested anywhere except buildbots.

I would do a separate commit first to enable the build flag, land that first, then remove the build flag. I think it should be safe because the runtime feature is also disabled everywhere except if experimental features are enabled, so in theory it shouldn't matter. But still safer to do in two stages.

@donny-dont donny-dont force-pushed the eng/Remove-ENABLE_CSS_PAINTING_API-build-flag branch from 12c11a6 to 515baac Compare April 2, 2024 20:54
@shallawa
Copy link
Contributor

What is the rule for removing an ENABLE() flag if it is unconditionally set to true? Currently we have all these flags set to true. This list includes ENABLE_MATHML and the ENABLE_OFFSCREEN_CANVAS.

#if !defined(ENABLE_CONTEXT_MENUS)
#define ENABLE_CONTEXT_MENUS 1
#endif

#if !defined(ENABLE_CONTEXT_MENU_EVENT)
#define ENABLE_CONTEXT_MENU_EVENT 1
#endif

#if !defined(ENABLE_CSS_PAINTING_API)
#define ENABLE_CSS_PAINTING_API 1
#endif

#if !defined(ENABLE_CUSTOM_CURSOR_SUPPORT)
#define ENABLE_CUSTOM_CURSOR_SUPPORT 1
#endif

#if !defined(ENABLE_DRAG_SUPPORT)
#define ENABLE_DRAG_SUPPORT 1
#endif

#if !defined(ENABLE_FTPDIR)
#define ENABLE_FTPDIR 1
#endif

#if !defined(ENABLE_INPUT_TYPE_COLOR)
#define ENABLE_INPUT_TYPE_COLOR 1
#endif

#if !defined(ENABLE_MATHML)
#define ENABLE_MATHML 1
#endif

#if !defined(ENABLE_OFFSCREEN_CANVAS)
#define ENABLE_OFFSCREEN_CANVAS 1
#endif

#if !defined(ENABLE_OFFSCREEN_CANVAS_IN_WORKERS)
#define ENABLE_OFFSCREEN_CANVAS_IN_WORKERS 1
#endif

#if !defined(ENABLE_POINTER_LOCK)
#define ENABLE_POINTER_LOCK 1
#endif

#if !defined(ENABLE_TEXT_CARET)
#define ENABLE_TEXT_CARET 1
#endif

#if !defined(ENABLE_TEXT_SELECTION)
#define ENABLE_TEXT_SELECTION 1
#endif

#if !defined(ENABLE_XSLT)
#define ENABLE_XSLT 1
#endif

So why do we care about ENABLE_CSS_PAINTING_API only and ignore the reset? Or are we planning to remove all of these flags?

@donny-dont
Copy link
Contributor Author

What is the rule for removing an ENABLE() flag if it is unconditionally set to true? Currently we have all these flags set to true. This list includes ENABLE_MATHML and the ENABLE_OFFSCREEN_CANVAS.
....

So why do we care about ENABLE_CSS_PAINTING_API only and ignore the reset? Or are we planning to remove all of these flags?

@shallawa myself and @annevk have been going through the list and removing ENABLEs one by one.

Algorithmically it boils down to. Does the ENABLE flag require port specific code? Something like ENABLE_XSLT does not pass that because it requires an implementation of libxslt, while the CSS Painting API is port agnostic so it passes this check. Does the ENABLE flag have a runtime option? Yes for CSS Painting API. Since it passes those checks its up for removal.

@donny-dont donny-dont added the merge-queue Applied to send a pull request to merge-queue label Apr 16, 2024
https://bugs.webkit.org/show_bug.cgi?id=271913

Reviewed by Said Abou-Hallawa and Michael Catanzaro.

All ports are building with `ENABLE_CSS_PAINTING_API` as `ON`. Remove the
build option and use the runtime option for controlling use.

* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WTF/wtf/PlatformEnable.h:
* Source/WebCore/bindings/js/JSPaintRenderingContext2DCustom.cpp:
* Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:
* Source/WebCore/bindings/js/WebCoreJSClientData.cpp:
* Source/WebCore/bindings/js/WebCoreJSClientData.h:
* Source/WebCore/css/CSSPaintCallback.h:
* Source/WebCore/css/CSSPaintCallback.idl:
* Source/WebCore/css/CSSPaintImageValue.cpp:
* Source/WebCore/css/CSSPaintImageValue.h:
* Source/WebCore/css/CSSPaintSize.h:
* Source/WebCore/css/CSSPaintSize.idl:
* Source/WebCore/css/CSSValue.cpp:
* Source/WebCore/css/CSSValue.h:
* Source/WebCore/css/CSSValueKeywords.in:
* Source/WebCore/css/DOMCSSNamespace+CSSPainting.idl:
* Source/WebCore/css/DOMCSSPaintWorklet.cpp:
* Source/WebCore/css/DOMCSSPaintWorklet.h:
* Source/WebCore/css/parser/CSSParserContext.cpp:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
* Source/WebCore/css/typedom/MainThreadStylePropertyMapReadOnly.cpp:
* Source/WebCore/dom/Document.cpp:
* Source/WebCore/dom/Document.h:
* Source/WebCore/html/CustomPaintCanvas.cpp:
* Source/WebCore/html/CustomPaintCanvas.h:
* Source/WebCore/html/CustomPaintImage.cpp:
* Source/WebCore/html/CustomPaintImage.h:
* Source/WebCore/html/canvas/PaintRenderingContext2D.cpp:
* Source/WebCore/html/canvas/PaintRenderingContext2D.h:
* Source/WebCore/html/canvas/PaintRenderingContext2D.idl:
* Source/WebCore/rendering/style/RenderStyle.cpp:
* Source/WebCore/rendering/style/RenderStyle.h:
* Source/WebCore/rendering/style/StyleImage.h:
* Source/WebCore/rendering/style/StylePaintImage.cpp:
* Source/WebCore/rendering/style/StylePaintImage.h:
* Source/WebCore/style/StyleBuilder.cpp:
* Source/WebCore/style/StyleBuilderState.cpp:
* Source/WebCore/workers/WorkerOrWorkletScriptController.cpp:
* Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:
* Source/WebCore/worklets/PaintWorkletGlobalScope.h:
* Source/WebCore/worklets/PaintWorkletGlobalScope.idl:
* Source/WebCore/worklets/WorkletGlobalScope.h:
* Source/cmake/WebKitFeatures.cmake:
* Tools/Scripts/webkitperl/FeatureList.pm:

Canonical link: https://commits.webkit.org/277584@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Remove-ENABLE_CSS_PAINTING_API-build-flag branch from 515baac to 4d1720e Compare April 17, 2024 00:14
@webkit-commit-queue
Copy link
Collaborator

Committed 277584@main (4d1720e): https://commits.webkit.org/277584@main

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

@webkit-commit-queue webkit-commit-queue merged commit 4d1720e into WebKit:main Apr 17, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 17, 2024
@annevk
Copy link
Contributor

annevk commented Apr 17, 2024

PlatformEnable.h is unfortunately not sufficient to determine if a flag is always true for all ports. You also have to look at the CMake code. And yeah, generally the less branches we have to (pretend to) maintain the better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases
Projects
None yet
6 participants