Bug 192828 – Fix CSS Variable cycle detection and import CSS Variables web platform tests

• Component: Layout and Rendering
• Status: RESOLVED CONFIGURATION CHANGED

[Justin Michaud]

Import CSS Variables web platform tests

[Justin Michaud]

Created attachment 357609 [details]
Patch

[Simon Fraser (smfr)]

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]

Created attachment 357629 [details]
Patch

[Justin Michaud]

Created attachment 357635 [details]
Patch

[EWS Watchlist]

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]

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]

Created attachment 357775 [details]
Patch

[EWS Watchlist]

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]

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 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]

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 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]

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 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]

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 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]

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]

Created attachment 357920 [details]
Patch

[Justin Michaud]

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]

Created attachment 357977 [details]
Patch

[Antti Koivisto]

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 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]

Closing old bugs assigned to me