Bug 192828 - Fix CSS Variable cycle detection and import CSS Variables web platform tests
Summary: Fix CSS Variable cycle detection and import CSS Variables web platform tests
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords:
Depends on: 192962
Blocks: 192334 193185
  Show dependency treegraph
 
Reported: 2018-12-18 13:51 PST by Justin Michaud
Modified: 2019-01-07 10:33 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
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.