Bug 192828

Summary:

Fix CSS Variable cycle detection and import CSS Variables web platform tests

Product:

WebKit

Reporter:

Justin Michaud <justin_michaud>

Component:

Layout and Rendering

Assignee:

Justin Michaud <justin_michaud>

Status:

RESOLVED CONFIGURATION CHANGED

Severity:

Normal

CC:

bfulgham, dino, ews-watchlist, koivisto, rniwa, simon.fraser, zalan

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

192962

Bug Blocks:

192334, 193185

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews106 for mac-sierra-wk2	none
Patch	none
Archive of layout-test-results from ews102 for mac-sierra	none
Archive of layout-test-results from ews121 for ios-simulator-wk2	none
Archive of layout-test-results from ews115 for mac-sierra	none
Archive of layout-test-results from ews206 for win-future	none
Archive of layout-test-results from ews107 for mac-sierra-wk2	none
Patch	none
Patch	koivisto: review+

Justin Michaud

Reported 2018-12-18 13:51:12 PST

Import CSS Variables web platform tests

Attachments
Patch (243.62 KB, patch) 2018-12-18 14:01 PST, Justin Michaud	no flags	Details Formatted Diff Diff
Patch (244.66 KB, patch) 2018-12-18 16:14 PST, Justin Michaud	no flags	Details Formatted Diff Diff
Patch (245.18 KB, patch) 2018-12-18 16:53 PST, Justin Michaud	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.01 MB, application/zip) 2018-12-18 22:12 PST, EWS Watchlist	no flags	Details
Patch (258.85 KB, patch) 2018-12-19 21:22 PST, Justin Michaud	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-sierra (2.71 MB, application/zip) 2018-12-19 22:32 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.99 MB, application/zip) 2018-12-19 23:21 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews115 for mac-sierra (2.30 MB, application/zip) 2018-12-19 23:24 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews206 for win-future (13.05 MB, application/zip) 2018-12-19 23:33 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.64 MB, application/zip) 2018-12-20 00:14 PST, EWS Watchlist	no flags	Details
Patch (267.90 KB, patch) 2018-12-20 19:51 PST, Justin Michaud	no flags	Details Formatted Diff Diff
Patch (267.00 KB, patch) 2018-12-21 14:00 PST, Justin Michaud	koivisto: review+	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Justin Michaud

Comment 1 2018-12-18 14:01:38 PST

Created attachment 357609 [details] Patch

Simon Fraser (smfr)

Comment 2 2018-12-18 15:42:48 PST

Comment on attachment 357609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357609&action=review > Source/WebCore/rendering/RenderBoxModelObject.cpp:936 > + bool hasZeroSize = bgLayer.size().type == FillSizeType::Size && bgLayer.size().size.width.isZero() && bgLayer.size().size.height.isZero(); Please implement isEmpty() for FillSize like IntSize: bool isEmpty() const { return m_width <= 0 || m_height <= 0; } and then use it.

Justin Michaud

Comment 3 2018-12-18 16:14:34 PST

Created attachment 357629 [details] Patch

Justin Michaud

Comment 4 2018-12-18 16:53:05 PST

Created attachment 357635 [details] Patch

EWS Watchlist

Comment 5 2018-12-18 22:12:11 PST Comment hidden (obsolete)

Comment on attachment 357635 [details] Patch Attachment 357635 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10468647 New failing tests: imported/w3c/web-platform-tests/css/css-variables/variable-substitution-variable-declaration.html

EWS Watchlist

Comment 6 2018-12-18 22:12:13 PST Comment hidden (obsolete)

Created attachment 357655 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Justin Michaud

Comment 7 2018-12-19 21:22:25 PST

Created attachment 357775 [details] Patch

EWS Watchlist

Comment 8 2018-12-19 22:32:54 PST Comment hidden (obsolete)

Comment on attachment 357775 [details] Patch Attachment 357775 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10486537 New failing tests: fast/css/variables/test-suite/173.html fast/css/variables/test-suite/149.html fast/css/variables/all-keyword-unset.html fast/css/variables/test-suite/079.html fast/css/variables/test-suite/128.html fast/css/variables/test-suite/110.html fast/css/variables/test-suite/174.html fast/css/variables/test-suite/121.html fast/css/variables/test-suite/080.html fast/css/variables/all-keyword-revert.html fast/css/variables/test-suite/133.html fast/css/variables/test-suite/107.html

EWS Watchlist

Comment 9 2018-12-19 22:32:56 PST Comment hidden (obsolete)

Created attachment 357787 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 10 2018-12-19 23:21:45 PST Comment hidden (obsolete)

Comment on attachment 357775 [details] Patch Attachment 357775 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10486611 New failing tests: fast/css/variables/test-suite/173.html fast/css/variables/test-suite/149.html fast/css/variables/all-keyword-unset.html fast/css/variables/test-suite/079.html fast/css/variables/test-suite/128.html fast/css/variables/test-suite/110.html fast/css/variables/test-suite/174.html fast/css/variables/test-suite/121.html fast/css/variables/test-suite/080.html fast/css/variables/all-keyword-revert.html fast/css/variables/test-suite/133.html fast/css/variables/test-suite/107.html

EWS Watchlist

Comment 11 2018-12-19 23:21:46 PST Comment hidden (obsolete)

Created attachment 357791 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

EWS Watchlist

Comment 12 2018-12-19 23:24:00 PST Comment hidden (obsolete)

Comment on attachment 357775 [details] Patch Attachment 357775 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10486617 New failing tests: fast/css/variables/test-suite/173.html fast/css/variables/test-suite/149.html fast/css/variables/all-keyword-unset.html fast/css/variables/test-suite/079.html fast/css/variables/test-suite/128.html fast/css/variables/test-suite/110.html fast/css/variables/test-suite/174.html fast/css/variables/test-suite/121.html fast/css/variables/test-suite/080.html fast/css/variables/all-keyword-revert.html fast/css/variables/test-suite/133.html fast/css/variables/test-suite/107.html

EWS Watchlist

Comment 13 2018-12-19 23:24:02 PST Comment hidden (obsolete)

Created attachment 357792 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 14 2018-12-19 23:33:32 PST Comment hidden (obsolete)

Comment on attachment 357775 [details] Patch Attachment 357775 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/10486950 New failing tests: fast/css/variables/test-suite/173.html fast/css/variables/test-suite/149.html fast/css/variables/all-keyword-unset.html fast/css/variables/test-suite/079.html fast/css/variables/test-suite/128.html fast/css/variables/test-suite/110.html fast/css/variables/test-suite/174.html fast/css/variables/test-suite/121.html fast/css/variables/test-suite/080.html fast/css/variables/all-keyword-revert.html fast/css/variables/test-suite/133.html fast/css/variables/test-suite/107.html

EWS Watchlist

Comment 15 2018-12-19 23:33:44 PST Comment hidden (obsolete)

Created attachment 357794 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

EWS Watchlist

Comment 16 2018-12-20 00:14:28 PST Comment hidden (obsolete)

Comment on attachment 357775 [details] Patch Attachment 357775 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10487603 New failing tests: fast/css/variables/test-suite/173.html fast/css/variables/test-suite/149.html fast/css/variables/all-keyword-unset.html fast/css/variables/test-suite/079.html fast/css/variables/test-suite/128.html fast/css/variables/test-suite/110.html fast/css/variables/test-suite/174.html fast/css/variables/test-suite/121.html fast/css/variables/test-suite/080.html fast/css/variables/all-keyword-revert.html fast/css/variables/test-suite/133.html fast/css/variables/test-suite/107.html

EWS Watchlist

Comment 17 2018-12-20 00:14:30 PST Comment hidden (obsolete)

Created attachment 357797 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Justin Michaud

Comment 18 2018-12-20 19:51:30 PST

Created attachment 357920 [details] Patch

Justin Michaud

Comment 19 2018-12-20 20:30:35 PST

The rendering changes are in <https://bugs.webkit.org/show_bug.cgi?id=192962> and will be removed from this patch once it lands.

Justin Michaud

Comment 20 2018-12-21 14:00:29 PST

Created attachment 357977 [details] Patch

Antti Koivisto

Comment 21 2019-01-07 10:30:53 PST

Comment on attachment 357977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357977&action=review r=me, but I think this could be done in a nicer way. > Source/WebCore/css/CSSVariableReferenceValue.cpp:49 > +static bool resolveTokenRange(CSSParserTokenRange, Vector<CSSParserToken>&, ApplyCascadedPropertyState&, bool&); > > -static bool resolveVariableFallback(CSSParserTokenRange range, Vector<CSSParserToken>& result, ApplyCascadedPropertyState& state) > +static bool resolveVariableFallback(CSSParserTokenRange range, Vector<CSSParserToken>& result, ApplyCascadedPropertyState& state, bool& substitutionValid) Do all combinations of the return bool and substitutionValid return argument make sense? If not, maybe you just want to return an 3-value enum instead? Or you could return a struct with both bits and avoid the return argument that way. > Source/WebCore/css/CSSVariableReferenceValue.cpp:86 > + bool fallbacksubstitutionValid = true; > + if (resolveVariableFallback(CSSParserTokenRange(range), fallbackResult, state, fallbacksubstitutionValid)) { How do I know fallbacksubstitutionValid bit needs to be set to true ? This sort of code should not require specific initialization by the client. Also fallbacksubstitutionValid is unused here so requiring it is annoying. Not using return arguments in the first place would avoid both problems. There pattern repeats in several places in this patch.

Antti Koivisto

Comment 22 2019-01-07 10:33:22 PST

Comment on attachment 357977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357977&action=review > Source/WebCore/css/StyleResolver.cpp:2345 > + // This is an arbitrary limit, to make sure that we don't crash. > + if (referencedProperties.size() > 1000) You could have const auto arbitraryLimitToMakeSureWeDontCrash = 1000; and avoid the commentary.

Justin Michaud

Comment 23 2024-04-08 13:37:26 PDT

Closing old bugs assigned to me

Note You need to log in before you can comment on or make changes to this bug.