RESOLVED CONFIGURATION CHANGED 192828
Fix CSS Variable cycle detection and import CSS Variables web platform tests 
https://bugs.webkit.org/show_bug.cgi?id=192828
Summary Fix CSS Variable cycle detection and import CSS Variables web platform tests
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
DetailsFormatted DiffDiff
Patch (244.66 KB, patch)
2018-12-18 16:14 PST, Justin Michaud 		no flags
DetailsFormatted DiffDiff
Patch (245.18 KB, patch)
2018-12-18 16:53 PST, Justin Michaud 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Patch (267.00 KB, patch)
2018-12-21 14:00 PST, Justin Michaud 		koivisto: review+
DetailsFormatted DiffDiff
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)
EWS Watchlist
Comment 6 2018-12-18 22:12:13 PST Comment hidden (obsolete)
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)
EWS Watchlist
Comment 9 2018-12-19 22:32:56 PST Comment hidden (obsolete)
EWS Watchlist
Comment 10 2018-12-19 23:21:45 PST Comment hidden (obsolete)
EWS Watchlist
Comment 11 2018-12-19 23:21:46 PST Comment hidden (obsolete)
EWS Watchlist
Comment 12 2018-12-19 23:24:00 PST Comment hidden (obsolete)
EWS Watchlist
Comment 13 2018-12-19 23:24:02 PST Comment hidden (obsolete)
EWS Watchlist
Comment 14 2018-12-19 23:33:32 PST Comment hidden (obsolete)
EWS Watchlist
Comment 15 2018-12-19 23:33:44 PST Comment hidden (obsolete)
EWS Watchlist
Comment 16 2018-12-20 00:14:28 PST Comment hidden (obsolete)
EWS Watchlist
Comment 17 2018-12-20 00:14:30 PST Comment hidden (obsolete)
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.