[webkit-reviews] review denied: [Bug 228730] Replace webkit- prefix logical properties with Standard Properties in LayoutTests/ : [Attachment 435559] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 16 00:42:11 PDT 2021
Frédéric Wang (:fredw) <fred.wang at free.fr> has denied Sonia Singla
<soniasingla.1812 at gmail.com>'s request for review:
Bug 228730: Replace webkit- prefix logical properties with Standard Properties
in LayoutTests/
https://bugs.webkit.org/show_bug.cgi?id=228730
Attachment 435559: Patch
https://bugs.webkit.org/attachment.cgi?id=435559&action=review
--- Comment #4 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 435559
--> https://bugs.webkit.org/attachment.cgi?id=435559
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=435559&action=review
Thanks Sonia. It looks that you are close to get EWS green. Some comments
inline...
> LayoutTests/ChangeLog:144
> + *
platform/gtk/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-csst
ext-expected.txt:
As a reminder, you should not modify any test in
LayoutTests/imported/w3c/web-platform-tests/ (these have not been sync). Since
you don't modify any code here in this patch, there is no need to update the
expectation.
> LayoutTests/fast/css/remove-shorthand-expected.txt:57
> +removes "margin-block-start-collapse, margin-block-end-collapse"
You didn't modify remove-shorthand.html in this patch.
It looks like this test is checking what are the longhand properties removed
when removing the shorthand -webkit-margin-collapse.
> LayoutTests/platform/glib/svg/css/getComputedStyle-basic-expected.txt:468
> +rect: style.getPropertyCSSValue(margin-block-start-collapse) : [object
CSSPrimitiveValue]
For all these getComputedStyle-basic-expected.txt, you modified the expectation
but not the test LayoutTests/svg/css/getComputedStyle-basic.xhtml itself.
I believe we want to continue to test the -webkit prefixed properties here,
until they are actually removed. What you should do instead is updating
Layout/fast/css/getComputedStyle/resources/property-names.js to add the
unprefixed forms. Please check all the tests that are relying on this helper JS
file and run locally before uploading a new patch:
LayoutTests/fast/css/getComputedStyle/computed-style-without-renderer.html
LayoutTests/fast/css/getComputedStyle/computed-style.html
LayoutTests/svg/css/getComputedStyle-basic.xhtml
>
LayoutTests/platform/ios/fast/css/getComputedStyle/computed-style-expected.txt:
236
> +margin-block-start-collapse: collapse;
For these computed-style-expected.txt files, see my comment about
property-names.js
>
LayoutTests/platform/ios/fast/css/getComputedStyle/computed-style-without-rende
rer-expected.txt:235
> +margin-block-start-collapse: collapse
For these computed-style-without-renderer-expected.txt files, see my comment
about property-names.js
More information about the webkit-reviews
mailing list