[webkit-reviews] review granted: [Bug 196139] Unprefix -webkit-text-orientation : [Attachment 394225] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted guowei_yang at apple.com's
request for review:
Bug 196139: Unprefix -webkit-text-orientation
https://bugs.webkit.org/show_bug.cgi?id=196139

Attachment 394225: Patch

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




--- Comment #16 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 394225
  --> https://bugs.webkit.org/attachment.cgi?id=394225
Patch

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

> Source/WebCore/ChangeLog:8
> +	   In order to unprefix -webkit-text-orientation to be
text-orientation, a new property, "text-orientation"

This should explain why using "aliases" was not acceptable. (The answer is that
the prefixed property accepts non-standard values, and the unprefixed property
shouldn't.)

> Source/WebCore/ChangeLog:10
> +	   is a high-priority property, and without extra logic, the CSS
property ordering rule cannot be enforced because 

Which ordering rule? I think you mean the "last rule wins" ordering rule.

> Source/WebCore/ChangeLog:14
> +	   each other, so that when applying high priorities, the algorithm
treats both -webkit-text-orientation and text-orientation
> +	   as the same property, and thus the CSS inline property ordering rule
will take effect. 

I don't think "treats them as the same property" is really what you mean.

"CSS inline" is a term-of-art, and I don't think you mean to invoke it here.

> Source/WebCore/ChangeLog:29
> +	   Changes made in:
> +		   CSSComputedStyleDeclaration.cpp
> +		   CSSParserFastPaths.cpp
> +		   StyleBuilderCustom.h
> +	   are for parsing the newly added "text-orientation" property
> +
> +	   Changes made in:
> +		   CSSProperties.json
> +		   makeprop.pl
> +		   CSSParserImpl.cpp
> +	   are for implementing the logic to treat the prefixed and unprefixed
CSS properties as the
> +	   same property so that the CSS inline property order rule will hold

These comments are better put on the bulleted lines below (lines 41-53). The
colons indicate than file-specific descriptions can be made there.

> Source/WebCore/ChangeLog:32
> +	   Tests: fast/text/orientation-inheritance.html

Do any Web Platform Tests need to be updated? Or any TestExpectations?

> Source/WebCore/WebCore.xcodeproj/xcshareddata/xcschemes/WebCore.xcscheme:47
> +	 <PathRunnable
> +	    runnableDebuggingMode = "0"
> +	    BundleIdentifier = "org.webkit.MiniBrowser"
> +	    FilePath = "/Users/framiere/projects/build/Debug/MiniBrowser.app">
> +	 </PathRunnable>

This should not be part of the patch.

> Source/WebCore/css/CSSProperties.json:120
> +	   "Indicates the prefixed or unprefixed version of the same property",

I think this comment should describe what the behavior does, not what it
happens to be used for in this patch. A human-readable description of what
"related-property" actually does (aka what the changes in CSSParserImpl.cpp do)
would be a better explanation.

> Source/WebCore/css/CSSProperties.json:641
> +		   "related-property": "-webkit-text-orientation",

The name "related-property" isn't great, because it has no indication
A) What it does
B) What it's used for
C) That it only is meaningful for high priority properties

What do you think of the name "enforce-order-with"? Perhaps you can come up
with something better.

> Source/WebCore/css/parser/CSSParserImpl.cpp:126
> +	   const unsigned relatedPropertyId =
getRelatedPropertyId(property.id());

Why is this before the "if" statement directly below it? It's usually best to
keep related code together.

> Source/WebCore/style/StyleBuilderCustom.h:764
> +inline void BuilderCustom::applyValueTextOrientation(BuilderState&
builderState, CSSValue& value)
> +{
> +    builderState.setTextOrientation(downcast<CSSPrimitiveValue>(value));
> +}

Is there any chance that we don't have to have an exact duplicate of
BuilderCustom::applyValueWebkitTextOrientation()? What happens if you try to
use the "name-for-methods" key in CSSProperties.json?

> LayoutTests/fast/text/orientation-mixed-unprefix-expected.html:9
> +<div id="t" style="-webkit-writing-mode: vertical-rl;
-webkit-text-orientation: mixed;">è¹æå¬å¸abcd</div>

I think all these tests need <meta charset="utf-8"> in order to properly
display this Unicode text.

> LayoutTests/fast/text/text-orientation-parse-competition.html:7
> +		<div id="test1" style="-webkit-writing-mode: vertical-rl;
-webkit-text-orientation: sideways; text-orientation: upright">ä½ å¥½ABC</div>

I think there should be another check in here with the order of
text-orientation and -webkit-text-orientation reversed.

> LayoutTests/fast/text/text-orientation-parse.html:61
> +		       
shouldBeEqualToString("window.getComputedStyle(document.getElementById('test1')
).webkitTextOrientation", "sideways");

I think we also need a parsing test that tests CSSValues themselves. Something
like:

<style id="thestyle">
dummy {
    text-orientation: upright;
    -webkit-text-orientation: sideways;
}
</style>
...
document.getElementById("thestyle").sheet.cssRules[0].style.getPropertyValue("t
ext-orientation") == ...;
document.getElementById("thestyle").sheet.cssRules[0].style.getPropertyValue("-
webkit-text-orientation") == ...;

Testing all the various combinations of this would be helpful.

> LayoutTests/fast/text/text-orientation-parse.html:94
> +		       
shouldBeEqualToString("window.getComputedStyle(document.getElementById('test27'
)).webkitTextOrientation", "sideways");

Shouldn't we also be checking .textOrientation in addition to
.webkitTextOrientation?

Shouldn't we also be testing .getPropertyValue("text-orientation") in addition
to .getPropertyValue("-webkit-text-orientation")?

I don't see any checks to make sure the new unprefixed property explicitly
doesn't parse any of the nonstandard values. Did I just miss them?