Bug 188697 - [css-logical] Flow relative margin, padding and border shorthands
Summary: [css-logical] Flow relative margin, padding and border shorthands
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oriol Brufau
URL:
Keywords: InRadar
Depends on: 189597
Blocks: 185977 189441 216178
  Show dependency treegraph
 
Reported: 2018-08-17 05:18 PDT by Oriol Brufau
Modified: 2020-09-04 10:51 PDT (History)
13 users (show)

See Also:


Attachments
Patch (201.06 KB, patch)
2018-08-22 10:17 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.65 MB, application/zip)
2018-08-22 12:27 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.46 MB, application/zip)
2018-08-22 12:30 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews103 for mac-sierra (2.83 MB, application/zip)
2018-08-22 12:32 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.52 MB, application/zip)
2018-08-22 12:34 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.32 MB, application/zip)
2018-08-22 14:31 PDT, EWS Watchlist
no flags Details
Patch (200.98 KB, patch)
2018-08-22 15:40 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.36 MB, application/zip)
2018-08-22 17:06 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.92 MB, application/zip)
2018-08-22 17:39 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.09 MB, application/zip)
2018-08-22 17:41 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.32 MB, application/zip)
2018-08-22 17:52 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.64 MB, application/zip)
2018-08-22 19:52 PDT, EWS Watchlist
no flags Details
Patch (201.09 KB, patch)
2018-08-23 16:25 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (201.49 KB, patch)
2018-08-23 16:59 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (203.40 KB, patch)
2018-08-24 06:07 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (211.41 KB, patch)
2018-08-24 07:20 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.37 MB, application/zip)
2018-08-24 08:49 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.90 MB, application/zip)
2018-08-24 08:51 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.13 MB, application/zip)
2018-08-24 09:22 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.31 MB, application/zip)
2018-08-24 09:28 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.42 MB, application/zip)
2018-08-24 11:27 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.10 MB, application/zip)
2018-08-24 11:34 PDT, EWS Watchlist
no flags Details
Patch (215.95 KB, patch)
2018-08-26 12:56 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (215.90 KB, patch)
2018-08-28 14:15 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (215.62 KB, patch)
2018-08-28 15:36 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (215.54 KB, patch)
2018-09-10 15:16 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.30 MB, application/zip)
2018-09-10 16:55 PDT, EWS Watchlist
no flags Details
Patch (216.79 KB, patch)
2018-09-12 17:21 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (222.98 KB, patch)
2018-09-21 08:23 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (223.01 KB, patch)
2018-09-21 08:45 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (224.36 KB, patch)
2018-09-25 10:47 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (215.11 KB, patch)
2018-10-17 12:19 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (214.92 KB, patch)
2018-10-17 13:18 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.76 MB, application/zip)
2018-10-17 16:02 PDT, EWS Watchlist
no flags Details
Patch (245.64 KB, patch)
2018-10-17 16:30 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.43 MB, application/zip)
2018-10-17 17:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (4.50 MB, application/zip)
2018-10-17 18:12 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.65 MB, application/zip)
2018-10-17 18:50 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews204 for win-future (12.72 MB, application/zip)
2018-10-17 19:28 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.35 MB, application/zip)
2018-10-17 19:42 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.47 MB, application/zip)
2018-10-17 21:39 PDT, EWS Watchlist
no flags Details
Patch (310.67 KB, patch)
2018-10-18 12:27 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (310.70 KB, patch)
2018-10-18 12:56 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.40 MB, application/zip)
2018-10-18 14:07 PDT, EWS Watchlist
no flags Details
Patch (313.48 KB, patch)
2018-10-18 17:00 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (314.24 KB, patch)
2018-10-19 07:56 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.62 MB, application/zip)
2018-10-19 08:58 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.23 MB, application/zip)
2018-10-19 10:12 PDT, EWS Watchlist
no flags Details
Patch (317.58 KB, patch)
2018-10-31 14:19 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.62 MB, application/zip)
2018-10-31 15:23 PDT, EWS Watchlist
no flags Details
Patch (317.42 KB, patch)
2018-10-31 15:26 PDT, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (317.46 KB, patch)
2018-11-15 07:58 PST, Oriol Brufau
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.38 MB, application/zip)
2018-11-15 08:56 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.98 MB, application/zip)
2018-11-15 09:20 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews204 for win-future (12.83 MB, application/zip)
2018-11-15 09:52 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.45 MB, application/zip)
2018-11-15 09:52 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (3.04 MB, application/zip)
2018-11-15 10:41 PST, EWS Watchlist
no flags Details
Patch (317.21 KB, patch)
2018-11-15 11:40 PST, Oriol Brufau
no flags Details | Formatted Diff | Diff
Patch (188.28 KB, patch)
2019-01-21 10:34 PST, Oriol Brufau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oriol Brufau 2018-08-17 05:18:55 PDT
Spec: https://drafts.csswg.org/css-logical-1/#box

Implement
 * margin-{block,inline}
 * padding-{block,inline}
 * border-{block,inline}
 * border-{block,inline}-{width,style,color}
 * inset-{block,inline}-{start,end}
 * inset-{block,inline}
 * inset

They will be implemented behind a flag, like has happened in Blink.
Firefox has shipped the inset longhands.
Comment 1 Oriol Brufau 2018-08-20 05:36:44 PDT
Bug 188711 is causing problems for logical inset properties, so I will add them later in another bug.

This one will add:
 * margin-{block,inline}
 * padding-{block,inline}
 * border-{block,inline}
 * border-{block,inline}-{width,style,color}
Comment 2 Oriol Brufau 2018-08-22 10:17:12 PDT Comment hidden (obsolete)
Comment 3 EWS Watchlist 2018-08-22 12:27:25 PDT Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-08-22 12:27:26 PDT Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-08-22 12:30:27 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-08-22 12:30:29 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-08-22 12:32:33 PDT Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-08-22 12:32:35 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-08-22 12:34:54 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-08-22 12:34:55 PDT Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-08-22 14:31:28 PDT Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-08-22 14:31:30 PDT Comment hidden (obsolete)
Comment 13 Oriol Brufau 2018-08-22 15:40:36 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2018-08-22 17:06:00 PDT Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-08-22 17:06:02 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-08-22 17:39:15 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-08-22 17:39:17 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2018-08-22 17:41:27 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2018-08-22 17:41:29 PDT Comment hidden (obsolete)
Comment 20 EWS Watchlist 2018-08-22 17:52:56 PDT Comment hidden (obsolete)
Comment 21 EWS Watchlist 2018-08-22 17:52:58 PDT Comment hidden (obsolete)
Comment 22 EWS Watchlist 2018-08-22 19:52:54 PDT Comment hidden (obsolete)
Comment 23 EWS Watchlist 2018-08-22 19:52:56 PDT Comment hidden (obsolete)
Comment 24 Oriol Brufau 2018-08-23 16:25:13 PDT Comment hidden (obsolete)
Comment 25 Oriol Brufau 2018-08-23 16:59:56 PDT Comment hidden (obsolete)
Comment 26 Michael Catanzaro 2018-08-24 05:43:06 PDT
Comment on attachment 347973 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347973&action=review

> Source/cmake/WebKitFeatures.cmake:103
> +    WEBKIT_OPTION_DEFINE(ENABLE_CSS_LOGICAL "Toggle CSS Logical Properties and Values support" PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES})

Problem is that ENABLE_EXPERIMENTAL_FEATURES isn't defined until the bottom of this file, so you can't use it here.

I don't think there's any particular reason for that -- we *could* choose to change it -- but until now we have not enabled any experimental build flags across all ports, only for particular ports. E.g. if you want it enabled for GTK, you would do that in OptionsGTK.cmake. If you don't like this, you could try moving the definition of ENABLE_EXPERIMENTAL_FEATURES up to the top of this file and see if that works.

Mostly we  use runtime flags for experimental features, which is why this hasn't been an issue until now.
Comment 27 Oriol Brufau 2018-08-24 06:07:21 PDT Comment hidden (obsolete)
Comment 28 Oriol Brufau 2018-08-24 07:20:38 PDT Comment hidden (obsolete)
Comment 29 EWS Watchlist 2018-08-24 08:49:42 PDT Comment hidden (obsolete)
Comment 30 EWS Watchlist 2018-08-24 08:49:44 PDT Comment hidden (obsolete)
Comment 31 EWS Watchlist 2018-08-24 08:51:42 PDT Comment hidden (obsolete)
Comment 32 EWS Watchlist 2018-08-24 08:51:44 PDT Comment hidden (obsolete)
Comment 33 EWS Watchlist 2018-08-24 09:22:17 PDT Comment hidden (obsolete)
Comment 34 EWS Watchlist 2018-08-24 09:22:19 PDT Comment hidden (obsolete)
Comment 35 EWS Watchlist 2018-08-24 09:28:22 PDT Comment hidden (obsolete)
Comment 36 EWS Watchlist 2018-08-24 09:28:24 PDT Comment hidden (obsolete)
Comment 37 EWS Watchlist 2018-08-24 11:27:50 PDT Comment hidden (obsolete)
Comment 38 EWS Watchlist 2018-08-24 11:27:52 PDT Comment hidden (obsolete)
Comment 39 EWS Watchlist 2018-08-24 11:34:44 PDT Comment hidden (obsolete)
Comment 40 EWS Watchlist 2018-08-24 11:34:46 PDT Comment hidden (obsolete)
Comment 41 Oriol Brufau 2018-08-26 12:56:00 PDT Comment hidden (obsolete)
Comment 42 Javier Fernandez 2018-08-27 06:45:39 PDT
Comment on attachment 348100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348100&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4232
> +    bool showEnd = !compareCSSValuePtr(startValue, endValue);

Do we really need this variable ?

> Source/WebCore/css/StyleProperties.cpp:423
> +    if (start.isImportant() != end.isImportant())

Shouldn't we move this check before the one we do for 'initial' ? Isn't having the same value of 'importance' a requirement for a valid parsing ?

> Source/WebCore/css/StyleProperties.cpp:700
> +String StyleProperties::borderPropertyValue(

I think this function has an incorrect indentation; it should be declared in a single line.

> Source/WebCore/css/StyleProperties.h:167
> +    String borderPropertyValue(

Please, declare this function in a single line.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5685
> +        return m_range.atEnd();

Shouldn't consumeBorder already do the check fr m_range.atEnd ? In any case, we could move it above, before adding the properties' value and just finishing the logic with return true. 

It may be a good idea to have ASSERTS here for ensuring we got non-null pointers from consumeBorder if such function returns true. Although not mandatory, since releaseNonNull should avoid referencing null pointers already, it'd would hep us to catch mistakes in the consumeBorder funcition's  parsing logic.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5710
> +        return m_range.atEnd();

Ditto

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5781
> +        return m_range.atEnd();

Ditto
Comment 43 Oriol Brufau 2018-08-27 09:02:27 PDT
Comment on attachment 348100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348100&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4232
>> +    bool showEnd = !compareCSSValuePtr(startValue, endValue);
> 
> Do we really need this variable ?

I can remove it, but this way is more consistent with getCSSPropertyValuesFor4SidesShorthand below and with Blink
(https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/style_property_serializer.cc?rcl=da886a9725b3b5f44903354f3cf01a1c7ccb1a0c&l=813)

>> Source/WebCore/css/StyleProperties.cpp:423
>> +    if (start.isImportant() != end.isImportant())
> 
> Shouldn't we move this check before the one we do for 'initial' ? Isn't having the same value of 'importance' a requirement for a valid parsing ?

You are right, my code is based on get4Values which has this bug.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5685
>> +        return m_range.atEnd();
> 
> Shouldn't consumeBorder already do the check fr m_range.atEnd ? In any case, we could move it above, before adding the properties' value and just finishing the logic with return true. 
> 
> It may be a good idea to have ASSERTS here for ensuring we got non-null pointers from consumeBorder if such function returns true. Although not mandatory, since releaseNonNull should avoid referencing null pointers already, it'd would hep us to catch mistakes in the consumeBorder funcition's  parsing logic.

Not sure. It seems that currently WebKit doesn't check m_range.atEnd() in consumeBorder. Blink checks it after AddExpandedPropertyForValue.
If I understand correctly, this would mean that border longhands are set before checking that there is no extra value at the end, but I can't reproduce this.
WebKit also checks m_range.atEnd() after addProperty in consume4Values.
So not sure if I'm misunderstanding something or if there is no difference in behavior.
Comment 44 Oriol Brufau 2018-08-28 13:55:01 PDT
Comment on attachment 348100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348100&action=review

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5685
>> +        return m_range.atEnd();
> 
> It may be a good idea to have ASSERTS here for ensuring we got non-null pointers from consumeBorder if such function returns true. Although not mandatory, since releaseNonNull should avoid referencing null pointers already, it'd would hep us to catch mistakes in the consumeBorder funcition's  parsing logic.

consumeBorder already returns false if some pointer is null, so adding ASSERTS after every call doesn't seem useful to me.
Comment 45 Oriol Brufau 2018-08-28 14:15:35 PDT Comment hidden (obsolete)
Comment 46 Javier Fernandez 2018-08-28 14:35:57 PDT
Comment on attachment 348100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348100&action=review

>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4232
>>> +    bool showEnd = !compareCSSValuePtr(startValue, endValue);
>> 
>> Do we really need this variable ?
> 
> I can remove it, but this way is more consistent with getCSSPropertyValuesFor4SidesShorthand below and with Blink
> (https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/style_property_serializer.cc?rcl=da886a9725b3b5f44903354f3cf01a1c7ccb1a0c&l=813)

Ok, it's not a big deal, so lets keep it.

>>>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5685
>>>> +        return m_range.atEnd();
>>> 
>>> Shouldn't consumeBorder already do the check fr m_range.atEnd ? In any case, we could move it above, before adding the properties' value and just finishing the logic with return true. 
>>> 
>>> It may be a good idea to have ASSERTS here for ensuring we got non-null pointers from consumeBorder if such function returns true. Although not mandatory, since releaseNonNull should avoid referencing null pointers already, it'd would hep us to catch mistakes in the consumeBorder funcition's  parsing logic.
>> 
>> Not sure. It seems that currently WebKit doesn't check m_range.atEnd() in consumeBorder. Blink checks it after AddExpandedPropertyForValue.
>> If I understand correctly, this would mean that border longhands are set before checking that there is no extra value at the end, but I can't reproduce this.
>> WebKit also checks m_range.atEnd() after addProperty in consume4Values.
>> So not sure if I'm misunderstanding something or if there is no difference in behavior.
> 
> consumeBorder already returns false if some pointer is null, so adding ASSERTS after every call doesn't seem useful to me.

Yes, I agree the asserts are not necessary. 

Additionally, since the parseShorthand will return false in case of !m_range.AtEnd(), the added values will be discarded, so doing the check before doesn't change the behavior. It seems there is not much consistent on this kind of logic; CSSProperyOverflow, CSSPropertyMarker or CSSPropertyBackgroundPosition are some examples of shorthand doing the check before adding the parsed values. 

Anyway, since there is no behavior change I think it's not a problem to keep the patch as it is. My comment was just a suggestion, looking at what other shorthand do. However, keeping consistence with the Blink implementation may be a good idea as well.
Comment 47 Oriol Brufau 2018-08-28 15:36:15 PDT Comment hidden (obsolete)
Comment 48 Oriol Brufau 2018-09-10 15:16:46 PDT Comment hidden (obsolete)
Comment 49 EWS Watchlist 2018-09-10 16:55:03 PDT Comment hidden (obsolete)
Comment 50 EWS Watchlist 2018-09-10 16:55:06 PDT Comment hidden (obsolete)
Comment 51 Manuel Rego Casasnovas 2018-09-11 03:02:04 PDT
Comment on attachment 349335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349335&action=review

It'd be nice to be more verbose on the ChangeLog files documenting the changes on the different files to ease the review process.
I don't have knowledge to review the changes related to the compilation flag, specially on the Mac side, I hope someone else can take a look to that.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3884
> +            if (!compareCSSValuePtr<CSSValue>(value, propertyValue(CSSPropertyBorderBlockEnd, DoNotUpdateLayout)))
> +                return nullptr;

Does this ever happens? Wouldn't be enough with an ASSERT() here?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:-4936
> -    addExpandedPropertyForValue(CSSPropertyBorderImage, CSSValuePool::singleton().createImplicitInitialValue(), important);

Why are you removing this line? Is it right?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4942
> +    if (!end)

Nit: I'd use "if (endImplicit)" here.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:5683
> +        addExpandedPropertyForValue(CSSPropertyBorderInlineColor, color.releaseNonNull(), important);

What happens wiht CSSPropertyBorderImage?

> Source/WebCore/css/parser/CSSPropertyParser.h:65
> +    bool consumeBorder(RefPtr<CSSValue>&, RefPtr<CSSValue>&, RefPtr<CSSValue>&);

Here the names of the arguments are important.

> Source/cmake/WebKitFeatures.cmake:178
> +macro(_WEBKIT_OPTION_ENFORCE_DEPENDS _name)
> +    foreach (_dependency ${_WEBKIT_AVAILABLE_OPTIONS_${_name}_DEPENDENCIES})
> +        if (NOT ${_dependency})
> +            message(STATUS "Disabling ${_name} since ${_dependency} is disabled.")
> +            set(${_name} OFF)
> +            set(_OPTION_CHANGED TRUE)
> +            break ()
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +macro(_WEBKIT_OPTION_ENFORCE_ALL_DEPENDS)
> +    set(_OPTION_CHANGED TRUE)
> +    while (${_OPTION_CHANGED})
> +        set(_OPTION_CHANGED FALSE)
> +        foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +            if (${_name})
> +                _WEBKIT_OPTION_ENFORCE_DEPENDS(${_name})
> +            endif ()
> +        endforeach ()
> +    endwhile ()
> +endmacro()
> +
> +macro(_WEBKIT_OPTION_ENFORCE_CONFLICTS _name)
> +    foreach (_conflict ${_WEBKIT_AVAILABLE_OPTIONS_${_name}_CONFLICTS})
> +        if (${_conflict})
> +            message(FATAL_ERROR "${_name} conflicts with ${_conflict}. You must disable one or the other.")
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +macro(_WEBKIT_OPTION_ENFORCE_ALL_CONFLICTS)
> +    foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +        if (${_name})
> +            _WEBKIT_OPTION_ENFORCE_CONFLICTS(${_name})
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +macro(WEBKIT_OPTION_END)
> +    set(_SETTING_WEBKIT_OPTIONS FALSE)
> +
> +    list(SORT _WEBKIT_AVAILABLE_OPTIONS)
> +    set(_MAX_FEATURE_LENGTH 0)
> +    foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +        string(LENGTH ${_name} _name_length)
> +        if (_name_length GREATER _MAX_FEATURE_LENGTH)
> +            set(_MAX_FEATURE_LENGTH ${_name_length})
> +        endif ()
> +
> +        option(${_name} "${_WEBKIT_AVAILABLE_OPTIONS_DESCRIPTION_${_name}}" ${_WEBKIT_AVAILABLE_OPTIONS_INITIAL_VALUE_${_name}})
> +        if (NOT ${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
> +            mark_as_advanced(FORCE ${_name})
> +        endif ()
> +    endforeach ()
> +
> +    # Run through every possible depends to make sure we have disabled anything
> +    # that could cause an unnecessary conflict before processing conflicts.
> +    _WEBKIT_OPTION_ENFORCE_ALL_DEPENDS()
> +    _WEBKIT_OPTION_ENFORCE_ALL_CONFLICTS()
> +
> +    foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +        if (${_name})
> +            list(APPEND FEATURE_DEFINES ${_name})
> +            set(FEATURE_DEFINES_WITH_SPACE_SEPARATOR "${FEATURE_DEFINES_WITH_SPACE_SEPARATOR} ${_name}")
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +macro(PRINT_WEBKIT_OPTIONS)
> +    message(STATUS "Enabled features:")
> +
> +    set(_should_print_dots ON)
> +    foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
> +        if (${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}})
> +            string(LENGTH ${_name} _name_length)
> +            set(_message " ${_name} ")
> +
> +            # Print dots on every other row, for readability.
> +            foreach (IGNORE RANGE ${_name_length} ${_MAX_FEATURE_LENGTH})
> +                if (${_should_print_dots})
> +                    set(_message "${_message}.")
> +                else ()
> +                    set(_message "${_message} ")
> +                endif ()
> +            endforeach ()
> +
> +            set(_should_print_dots (NOT ${_should_print_dots}))
> +
> +            set(_message "${_message} ${${_name}}")
> +            message(STATUS "${_message}")
> +        endif ()
> +    endforeach ()
> +endmacro()
> +
> +set(_WEBKIT_CONFIG_FILE_VARIABLES "")
> +
> +macro(EXPOSE_VARIABLE_TO_BUILD _variable_name)
> +    list(APPEND _WEBKIT_CONFIG_FILE_VARIABLES ${_variable_name})
> +endmacro()
> +
> +macro(SET_AND_EXPOSE_TO_BUILD _variable_name)
> +    # It's important to handle the case where the value isn't passed, because often
> +    # during configuration an empty variable is the result of a failed package search.
> +    if (${ARGC} GREATER 1)
> +        set(_variable_value ${ARGV1})
> +    else ()
> +        set(_variable_value OFF)
> +    endif ()
> +
> +    set(${_variable_name} ${_variable_value})
> +    EXPOSE_VARIABLE_TO_BUILD(${_variable_name})
> +endmacro()
> +
> +option(ENABLE_EXPERIMENTAL_FEATURES "Enable experimental features" OFF)
> +SET_AND_EXPOSE_TO_BUILD(ENABLE_EXPERIMENTAL_FEATURES ${ENABLE_EXPERIMENTAL_FEATURES})
> +

Why are you moving all these lines, is this needed for this change?

> LayoutTests/imported/w3c/ChangeLog:14
> +        The tests still have some failures because sideways writing modes have
> +        not been implemented yet.

Is there any bug about this? Otherwise it'd be nice to report it and link it from here for future reference.

> LayoutTests/platform/mac/TestExpectations:1773
> +# These tests need the ENABLE_CSS_LOGICAL compile flag.
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-style.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-margin.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-color.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-shorthands.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-padding.html  [ Skip ]
> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html  [ Skip ]

So the tests are skip in Mac? Why? What happens in other platforms?
Comment 52 Oriol Brufau 2018-09-11 11:44:43 PDT
Comment on attachment 349335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349335&action=review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3884
>> +                return nullptr;
> 
> Does this ever happens? Wouldn't be enough with an ASSERT() here?

Yes, it happens e.g. if you have `style.cssText = "border-block-start: 1px solid blue; border-block-end: 2px dotted green"`.
Then `style.borderBlock` can't be serialized.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:-4936
>> -    addExpandedPropertyForValue(CSSPropertyBorderImage, CSSValuePool::singleton().createImplicitInitialValue(), important);
> 
> Why are you removing this line? Is it right?

I'm moving it into the `case CSSPropertyBorder`.
Yes, it's right because border-image should not be reset by border-block nor border-inline.
See https://github.com/w3c/csswg-drafts/issues/2874
I did the same for Blink.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:4942
>> +    if (!end)
> 
> Nit: I'd use "if (endImplicit)" here.

OK

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:5683
>> +        addExpandedPropertyForValue(CSSPropertyBorderInlineColor, color.releaseNonNull(), important);
> 
> What happens wiht CSSPropertyBorderImage?

Shouldn't be reset by border-inline, https://github.com/w3c/csswg-drafts/issues/2874

>> Source/cmake/WebKitFeatures.cmake:178
>> +
> 
> Why are you moving all these lines, is this needed for this change?

Because ENABLE_EXPERIMENTAL_FEATURES was set at the bottom.
In order to set ENABLE_CSS_LOGICAL to ENABLE_EXPERIMENTAL_FEATURES, ENABLE_EXPERIMENTAL_FEATURES needs to appear earlier.
But ENABLE_EXPERIMENTAL_FEATURES depends on various things, which also need to be moved.

>> LayoutTests/imported/w3c/ChangeLog:14
>> +        not been implemented yet.
> 
> Is there any bug about this? Otherwise it'd be nice to report it and link it from here for future reference.

https://bugs.webkit.org/show_bug.cgi?id=166941

>> LayoutTests/platform/mac/TestExpectations:1773
>> +imported/w3c/web-platform-tests/css/css-logical/logical-box-border-width.html  [ Skip ]
> 
> So the tests are skip in Mac? Why? What happens in other platforms?

Otherwise EWS for Mac fail, probably because ENABLE_EXPERIMENTAL_FEATURES is false.
In other platforms EWS seem OK.
Comment 53 Oriol Brufau 2018-09-12 17:21:03 PDT Comment hidden (obsolete)
Comment 54 Oriol Brufau 2018-09-13 11:49:55 PDT Comment hidden (obsolete)
Comment 55 Oriol Brufau 2018-09-21 08:23:58 PDT Comment hidden (obsolete)
Comment 56 Oriol Brufau 2018-09-21 08:45:27 PDT Comment hidden (obsolete)
Comment 57 Simon Fraser (smfr) 2018-09-24 16:05:44 PDT
Comment on attachment 350367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350367&action=review

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:91
> +ENABLE_CSS_LOGICAL = ENABLE_EXPERIMENTAL_FEATURES;

I would prefer this be called ENABLE_CSS_LOGICAL_PROPERTIES. Otherwise, the reader asks "enable CSS logical what?".

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4235
> +    if (showEnd)

You could just do if (!compareCSSValuePtr(startValue, endValue)) here.

> Source/WebCore/css/StyleProperties.cpp:414
> +            // All components are "initial" and start is not implicit.

This comment duplicates the previous line of code.

> Source/WebCore/css/StyleProperties.cpp:424
> +    if (showEnd) {

if (!start.value()->equals(*end.value()))

> Source/WebCore/css/StyleProperties.cpp:1067
> +                if (!borderBlockFallbackShorthandProperty)
> +                    borderBlockFallbackShorthandProperty = CSSPropertyBorderBlockColor;
> +                // FIXME: Deal with cases where only some of border-block-(start|end) are specified.
> +                ASSERT(CSSPropertyBorderBlock - firstCSSProperty < shorthandPropertyAppeared.size());
> +                if (!shorthandPropertyAppeared[CSSPropertyBorderBlock - firstCSSProperty]) {
> +                    value = borderPropertyValue(borderBlockWidthShorthand(), borderBlockStyleShorthand(), borderBlockColorShorthand());
> +                    if (value.isNull())
> +                        shorthandPropertyAppeared.set(CSSPropertyBorderBlock - firstCSSProperty);
> +                    else
> +                        shorthandPropertyID = CSSPropertyBorderBlock;
> +                } else if (shorthandPropertyUsed[CSSPropertyBorderBlock - firstCSSProperty])
> +                    shorthandPropertyID = CSSPropertyBorderBlock;
> +                if (!shorthandPropertyID)
> +                    shorthandPropertyID = borderBlockFallbackShorthandProperty;

This is complicated enough that maybe it can move into a separate function.

> Source/WebCore/css/StyleProperties.cpp:1094
> +                if (!borderInlineFallbackShorthandProperty)
> +                    borderInlineFallbackShorthandProperty = CSSPropertyBorderInlineColor;
> +                // FIXME: Deal with cases where only some of border-inline-(start|end) are specified.
> +                ASSERT(CSSPropertyBorderInline - firstCSSProperty < shorthandPropertyAppeared.size());
> +                if (!shorthandPropertyAppeared[CSSPropertyBorderInline - firstCSSProperty]) {
> +                    value = borderPropertyValue(borderInlineWidthShorthand(), borderInlineStyleShorthand(), borderInlineColorShorthand());
> +                    if (value.isNull())
> +                        shorthandPropertyAppeared.set(CSSPropertyBorderInline - firstCSSProperty);
> +                    else
> +                        shorthandPropertyID = CSSPropertyBorderInline;
> +                } else if (shorthandPropertyUsed[CSSPropertyBorderInline - firstCSSProperty])
> +                    shorthandPropertyID = CSSPropertyBorderInline;
> +                if (!shorthandPropertyID)
> +                    shorthandPropertyID = borderInlineFallbackShorthandProperty;

Ditto.

> Source/WebCore/css/parser/CSSPropertyParser.h:69
> +    bool consume2Values(const StylePropertyShorthand&, bool important);

I would call this consume2ValueShorthand()

> Source/WebCore/css/parser/CSSPropertyParser.h:70
>      bool consume4Values(const StylePropertyShorthand&, bool important);

And rename this accordingly.
Comment 58 Oriol Brufau 2018-09-24 16:19:55 PDT
Comment on attachment 350367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350367&action=review

>> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:91
>> +ENABLE_CSS_LOGICAL = ENABLE_EXPERIMENTAL_FEATURES;
> 
> I would prefer this be called ENABLE_CSS_LOGICAL_PROPERTIES. Otherwise, the reader asks "enable CSS logical what?".

My idea was that this flag could be reused for CSS logical values. Do you prefer a separate flag for that?
Comment 59 Simon Fraser (smfr) 2018-09-24 16:26:22 PDT
(In reply to Oriol Brufau from comment #58)
> Comment on attachment 350367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350367&action=review
> 
> >> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:91
> >> +ENABLE_CSS_LOGICAL = ENABLE_EXPERIMENTAL_FEATURES;
> > 
> > I would prefer this be called ENABLE_CSS_LOGICAL_PROPERTIES. Otherwise, the reader asks "enable CSS logical what?".
> 
> My idea was that this flag could be reused for CSS logical values. Do you
> prefer a separate flag for that?

I see. I guess it does match the spec short name so maybe it's OK.
Comment 60 Simon Fraser (smfr) 2018-09-24 16:27:35 PDT
(In reply to Oriol Brufau from comment #52)
> Otherwise EWS for Mac fail, probably because ENABLE_EXPERIMENTAL_FEATURES is
> false.
> In other platforms EWS seem OK.

macOS is our primary testing platform, so it's bad to disable the tests there. You probably need to have the tests enable the experimental feature explicitly (see webkit-dev discussion)
Comment 61 Oriol Brufau 2018-09-25 10:47:46 PDT Comment hidden (obsolete)
Comment 62 Oriol Brufau 2018-09-25 11:12:49 PDT
(In reply to Simon Fraser (smfr) from comment #57)
> This is complicated enough that maybe it can move into a separate function.
It needs to access various local variables, so instead of creating a separate function with lots of parameters I created a lambda function.

(In reply to Simon Fraser (smfr) from comment #59)
> I see. I guess it does match the spec short name so maybe it's OK.
OK, I have left this as-is and addressed your other suggestions from comment #57.

(In reply to Simon Fraser (smfr) from comment #60)
> macOS is our primary testing platform, so it's bad to disable the tests
> there. You probably need to have the tests enable the experimental feature
> explicitly (see webkit-dev discussion)

Can you link that discussion? In https://lists.webkit.org/pipermail/webkit-dev/2018-August/030094.html you said "Right, sorry. I don't think we have a way to do that."

Then Michael Catanzaro said I could try setting the flags to ENABLE_EXPERIMENTAL_FEATURES which is what I did, but it seems that's false for Mac EWS.

More recently there has been this thread about Internal feature flags: https://lists.webkit.org/pipermail/webkit-dev/2018-September/030123.html
But as far as I understand these are runtime flags.
Comment 63 Oriol Brufau 2018-10-17 12:19:06 PDT Comment hidden (obsolete)
Comment 64 Oriol Brufau 2018-10-17 12:25:19 PDT
(In reply to Simon Fraser (smfr) from comment #60)
> macOS is our primary testing platform, so it's bad to disable the tests
> there.
Instead of a compile flag, I have switched to a runtime flag so that the tests don't need to be skipped. I have added code to properly hide CSS properties behind runtime flags (except in cssText, which will need bug 190496), and I have added tests about this.

Do the changes look good?
Comment 65 Oriol Brufau 2018-10-17 13:18:20 PDT Comment hidden (obsolete)
Comment 66 EWS Watchlist 2018-10-17 16:02:02 PDT Comment hidden (obsolete)
Comment 67 EWS Watchlist 2018-10-17 16:02:14 PDT Comment hidden (obsolete)
Comment 68 Oriol Brufau 2018-10-17 16:30:33 PDT Comment hidden (obsolete)
Comment 69 Oriol Brufau 2018-10-17 16:37:23 PDT
Comment on attachment 352658 [details]
Archive of layout-test-results from ews202 for win-future

I added a test with lists all exposed CSS properties in order to check that disabled ones don't appear there. But it seems different platforms can have different properties so I may need to rethink this.

The clang compile error should be fixed now, let's see what EWS say about other platforms.
Comment 70 EWS Watchlist 2018-10-17 17:40:54 PDT Comment hidden (obsolete)
Comment 71 EWS Watchlist 2018-10-17 17:40:57 PDT Comment hidden (obsolete)
Comment 72 EWS Watchlist 2018-10-17 18:12:00 PDT Comment hidden (obsolete)
Comment 73 EWS Watchlist 2018-10-17 18:12:02 PDT Comment hidden (obsolete)
Comment 74 EWS Watchlist 2018-10-17 18:50:30 PDT Comment hidden (obsolete)
Comment 75 EWS Watchlist 2018-10-17 18:50:32 PDT Comment hidden (obsolete)
Comment 76 EWS Watchlist 2018-10-17 19:27:50 PDT Comment hidden (obsolete)
Comment 77 EWS Watchlist 2018-10-17 19:28:02 PDT Comment hidden (obsolete)
Comment 78 EWS Watchlist 2018-10-17 19:42:10 PDT Comment hidden (obsolete)
Comment 79 EWS Watchlist 2018-10-17 19:42:13 PDT Comment hidden (obsolete)
Comment 80 EWS Watchlist 2018-10-17 21:39:28 PDT Comment hidden (obsolete)
Comment 81 EWS Watchlist 2018-10-17 21:39:30 PDT Comment hidden (obsolete)
Comment 82 Oriol Brufau 2018-10-18 12:27:52 PDT Comment hidden (obsolete)
Comment 83 Oriol Brufau 2018-10-18 12:56:41 PDT Comment hidden (obsolete)
Comment 84 EWS Watchlist 2018-10-18 14:07:04 PDT Comment hidden (obsolete)
Comment 85 EWS Watchlist 2018-10-18 14:07:06 PDT Comment hidden (obsolete)
Comment 86 Oriol Brufau 2018-10-18 17:00:50 PDT Comment hidden (obsolete)
Comment 87 Oriol Brufau 2018-10-19 07:56:35 PDT Comment hidden (obsolete)
Comment 88 EWS Watchlist 2018-10-19 08:58:06 PDT Comment hidden (obsolete)
Comment 89 EWS Watchlist 2018-10-19 08:58:09 PDT Comment hidden (obsolete)
Comment 90 EWS Watchlist 2018-10-19 10:12:22 PDT Comment hidden (obsolete)
Comment 91 EWS Watchlist 2018-10-19 10:12:25 PDT Comment hidden (obsolete)
Comment 92 Oriol Brufau 2018-10-31 14:19:42 PDT Comment hidden (obsolete)
Comment 93 EWS Watchlist 2018-10-31 15:23:17 PDT Comment hidden (obsolete)
Comment 94 EWS Watchlist 2018-10-31 15:23:20 PDT Comment hidden (obsolete)
Comment 95 Oriol Brufau 2018-10-31 15:26:06 PDT Comment hidden (obsolete)
Comment 96 Oriol Brufau 2018-11-02 09:43:59 PDT
Comment on attachment 353540 [details]
Patch

Finally EWS is green. Requesting review.
Simon Fraser already approved the code for the logical shorthands, so only the code related with runtime flags needs to be reviewed.
Comment 97 Manuel Rego Casasnovas 2018-11-09 01:24:48 PST
r=me

This is really nice, we never had support to disable CSS properties from runtime flags so this is great.

Simon already approved the CSS changes in the previous version using compile flags, so that part should be fine already.
The rest of the patch LGTM but mabye someone wants to review the changes in CSSProperties.json and makepprop.pl, as I'm not an expert on those.

Adding some people on CC just in case they want to take a look. Thanks!
Comment 98 Antti Koivisto 2018-11-14 22:47:44 PST
Comment on attachment 353540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353540&action=review

r=me, looks good

> Source/WebCore/css/CSSStyleDeclaration.cpp:244
> +        CSSPropertyID id = static_cast<CSSPropertyID>(propertyID);

Could be auto

> Source/WebCore/css/StyleProperties.cpp:390
> +        return String();

return { };

(some other places too)

> Source/WebCore/css/StyleProperties.cpp:393
> +    PropertyReference start = propertyAt(startValueIndex);
> +    PropertyReference end = propertyAt(endValueIndex);

auto

> Source/WebCore/css/parser/CSSPropertyParser.cpp:150
> +        CSSPropertyID propertyID = static_cast<CSSPropertyID>(hashTableEntry->id);

auto
Comment 99 Oriol Brufau 2018-11-15 07:58:18 PST Comment hidden (obsolete)
Comment 100 EWS Watchlist 2018-11-15 08:56:41 PST Comment hidden (obsolete)
Comment 101 EWS Watchlist 2018-11-15 08:56:44 PST Comment hidden (obsolete)
Comment 102 EWS Watchlist 2018-11-15 09:20:00 PST Comment hidden (obsolete)
Comment 103 EWS Watchlist 2018-11-15 09:20:03 PST Comment hidden (obsolete)
Comment 104 EWS Watchlist 2018-11-15 09:52:28 PST Comment hidden (obsolete)
Comment 105 EWS Watchlist 2018-11-15 09:52:41 PST Comment hidden (obsolete)
Comment 106 EWS Watchlist 2018-11-15 09:52:45 PST Comment hidden (obsolete)
Comment 107 EWS Watchlist 2018-11-15 09:52:47 PST Comment hidden (obsolete)
Comment 108 EWS Watchlist 2018-11-15 10:41:49 PST Comment hidden (obsolete)
Comment 109 EWS Watchlist 2018-11-15 10:41:52 PST Comment hidden (obsolete)
Comment 110 Oriol Brufau 2018-11-15 11:40:08 PST
Created attachment 354961 [details]
Patch
Comment 111 Oriol Brufau 2018-11-15 11:43:31 PST
I have updated the expectations for the list of enabled CSS properties, now EWS should be green.
Comment 112 WebKit Commit Bot 2018-11-15 13:15:36 PST
Comment on attachment 354961 [details]
Patch

Clearing flags on attachment: 354961

Committed r238244: <https://trac.webkit.org/changeset/238244>
Comment 113 WebKit Commit Bot 2018-11-15 13:15:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 114 Radar WebKit Bug Importer 2018-11-15 13:17:08 PST
<rdar://problem/46105624>
Comment 115 Truitt Savell 2018-11-15 17:02:08 PST
Looks like the changes in https://trac.webkit.org/changeset/238244/webkit 

has caused all high Sierra test runs to fail early with 50 crashes and caused 25 api failures. This is isolated to only High Sierra which is why EWS did not catch it. 

Build:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/builds/9301

log from a Crash:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/r238245%20(9301)/accessibility/aria-hidden-hides-all-elements-crash-log.txt

The traceback contains CSS related files which point towards this revisions changes.
Comment 116 Truitt Savell 2018-11-15 17:07:47 PST
Reverted r238244 for reason:

Caused High Sierra test runs to fail early with 50 crashes and casued 25 API failures.

Committed r238262: <https://trac.webkit.org/changeset/238262>
Comment 117 Oriol Brufau 2019-01-21 10:34:59 PST
Created attachment 359698 [details]
Patch
Comment 118 Oriol Brufau 2019-01-21 10:38:50 PST
I have removed the webexposed tests that listed all CSS properties since each platform has different expectations and it's a mess.

The crash seemed unrelated, let's try to land again.
Comment 119 Manuel Rego Casasnovas 2019-01-22 01:26:24 PST
Comment on attachment 359698 [details]
Patch

Ok, let's see re-land this and watch to bots to see if the crash appears again.
Comment 120 WebKit Commit Bot 2019-01-22 01:52:54 PST
Comment on attachment 359698 [details]
Patch

Clearing flags on attachment: 359698

Committed r240251: <https://trac.webkit.org/changeset/240251>
Comment 121 WebKit Commit Bot 2019-01-22 01:52:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 122 Michael Catanzaro 2019-01-22 16:37:35 PST
Committed r240310: <https://trac.webkit.org/changeset/240310>