Drop SAC types from the CSS engine API and matcher#20
Drop SAC types from the CSS engine API and matcher#20
Conversation
The CSS core engine had two stacked abstract classes with no concrete core implementation: AbstractCSSEngine (1,113 LOC) and a tiny CSSEngineImpl (95 LOC) that only added parser-factory wiring, property-handler registration helpers, and the boolean value converter. Inline the small subclass into the base, keep the name CSSEngineImpl since that is the type SWT engine subclasses and tests already reference, and make makeCSSParser concrete (it was abstract solely so CSSEngineImpl could supply its single implementation). The class remains abstract because the CSSEngine.reapply() method has no sensible default in the core bundle. Update ParserTestUtil's parseCssWithoutImports cast and rename AbstractCSSEngineTest to CSSEngineImplTest in the same pass to keep test class names aligned with production class names. The test's anonymous subclass no longer needs to override makeCSSParser since it is now concrete on the base. All bundles internal (x-friends only); no API surface changed.
Same housekeeping pattern as the previous core-engine merge: AbstractCSSSWTEngineImpl was an abstract layer with two abstract hooks (initializeCSSPropertyHandlers, initializeCSSElementProvider) whose only concrete subclass was CSSSWTEngineImpl. Inline the abstract layer into the concrete leaf, drop the two now-redundant template methods, and register the SWT element provider and the registry-driven property handler provider directly in the constructor. Bundle internal (x-friends only); no API surface changed. All 194 css.swt tests still pass.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly reworks the CSS engine by introducing a new internal selector AST and matcher, replacing the previous SAC-based implementation. The refactoring involves merging AbstractCSSEngine into CSSEngineImpl, removing deprecated serialization and helper classes, and updating various components to utilize the new selector logic. Key additions include a computeStyleRanges method in SourceViewer and enhancements to dark theme application in IDEApplication. Reviewers identified a potential stability issue in SourceViewer regarding document partitioner swapping and suggested several improvements, including fixing typos in the new ICSSPropertyHandler methods and optimizing performance in the CSS matching path by avoiding redundant string splitting and re-calculating specificity.
| originalDocumentPartitioner.disconnect(); | ||
| originalDocumentPartitioner.connect(document); | ||
| documentExt.setDocumentPartitioner(partitioning, originalDocumentPartitioner); |
There was a problem hiding this comment.
Disconnecting the partitioner from the viewer's active document and connecting it to an external document is risky. IDocumentPartitioner implementations often maintain state or trigger events that assume a stable document connection. If another part of the UI (e.g., a background thread or a re-entrant call) interacts with the original document while the partitioner is swapped, it could lead to inconsistent state or BadLocationException. It is safer to create a temporary partitioner instance for the external document or use a stateless scanner.
| * step (re-layout, redraw, batched commit, ...) override this method; | ||
| * the default is a no-op. | ||
| */ | ||
| default void onAllCSSPropertiesApplyed(Object element, CSSEngine engine) throws Exception { |
There was a problem hiding this comment.
The method name contains a typo: Applyed should be Applied. Since this is a new addition to the interface as part of the engine rework, it is better to use the correct spelling now.
| default void onAllCSSPropertiesApplyed(Object element, CSSEngine engine) throws Exception { | |
| default void onAllCSSPropertiesApplied(Object element, CSSEngine engine) throws Exception { |
| * also receives the pseudo class. Defaults to delegating to the | ||
| * pseudo-less form. | ||
| */ | ||
| default void onAllCSSPropertiesApplyed(Object element, CSSEngine engine, String pseudo) throws Exception { |
There was a problem hiding this comment.
The method name contains a typo: Applyed should be Applied.
| default void onAllCSSPropertiesApplyed(Object element, CSSEngine engine, String pseudo) throws Exception { | |
| default void onAllCSSPropertiesApplied(Object element, CSSEngine engine, String pseudo) throws Exception { |
| return false; | ||
| } | ||
| // CSS class attribute can be a whitespace-separated list of classes. | ||
| for (String token : elementClass.split("\\s+")) { |
There was a problem hiding this comment.
Using String.split("\\s+") inside the CSS matching loop is inefficient as it compiles a regex and allocates a new array on every match evaluation. Consider using a manual loop with Character.isWhitespace or a pre-compiled Pattern to improve performance in the engine's hot path.
| return false; | ||
| } | ||
| String value = inc.value(); | ||
| for (String token : actual.split("\\s+")) { |
There was a problem hiding this comment.
Using String.split("\\s+") here is inefficient for high-frequency CSS matching. A manual word-boundary check would be more performant.
| } | ||
|
|
||
| @Override | ||
| public int specificity() { |
There was a problem hiding this comment.
The specificity of a SelectorList is calculated on every call using a stream. Since records are immutable, it is more efficient to calculate the specificity once in the constructor and store it in a field.
ICSSPropertyHandler2 added a single capability on top of ICSSPropertyHandler: a callback the engine fires after every property in a style declaration has been applied. Java 21 default methods make the second interface redundant: move the two onAllCSSPropertiesApplyed defaults onto ICSSPropertyHandler so every handler picks up a no-op default and only handlers that need the final-step hook (border, font) override it. ICSSPropertyHandler2Delegate had no implementations in the platform tree at all, so its branch in CSSEngineImpl.applyStyleDeclaration was dead. Drop the interface and the dead branch in the same pass. CSSEngineImpl.applyStyleDeclaration now collects every applied ICSSPropertyHandler (de-duped, order-preserving) and fires the callback on each. The previous singleton-list cache stays, just keyed on ICSSPropertyHandler instead of ICSSPropertyHandler2. CSSPropertyBorderSWTHandler and CSSPropertyFontSWTHandler drop their explicit "implements ICSSPropertyHandler2" since they already extend ICSSPropertyHandler-compatible bases. All bundles internal (x-friends only); no API surface changed.
PropertyHelper provided a reflective bean-property reader using java.beans.Introspector. Nothing in the platform tree calls it; its only consumer was TestPropertyHelper, a unit test that exercised the helper itself. Delete the helper and the orphaned test in one pass.
The 3,205-line URI class in org.eclipse.e4.ui.css.swt.helpers was a copy-paste of org.eclipse.emf.common.util.URI added when CSS engine authors did not want a hard dependency on EMF. EMF is now a transitive dependency throughout the platform, so the duplication is pure cost. Two callers consume the helper (CSSPropertyTabRendererSWTHandler, CSSPropertyThemeElementDefinitionHandler); both use stable EMF URI methods (createURI, authority, segment, segmentCount, lastSegment) that work identically on the EMF class. Switch the imports to org.eclipse.emf.common.util.URI, add the matching Require-Bundle to css.swt MANIFEST.MF, and delete the vendored copy.
Adds a small engine-internal selector AST (sealed Selector interface plus records for type, class, id, attribute, pseudo, compound, and combinator forms) and a static SelectorMatcher that walks an Element through it, alongside the existing SAC-based selector tree. New code only; nothing else in the engine consumes it yet. This is the foundation for Phase 3 step 1 of the CSS engine rework: once the parser-output translator and the engine.matches signature follow, the 26 vendored Batik selector wrappers under impl/sac/* can go away. Specificity follows CSS 2.1; pseudo-class semantics preserve the static-pseudo-instance carve-out from CSSPseudoClassConditionImpl so cascade behaviour does not shift. Sixteen unit tests cover universal, type case-sensitivity, class, id, compound, descendant, child, attribute presence and word-match, pseudo-class via isPseudoInstanceOf, selector lists, and CSS 2.1 specificity arithmetic. The adjacent-sibling test pins that the matcher returns false (rather than throwing) on elements without sibling support, since TestElement's ElementAdapter base does not expose siblings. The new package is exported x-friends to org.eclipse.e4.ui.tests.css.core so the unit tests can reach it. Bundle remains at JavaSE-17, so the matcher uses instanceof pattern chains rather than switch patterns.
Adds a SAC->internal Selector translator at the parser-output boundary in CSSDocumentHandlerImpl.startSelector and switches CSSEngine.matches and parseSelectors to the internal Selectors AST. CSSEngineImpl.matches now delegates to SelectorMatcher; applyConditionalPseudoStyle walks the internal AST instead of the SAC tree. The parser is configured with Batik's stock DefaultSelectorFactory and DefaultConditionFactory, so 23 vendored impl/sac/* selector and condition wrappers fall away. The dead ExtendedDocumentCSS.queryConditionSelector / querySelector methods (and the SAC_*_CONDITION int constants behind them) go with them. Selectors.SelectorList grows getLength()/item(int) SAC-style accessors and every record overrides toString() to return text() for readable selector text in error messages and rule.getSelectorText(). CSSEngineTest and SelectorTest are rewritten on the internal AST. Phase 3 step 1 of the CSS engine rework. Net ~-2,300 LOC. Three impl/sac classes (CSSDocumentHandlerImpl, DocumentHandlerFactoryImpl, SACParserFactoryImpl) remain as parser plumbing for step 2.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the CSS engine's selector handling by replacing the W3C SAC-based selector implementation with an internal AST. It introduces new classes such as Selectors and SelectorMatcher to handle selector matching, and adds a SacTranslator to bridge the gap between the existing SAC parser and the new internal AST. Additionally, the pull request fixes a typo in the onAllCSSPropertiesApplied method name, updates dependencies in the MANIFEST.MF files, and removes the unused PropertyHelper class. I have no feedback to provide as there were no review comments.
1 participant
Phase 3 step 1 of the CSS engine rework. Adds a SAC->internal Selector translator at the parser-output boundary in CSSDocumentHandlerImpl.startSelector and switches CSSEngine.matches and parseSelectors to the internal Selectors AST. The parser is configured with Batik's stock DefaultSelectorFactory and DefaultConditionFactory, so 23 vendored impl/sac/* selector and condition wrappers fall away.
CSSEngineImpl.matches now delegates to SelectorMatcher; applyConditionalPseudoStyle walks the internal AST instead of the SAC tree. Dead ExtendedDocumentCSS.queryConditionSelector / querySelector and the SAC_*_CONDITION constants behind them go with the wrappers. Three impl/sac classes (CSSDocumentHandlerImpl, DocumentHandlerFactoryImpl, SACParserFactoryImpl) remain as parser plumbing for step 2.
Net ~-2,300 LOC; CSSEngineTest and SelectorTest are rewritten on the internal AST. Opened against vogella/master to exercise the build verification workflow; not intended for upstream merge.