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 RenderingAssignee: Justin Michaud <justin_michaud>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: bfulgham, dino, ews-watchlist, koivisto, rniwa, simon.fraser, zalan
Priority: P2    
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+

Description Justin Michaud 2018-12-18 13:51:12 PST 
Import CSS Variables web platform tests
Comment 1 Justin Michaud 2018-12-18 14:01:38 PST 
Created attachment 357609 [details]
Patch
Comment 2 Simon Fraser (smfr) 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.
Comment 3 Justin Michaud 2018-12-18 16:14:34 PST 
Created attachment 357629 [details]
Patch
Comment 4 Justin Michaud 2018-12-18 16:53:05 PST 
Created attachment 357635 [details]
Patch
Comment 5 EWS Watchlist 2018-12-18 22:12:11 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-12-18 22:12:13 PST Comment hidden (obsolete)
Comment 7 Justin Michaud 2018-12-19 21:22:25 PST 
Created attachment 357775 [details]
Patch
Comment 8 EWS Watchlist 2018-12-19 22:32:54 PST Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-12-19 22:32:56 PST Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-12-19 23:21:45 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-12-19 23:21:46 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-12-19 23:24:00 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 2018-12-19 23:24:02 PST Comment hidden (obsolete)
Comment 14 EWS Watchlist 2018-12-19 23:33:32 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-12-19 23:33:44 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-12-20 00:14:28 PST Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-12-20 00:14:30 PST Comment hidden (obsolete)
Comment 18 Justin Michaud 2018-12-20 19:51:30 PST 
Created attachment 357920 [details]
Patch
Comment 19 Justin Michaud 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.
Comment 20 Justin Michaud 2018-12-21 14:00:29 PST 
Created attachment 357977 [details]
Patch
Comment 21 Antti Koivisto 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.
Comment 22 Antti Koivisto 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.
Comment 23 Justin Michaud 2024-04-08 13:37:26 PDT 
Closing old bugs assigned to me