[webkit-reviews] review denied: [Bug 188386] [css-logical] Flow relative margin, padding, border and sizing properties : [Attachment 346764] Patch

[bugzilla-daemon at webkit.org]

Manuel Rego Casasnovas <rego at igalia.com> has denied Oriol Brufau
<obrufau at igalia.com>'s request for review:
Bug 188386: [css-logical] Flow relative margin, padding, border and sizing
properties
https://bugs.webkit.org/show_bug.cgi?id=188386

Attachment 346764: Patch

https://bugs.webkit.org/attachment.cgi?id=346764&action=review




--- Comment #10 from Manuel Rego Casasnovas <rego at igalia.com> ---
Comment on attachment 346764
  --> https://bugs.webkit.org/attachment.cgi?id=346764
Patch

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

It looks good but I think it still needs some minor changes.
Apart from that you should fix the test afilures reported by the EWSs.

> Source/WebCore/ChangeLog:35
> +	   (WebCore::isLayoutDependent):

Please document this change here, as this is something special.

> Source/WebCore/css/CSSProperties.json:3069
> -		   "category": "css-22",
> -		   "url":
"https://www.w3.org/TR/CSS22/box.html#propdef-padding"
> +		   "category": "css-22",
> +		   "url":
"https://www.w3.org/TR/CSS22/box.html#propdef-padding"

What's the change in these lines?

> LayoutTests/imported/w3c/ChangeLog:14
> +	   These properties provide the author with the ability to control
margins
> +	   through logical, rather than physical, direction and dimension
mappings.
> +
> +	   Only longhand properties and border shorthands for specific sides
are
> +	   implemented as part of this patch.
> +
> +	   The existing prefixed properties become aliases of the new ones.

Probably this specific ChangeLog (the one in "LayoutTests/imported/w3c/")
should mention that you're importing the WPT test suite for CSS Logical
Properties and Values spec.

>
LayoutTests/imported/w3c/web-platform-tests/css/css-logical/animation-001-expec
ted.txt:2
> +FAIL Logical properties can be animated using object notation assert_equals:
expected "50px" but got "0px"

It'd be nice to report a bug about these failures.

>
LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logical-box-border-
color-expected.txt:15
> +FAIL Test that logical border-*-color properties share computed values with
their physical associates, with 'writing-mode: sideways-rl; direction: rtl; '.
assert_equals: logical properties on one declaration, writing mode properties
on another, 'writing-mode: sideways-rl; direction: rtl; ', border-bottom-color
expected "rgb(1, 1, 1)" but got "rgb(4, 4, 4)"

Add a comment on ChangeLog about all sideways tests failing as they're not
supported.

>
LayoutTests/imported/w3c/web-platform-tests/css/css-logical/logical-box-margin-
expected.txt:4
> +FAIL Test that margin-* shorthands set the computed value of both logical
and physical longhands, with 'writing-mode: horizontal-tb; direction: ltr; '.
assert_equals: shorthand properties on one declaration, writing mode properties
on another, 'writing-mode: horizontal-tb; direction: ltr; ',
margin-inline-start expected "1px" but got "0px"

Why this stuff is failing?