Replace compile flag with runtime check for text-underline-position

Replace compile flag with runtime check for text-underline-position

Replaces the CSS3_TEXT compile flag (from WebKit feature flags) with
runtime check for text-underline-position usage. Also removed the
-webkit- vendor prefix from the CSS property (including layout test
updates).

BUG=239225
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161235

[abinader, 2013-10-30 22:33:25]

This is an ongoing process of polishing text-underline-position code, which
included fixing some issues that popped up while replacing the CSS3_TEXT compile
flag with runtime check, as well as removing the -webkit- vendor prefix. 

After this patch, more work is ongoing on the following issues:

[1] Remove 'alphabetic' from list of supported values to text-underline-position
CSS property
https://code.google.com/p/chromium/issues/detail?id=311335

[2] text-underline-position should not be inherited
https://code.google.com/p/chromium/issues/detail?id=242303

[Julien - ping for review, 2013-10-31 22:54:06]

The unprefixing is awesome. One issue with the change before it can land though.

https://codereview.chromium.org/40733004/diff/220001/LayoutTests/virtual/stab...
File
LayoutTests/virtual/stable/webexposed/css-properties-as-js-properties-expected.txt
(right):

https://codereview.chromium.org/40733004/diff/220001/LayoutTests/virtual/stab...
LayoutTests/virtual/stable/webexposed/css-properties-as-js-properties-expected.txt:195:
textUnderlinePosition
That is NOT OK. We don't want text-underline-position in stable (ie if "Enable
experimental Web feature" is off).

This means you have to implement a filter in core/page/RuntimeCSSEnabled.cpp
(see setPropertySwitchesFromRuntimeFeatures).

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSParse...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:9159: return false;
Do we need this line if we implement a default case?

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSPrope...
File Source/core/css/CSSProperties.in (left):

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:242: -webkit-text-underline-position
custom_value, condition=CSS3_TEXT
This was the last property with "condition", we may want to follow-up this
change with removing support for conditional properties!

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSPrope...
File Source/core/css/CSSPropertyNames.in (left):

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSPrope...
Source/core/css/CSSPropertyNames.in:385: -webkit-text-underline-position
condition=CSS3_TEXT
Same comment here about a "conditional" follow-up.

https://codereview.chromium.org/40733004/diff/220001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/40733004/diff/220001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:1101: textDecorationThickness =
max(textDecorationThickness, styleToUse->computedFontSize() / 10.0f);
Maybe be worth simplifying the code while removing the #idef. Here this could be
simplified as:

float textDecorationThickness = std::max(1.f, styleToUse->computedFontSize() /
10.f);

[abinader, 2013-11-01 12:16:42]

https://codereview.chromium.org/40733004/diff/220001/LayoutTests/virtual/stab...
File
LayoutTests/virtual/stable/webexposed/css-properties-as-js-properties-expected.txt
(right):

https://codereview.chromium.org/40733004/diff/220001/LayoutTests/virtual/stab...
LayoutTests/virtual/stable/webexposed/css-properties-as-js-properties-expected.txt:195:
textUnderlinePosition
On 2013/10/31 22:54:07, Julien Chaffraix - PST wrote:
> That is NOT OK. We don't want text-underline-position in stable (ie if "Enable
> experimental Web feature" is off).
> 
> This means you have to implement a filter in core/page/RuntimeCSSEnabled.cpp
> (see setPropertySwitchesFromRuntimeFeatures).

Yes, absolutely right! Fixed in next patch :-)

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSParse...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:9159: return false;
On 2013/10/31 22:54:07, Julien Chaffraix - PST wrote:
> Do we need this line if we implement a default case?

Sounds redundant, indeed. I've put an ASSERT_NOT_REACHED() with a break on
default case.

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSPrope...
File Source/core/css/CSSProperties.in (left):

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:242: -webkit-text-underline-position
custom_value, condition=CSS3_TEXT
On 2013/10/31 22:54:07, Julien Chaffraix - PST wrote:
> This was the last property with "condition", we may want to follow-up this
> change with removing support for conditional properties!

Yes :-D I am going to remove that on a separate patch, as it changes a lot of
different files inside Source/build/scripts.

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSPrope...
File Source/core/css/CSSPropertyNames.in (left):

https://codereview.chromium.org/40733004/diff/220001/Source/core/css/CSSPrope...
Source/core/css/CSSPropertyNames.in:385: -webkit-text-underline-position
condition=CSS3_TEXT
On 2013/10/31 22:54:07, Julien Chaffraix - PST wrote:
> Same comment here about a "conditional" follow-up.
 
Ack!

https://codereview.chromium.org/40733004/diff/220001/Source/core/rendering/In...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/40733004/diff/220001/Source/core/rendering/In...
Source/core/rendering/InlineTextBox.cpp:1101: textDecorationThickness =
max(textDecorationThickness, styleToUse->computedFontSize() / 10.0f);
On 2013/10/31 22:54:07, Julien Chaffraix - PST wrote:
> Maybe be worth simplifying the code while removing the #idef. Here this could
be
> simplified as:
> 
> float textDecorationThickness = std::max(1.f, styleToUse->computedFontSize() /
> 10.f);

Sure thing. Did the same for gap calculation inside computeUnderlineOffset
static function. Good catch :-)

[abinader, 2013-11-01 12:23:41]

Status update:

[1] Remove 'alphabetic' from list of supported values to text-underline-position
https://code.google.com/p/chromium/issues/detail?id=311335
I have a patch ready for removing the 'alphabetic' value from
text-underline-position, just need to wait for this patch to land first.

[2] text-underline-position should not be inherited
https://code.google.com/p/chromium/issues/detail?id=242303
This is no longer valid, as the current specification treats
text-underline-position as inheritable.

[3] Implement support for 'left' and 'right' values from text-underline-position
https://code.google.com/p/chromium/issues/detail?id=313888
This is my current work-in-progress related with text-underline-position. Expect
a patch soon :-)

[4] Remove support for conditional properties inside generated files
https://code.google.com/p/chromium/issues/detail?id=314055
Finishing the patch atm, will follow after this patch goes in.

[Julien - ping for review, 2013-11-01 17:20:56]

lgtm

https://codereview.chromium.org/40733004/diff/370001/LayoutTests/fast/css3-te...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt
(right):

https://codereview.chromium.org/40733004/diff/370001/LayoutTests/fast/css3-te...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt:10:
FAIL computedStyle.getPropertyCSSValue('text-underline-position').cssText should
be auto. Threw exception TypeError: Cannot read property 'cssText' of null
These baselines are wrong and are trivial to fix (cross-platform, text-only). We
should fix them to have some cq coverage instead of relying on the
auto-rebaseline bot.

https://codereview.chromium.org/40733004/diff/370001/Source/core/css/CSSParse...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/40733004/diff/370001/Source/core/css/CSSParse...
Source/core/css/CSSParser-in.cpp:9155: ASSERT_NOT_REACHED();
Note that this ASSERT will trigger, so I don't think it's a good idea.

text-underline-position: groove;

Why don't we stick to the existing code as the improvements we tried to make are
not really helping?

[abinader, 2013-11-01 19:13:30]

On 2013/11/01 17:20:56, Julien Chaffraix - PST wrote:
> lgtm
> 
>
https://codereview.chromium.org/40733004/diff/370001/LayoutTests/fast/css3-te...
> File
>
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt
> (right):
> 
>
https://codereview.chromium.org/40733004/diff/370001/LayoutTests/fast/css3-te...
>
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt:10:
> FAIL computedStyle.getPropertyCSSValue('text-underline-position').cssText
should
> be auto. Threw exception TypeError: Cannot read property 'cssText' of null
> These baselines are wrong and are trivial to fix (cross-platform, text-only).
We
> should fix them to have some cq coverage instead of relying on the
> auto-rebaseline bot.
> 
>
https://codereview.chromium.org/40733004/diff/370001/Source/core/css/CSSParse...
> File Source/core/css/CSSParser-in.cpp (right):
> 
>
https://codereview.chromium.org/40733004/diff/370001/Source/core/css/CSSParse...
> Source/core/css/CSSParser-in.cpp:9155: ASSERT_NOT_REACHED();
> Note that this ASSERT will trigger, so I don't think it's a good idea.
> 
> text-underline-position: groove;
> 
> Why don't we stick to the existing code as the improvements we tried to make
are
> not really helping?

Both fixed this time. You are right about the assertion, that was a bad
assumption indeed. Uploading a new patch now.

[commit-bot: I haz the power, 2013-11-01 19:14:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bruno.d@partner.samsung.com/40733004/4...

[commit-bot: I haz the power, 2013-11-01 20:34:11]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-03 17:06:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bruno.d@partner.samsung.com/40733004/6...

[commit-bot: I haz the power, 2013-11-03 18:26:28]

Change committed as 161235