[css3-text] Implement text-decoration property shorthand

[css3-text]  Implement text-decoration property shorthand

Adds support for CSS Level 3 text-decoration property implementation as
shorthand (enabled via runtime flag), according to the specification:

http://www.w3.org/TR/css-text-decor-3/#text-decoration-property

Backported from WebKit: https://bugs.webkit.org/show_bug.cgi?id=92000

All layout tests that verifies/uses text-decoration computed style were
updated. Editing behavior is preserved for text-decoration.

BUG=165462
NOTRY=true

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

[abinader, 2013-07-25 16:38:03]

Both Win and Linux reports a failing test:

LayoutTests/fast/images/imagemap-case.html

Which is not related to this patch - probably caused by ToT changes.

[abinader, 2013-07-26 02:28:34]

On 2013/07/25 16:38:03, abinader wrote:
> Both Win and Linux reports a failing test:
> 
> LayoutTests/fast/images/imagemap-case.html
> 
> Which is not related to this patch - probably caused by ToT changes.

Seems fixed now, {mac,linux,win}_layout_rel bots are now green.

[Julien - ping for review, 2013-07-27 01:46:35]

Levi should probably look at the editing bits, they look too complex to me and I
would need more time to determine if they are correct.

https://codereview.chromium.org/20262002/diff/12001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html
(right):

https://codereview.chromium.org/20262002/diff/12001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html:7:
<script
src="script-tests/getComputedStyle-text-decoration-shorthand.js"></script>
Nit: I would rather see this script inlined.

Also it seems that git cl considers this file to be a copy of an existing one,
probably not something you intended.

https://codereview.chromium.org/20262002/diff/12001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-shorthand.js
(right):

https://codereview.chromium.org/20262002/diff/12001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-shorthand.js:53:

The unconsistencies and copy and pasting in test makes me sad, can't we have a
testing function that takes care of testing all the values all the time?

https://codereview.chromium.org/20262002/diff/12001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/20262002/diff/12001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:8822: if (list->length() && (isValid ||
inShorthand())) {
Is it totally right to allow an empty list of keywords to be considered fine
here? (e.g. navy dotted row)

https://codereview.chromium.org/20262002/diff/12001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/12001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:1119: isPropertyTextDecorationEquivalent =
isPropertyTextDecorationEquivalent || property.id() ==
CSSPropertyTextDecorationLine;
isPropertyTextDecorationEquivalent |= property.id() ==
CSSPropertyTextDecorationLine;

[abinader, 2013-07-28 04:21:52]

https://codereview.chromium.org/20262002/diff/12001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html
(right):

https://codereview.chromium.org/20262002/diff/12001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html:7:
<script
src="script-tests/getComputedStyle-text-decoration-shorthand.js"></script>
On 2013/07/27 01:46:35, Julien Chaffraix wrote:
> Nit: I would rather see this script inlined.
> 
> Also it seems that git cl considers this file to be a copy of an existing one,
> probably not something you intended.

Right, putting JS code on a separate file seemed to be the default pattern for
computed style layout tests, but I can easily embed it.

https://codereview.chromium.org/20262002/diff/12001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-shorthand.js
(right):

https://codereview.chromium.org/20262002/diff/12001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-decoration-shorthand.js:53:

On 2013/07/27 01:46:35, Julien Chaffraix wrote:
> The unconsistencies and copy and pasting in test makes me sad, can't we have a
> testing function that takes care of testing all the values all the time?

Agreed, I already had implemented such types of testing functions previously and
forgot to use them (like the one from the link below):

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'll update the layout test to clean this up.

https://codereview.chromium.org/20262002/diff/12001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/20262002/diff/12001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:8822: if (list->length() && (isValid ||
inShorthand())) {
On 2013/07/27 01:46:35, Julien Chaffraix wrote:
> Is it totally right to allow an empty list of keywords to be considered fine
> here? (e.g. navy dotted row)

If I understood your question correctly, you are questioning whether doing the
following:

elem.style.textDecoration = "navy dotted"

Would be valid. So the answer is yes: according to the specification, "Omitted
values are set to their initial values." - in this case, the text decoration
line value is omitted, thus making it revert back to "initial", which is then
properly computed as "none". I'll add it to the
getComputedStyle-text-decoration-shorthand.html layout test for sanity check.

https://codereview.chromium.org/20262002/diff/12001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/12001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:1119: isPropertyTextDecorationEquivalent =
isPropertyTextDecorationEquivalent || property.id() ==
CSSPropertyTextDecorationLine;
On 2013/07/27 01:46:35, Julien Chaffraix wrote:
> isPropertyTextDecorationEquivalent |= property.id() ==
> CSSPropertyTextDecorationLine;

Ah yes, shame on me on skipping operator|= usage :)

[abinader, 2013-07-28 15:28:20]

That's, weird, all {mac,win,linux}_blink_rel trybots are shown as failed, though
they really are green when you click on their links. I believe Alexis has
mentioned about this issue a few days ago o blink-dev. I'm manually firing
{mac,win,linux}_release_rel for sanity check.

[abinader, 2013-07-29 15:43:40]

On 2013/07/28 15:28:20, abinader wrote:
> That's, weird, all {mac,win,linux}_blink_rel trybots are shown as failed,
though
> they really are green when you click on their links. I believe Alexis has
> mentioned about this issue a few days ago o blink-dev. I'm manually firing
> {mac,win,linux}_release_rel for sanity check.

Read it: {mac,win,linux}_layout_rel. I've fired _blink_rel ones again today,
with same behavior as previous run (runs are green but are shown as red).

[Julien - ping for review, 2013-07-29 18:53:11]

FWIW I think we should pepper the editing code with ASSERTs that we don't see
CSSPropertyTextDecoration if CSS 3 Text Decoration is enabled.

https://codereview.chromium.org/20262002/diff/12001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/20262002/diff/12001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:8822: if (list->length() && (isValid ||
inShorthand())) {
On 2013/07/28 04:21:52, abinader wrote:
> On 2013/07/27 01:46:35, Julien Chaffraix wrote:
> > Is it totally right to allow an empty list of keywords to be considered fine
> > here? (e.g. navy dotted row)
> 
> If I understood your question correctly, you are questioning whether doing the
> following:
> 
> elem.style.textDecoration = "navy dotted"
> 
> Would be valid. So the answer is yes: according to the specification, "Omitted
> values are set to their initial values." - in this case, the text decoration
> line value is omitted, thus making it revert back to "initial", which is then
> properly computed as "none". I'll add it to the
> getComputedStyle-text-decoration-shorthand.html layout test for sanity check.

Ah, that's not really what I meant (sorry for being unclear). Rereading the
logic, it's actually OK anyway: I was afraid we would accept non-valid
text-decoration-style values here due to the inShorthand() check.

https://codereview.chromium.org/20262002/diff/29001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/20262002/diff/29001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:3092:
list->append(getCSSPropertyValuesForShorthandProperties(StylePropertyShorthand(CSSPropertyTextDecoration,
properties, WTF_ARRAY_LENGTH(properties))));
CSSComputedStyleDeclaration::getCSSPropertyValuesForShorthandProperties already
returns a space-separated list of properties. Couldn't we just return
getCSSPropertyValuesForShorthandProperties(textDecorationShorthand()) above
instead of wrapping this list in another list?

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:674:
m_mutableStyle->removeProperty(CSSPropertyTextDecoration);
This should be in an 'else': shorthands are expanded at parsing time so we
shouldn't see them here.

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:681: CSSPropertyTextDecoration,
Note that once we remove RuntimeEnabledFeatures::css3TextDecorationsEnabled()
this should be removed from the list as we should never see a shorthand.

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:755: if (propertyID ==
CSSPropertyWebkitTextDecorationsInEffect) {
Probably overstepping my knowledge of editing here but
WebKitTextDecorationInEffect seems very much unused in our metrics. The extra
complexity from it seems nasty so maybe we could remove it and simplify this CL?
(it shouldn't block this CL though and could be removed as a follow-up CL but I
would prefer to do this change before)

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:757: RefPtr<CSSValue> textDecorationLine =
inlineStyle->getPropertyCSSValue(CSSPropertyTextDecorationLine);
This is usually inlined into the if () statement as you only need
|textDecorationLine| inside the branch.

[abinader, 2013-07-29 21:15:31]

Updated patch according to Julien's 2nd round review.

Highlights:
- Now using getCSSPropertyValuesForShorthandProperties() instead of custom
function in CSSComputedStyleDeclaration.cpp;
- Fixed a typo in CSSShorthand.in - including updated
inspector/styles/styles-new-API-expected.txt to reflect right order of
text-decoration longhands;
- Added ASSERT on EditingStyle to prohibit CSSPropertyTextDecoration as eligible
editing property when CSS3 Text Decoration runtime feature is enabled;
- Minor fixes & cleanups in EditingStyle.cpp;

https://codereview.chromium.org/20262002/diff/12001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/20262002/diff/12001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:8822: if (list->length() && (isValid ||
inShorthand())) {
On 2013/07/29 18:53:11, Julien Chaffraix wrote:
> On 2013/07/28 04:21:52, abinader wrote:
> > On 2013/07/27 01:46:35, Julien Chaffraix wrote:
> > > Is it totally right to allow an empty list of keywords to be considered
fine
> > > here? (e.g. navy dotted row)
> > 
> > If I understood your question correctly, you are questioning whether doing
the
> > following:
> > 
> > elem.style.textDecoration = "navy dotted"
> > 
> > Would be valid. So the answer is yes: according to the specification,
"Omitted
> > values are set to their initial values." - in this case, the text decoration
> > line value is omitted, thus making it revert back to "initial", which is
then
> > properly computed as "none". I'll add it to the
> > getComputedStyle-text-decoration-shorthand.html layout test for sanity
check.
> 
> Ah, that's not really what I meant (sorry for being unclear). Rereading the
> logic, it's actually OK anyway: I was afraid we would accept non-valid
> text-decoration-style values here due to the inShorthand() check.

Ah, I see your point now :) no, this function is not responsible for
text-decoration-style.

https://codereview.chromium.org/20262002/diff/29001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/20262002/diff/29001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:3092:
list->append(getCSSPropertyValuesForShorthandProperties(StylePropertyShorthand(CSSPropertyTextDecoration,
properties, WTF_ARRAY_LENGTH(properties))));
On 2013/07/29 18:53:12, Julien Chaffraix wrote:
> CSSComputedStyleDeclaration::getCSSPropertyValuesForShorthandProperties
already
> returns a space-separated list of properties. Couldn't we just return
> getCSSPropertyValuesForShorthandProperties(textDecorationShorthand()) above
> instead of wrapping this list in another list?

Nice catch, Julien! By using
getCSSPropertyValuesForShorthandProperties(textDecorationShorthand()) I was able
to catch a typo in CSSShorthands.in, where the longhand property orders for
text-decoration were incorrect (causing the computed style layout test to fail).

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:674:
m_mutableStyle->removeProperty(CSSPropertyTextDecoration);
On 2013/07/29 18:53:12, Julien Chaffraix wrote:
> This should be in an 'else': shorthands are expanded at parsing time so we
> shouldn't see them here.

You are right, I probably overlooked at it.

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:681: CSSPropertyTextDecoration,
On 2013/07/29 18:53:12, Julien Chaffraix wrote:
> Note that once we remove RuntimeEnabledFeatures::css3TextDecorationsEnabled()
> this should be removed from the list as we should never see a shorthand.

Ack, I'll add a FIXME: Needs to be removed when CSS3 Text Decoration feature is
no longer experimental.

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:755: if (propertyID ==
CSSPropertyWebkitTextDecorationsInEffect) {
On 2013/07/29 18:53:12, Julien Chaffraix wrote:
> Probably overstepping my knowledge of editing here but
> WebKitTextDecorationInEffect seems very much unused in our metrics. The extra
> complexity from it seems nasty so maybe we could remove it and simplify this
CL?
> (it shouldn't block this CL though and could be removed as a follow-up CL but
I
> would prefer to do this change before)

Yes, totally agreed upon removing this somewhat hackish property - however, I'm
afraid due to its complexity it would be better to postpone this task to be done
after this patch gets landed (as we dicussed on IRC).

https://codereview.chromium.org/20262002/diff/29001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:757: RefPtr<CSSValue> textDecorationLine =
inlineStyle->getPropertyCSSValue(CSSPropertyTextDecorationLine);
On 2013/07/29 18:53:12, Julien Chaffraix wrote:
> This is usually inlined into the if () statement as you only need
> |textDecorationLine| inside the branch.

Indeed, makes code looks sharper as well :)

[Julien - ping for review, 2013-07-30 22:37:22]

https://codereview.chromium.org/20262002/diff/42001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/20262002/diff/42001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:267: crbug.com/165462
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-line.html [
NeedsRebaseline ]
Could you explain why this test needs a rebaseline after this change? (I would
have expected it to be orthogonal to more decoration changes)

https://codereview.chromium.org/20262002/diff/42001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/20262002/diff/42001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:2121: // Fall through to CSS2.1 implementation
if runtime feature is disabled.
This comment could be improved if you explained why it's important:

// Fall through 'text-decoration-line' parsing if CSS 3 Text Decoration is
disabled to match CSS 2.1 rules for parsing 'text-decoration'.

https://codereview.chromium.org/20262002/diff/42001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/42001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:104:
ASSERT(RuntimeEnabledFeatures::css3TextDecorationsEnabled() &&
!properties.contains(CSSPropertyTextDecoration));
Won't this ASSERT will trigger on Chrome if you disable the new Web Platform
features?

https://codereview.chromium.org/20262002/diff/42001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:777: }
These 2 branches are really the same if you had a variable for the
CSSPropertyID:

CSSPropertyID textDecorationPropertyID =
RuntimeEnabledFeatures::css3TextDecorationsEnabled() ?
CSSPropertyTextDecorationLine : CSSPropertyTextDecoration;
...

Most of the code change in this file would actually be removed / simplified
greatly if you just stored this CSSPropertyID in EditingStyle and used this
variable instead of the hardcoded CSSPropertyTextDecoration.

[abinader, 2013-07-31 21:24:02]

https://codereview.chromium.org/20262002/diff/42001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/20262002/diff/42001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:267: crbug.com/165462
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-line.html [
NeedsRebaseline ]
On 2013/07/30 22:37:22, Julien Chaffraix wrote:
> Could you explain why this test needs a rebaseline after this change? (I would
> have expected it to be orthogonal to more decoration changes)

Yes, you are right about preserving behavior. However, I found out we were doing
it wrong on a specific test:
(1) assign "text-decoration: <non-initial valid value(s)>" then
(2) set element.style.textDecorationLine = "";

This should clear the property value back to initial value (none), but isn't
currently being properly done. FWIW, I've checked Firefox's behavior which also
does the same (resets the property value back to initial).

https://codereview.chromium.org/20262002/diff/42001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/20262002/diff/42001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:2121: // Fall through to CSS2.1 implementation
if runtime feature is disabled.
On 2013/07/30 22:37:22, Julien Chaffraix wrote:
> This comment could be improved if you explained why it's important:
> 
> // Fall through 'text-decoration-line' parsing if CSS 3 Text Decoration is
> disabled to match CSS 2.1 rules for parsing 'text-decoration'.

Ack! Sounds better indeed.

https://codereview.chromium.org/20262002/diff/42001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/42001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:104:
ASSERT(RuntimeEnabledFeatures::css3TextDecorationsEnabled() &&
!properties.contains(CSSPropertyTextDecoration));
On 2013/07/30 22:37:22, Julien Chaffraix wrote:
> Won't this ASSERT will trigger on Chrome if you disable the new Web Platform
> features?

Oh, dumb me. Indeed... I also need to make the other way (remove
CSSPropertyTextDecorationLine from properties vector if runtime feature is
disabled) on the code above it.

https://codereview.chromium.org/20262002/diff/42001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:777: }
On 2013/07/30 22:37:22, Julien Chaffraix wrote:
> These 2 branches are really the same if you had a variable for the
> CSSPropertyID:
> 
> CSSPropertyID textDecorationPropertyID =
> RuntimeEnabledFeatures::css3TextDecorationsEnabled() ?
> CSSPropertyTextDecorationLine : CSSPropertyTextDecoration;
> ...
> 
> Most of the code change in this file would actually be removed / simplified
> greatly if you just stored this CSSPropertyID in EditingStyle and used this
> variable instead of the hardcoded CSSPropertyTextDecoration.

Sounds good, I thought about it but wanted to highlight every place where we are
modifying the code for clarity. I'm working on this idea atm.

[Julien - ping for review, 2013-08-02 23:22:12]

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:59: // Needs to be removed when CSS3 Text
Decoration is no longer experimental.
I don't think this comment adds much value: it's fairly obvious that we should
remove this function once it's trivially returning one value.
Again, this is a 'what' comment.

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:60: static const CSSPropertyID&
runtimeTextDecorationProperty()
How about textDecorationLine()? (this matches what we expect to support in the
future and you can explain *why* we return CSSPropertyDecoration for CSS 2.1)

Alternative naming: textDecorationPropertyForEditing()

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:73: // If you change this list, make sure
to update nonInheritedPropertyCount.
I missed that: nonInheritPropertyCount should still be updated as
text-decoration-line is not inherited per the specification.

Is this tested somewhere as it seems like a bad idea to include non-inherited
properties into the inherited ones?

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:116: ||
(!RuntimeEnabledFeatures::css3TextDecorationsEnabled() &&
properties.contains(CSSPropertyTextDecoration)));
This would be better off written:

#ifndef NDEBUG
    if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
        ASSERT(!properties.contains(CSSPropertyTextDecoration));
    else
        ASSERT(properties.contains(CSSPropertyTextDecoration));
#endif

Also, isn't it redundant with the code above anyway as we can trivially see what
you remove?

[abinader, 2013-08-05 22:51:14]

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:59: // Needs to be removed when CSS3 Text
Decoration is no longer experimental.
On 2013/08/02 23:22:13, Julien Chaffraix wrote:
> I don't think this comment adds much value: it's fairly obvious that we should
> remove this function once it's trivially returning one value.
> Again, this is a 'what' comment.

Ack.

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:60: static const CSSPropertyID&
runtimeTextDecorationProperty()
On 2013/08/02 23:22:13, Julien Chaffraix wrote:
> How about textDecorationLine()? (this matches what we expect to support in the
> future and you can explain *why* we return CSSPropertyDecoration for CSS 2.1)
> 
> Alternative naming: textDecorationPropertyForEditing()

Right, textDecorationPropertyForEditing() sounds good.

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:73: // If you change this list, make sure
to update nonInheritedPropertyCount.
On 2013/08/02 23:22:13, Julien Chaffraix wrote:
> I missed that: nonInheritPropertyCount should still be updated as
> text-decoration-line is not inherited per the specification.
> 
> Is this tested somewhere as it seems like a bad idea to include non-inherited
> properties into the inherited ones?

I wasn't planning on modifying the nonInheritedStaticPropertiesCount from 2 to
3, but I'm seeing now this is really needed for inheritableEditingProperties()
sanity - I erroneously assumed that both static functions shared the same
"properties" vector, and I was only making the proper sanity check on
allEditingProperties(). Nice catch!

https://codereview.chromium.org/20262002/diff/56001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:116: ||
(!RuntimeEnabledFeatures::css3TextDecorationsEnabled() &&
properties.contains(CSSPropertyTextDecoration)));
On 2013/08/02 23:22:13, Julien Chaffraix wrote:
> This would be better off written:
> 
> #ifndef NDEBUG
>     if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
>         ASSERT(!properties.contains(CSSPropertyTextDecoration));
>     else
>         ASSERT(properties.contains(CSSPropertyTextDecoration));
> #endif
> 
> Also, isn't it redundant with the code above anyway as we can trivially see
what
> you remove?

I guess you're right, this looks too trivial and thus unnecessary.

[Julien - ping for review, 2013-08-07 17:10:00]

lgtm

https://codereview.chromium.org/20262002/diff/72001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html
(right):

https://codereview.chromium.org/20262002/diff/72001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html:65:
shouldBe("checkComputedStyleValue()", "true");
The specification allows any order for <text-decoration-line>,
<text-decoration-style> and <text-decoration-color> in the shorthand.

Could we add some testing for the 6 possible orderings to ensure that we
properly parse them?

https://codereview.chromium.org/20262002/diff/72001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/72001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:68: static const unsigned
nonInheritedStaticPropertiesCount = 3;
It's too bad this isn't tested but I couldn't find a test that exercised this
code either.

[abinader, 2013-08-07 17:24:04]

https://codereview.chromium.org/20262002/diff/72001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html
(right):

https://codereview.chromium.org/20262002/diff/72001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html:65:
shouldBe("checkComputedStyleValue()", "true");
On 2013/08/07 17:10:00, Julien Chaffraix wrote:
> The specification allows any order for <text-decoration-line>,
> <text-decoration-style> and <text-decoration-color> in the shorthand.
> 
> Could we add some testing for the 6 possible orderings to ensure that we
> properly parse them?

Sure we can :-) Shall we append it to this patch or can it appear on a separate
one?

https://codereview.chromium.org/20262002/diff/72001/Source/core/editing/Editi...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/20262002/diff/72001/Source/core/editing/Editi...
Source/core/editing/EditingStyle.cpp:68: static const unsigned
nonInheritedStaticPropertiesCount = 3;
On 2013/08/07 17:10:00, Julien Chaffraix wrote:
> It's too bad this isn't tested but I couldn't find a test that exercised this
> code either.

Right, just in case, as we discussed yesterday on IRC, I am proposing usage of
CSSProperty::isInheritedProperty() instead of this static value:

https://codereview.chromium.org/22378004

[Julien - ping for review, 2013-08-07 18:14:54]

https://codereview.chromium.org/20262002/diff/72001/LayoutTests/fast/css3-tex...
File
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html
(right):

https://codereview.chromium.org/20262002/diff/72001/LayoutTests/fast/css3-tex...
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html:65:
shouldBe("checkComputedStyleValue()", "true");
On 2013/08/07 17:24:04, abinader wrote:
> On 2013/08/07 17:10:00, Julien Chaffraix wrote:
> > The specification allows any order for <text-decoration-line>,
> > <text-decoration-style> and <text-decoration-color> in the shorthand.
> > 
> > Could we add some testing for the 6 possible orderings to ensure that we
> > properly parse them?
> 
> Sure we can :-) Shall we append it to this patch or can it appear on a
separate
> one?

This patch please, it's bad form to land a change without proper coverage.

[abinader, 2013-08-07 18:51:52]

On 2013/08/07 18:14:54, Julien Chaffraix wrote:
>
https://codereview.chromium.org/20262002/diff/72001/LayoutTests/fast/css3-tex...
> File
>
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html
> (right):
> 
>
https://codereview.chromium.org/20262002/diff/72001/LayoutTests/fast/css3-tex...
>
LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-shorthand.html:65:
> shouldBe("checkComputedStyleValue()", "true");
> On 2013/08/07 17:24:04, abinader wrote:
> > On 2013/08/07 17:10:00, Julien Chaffraix wrote:
> > > The specification allows any order for <text-decoration-line>,
> > > <text-decoration-style> and <text-decoration-color> in the shorthand.
> > > 
> > > Could we add some testing for the 6 possible orderings to ensure that we
> > > properly parse them?
> > 
> > Sure we can :-) Shall we append it to this patch or can it appear on a
> separate
> > one?
> 
> This patch please, it's bad form to land a change without proper coverage.

Done! I have added a new test:
getComputedStyle-text-decoration-shorthand-ordering.html which tests for all 6
combinations.

[commit-bot: I haz the power, 2013-08-07 18:52:18]

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

[abinader, 2013-08-07 19:29:28]

On 2013/08/07 18:52:18, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/bruno.d%40partner.samsung.com/20262002...

Aborting commit because last minute revistion 155705 has added new editing/
layout tests that needs to replace mentions to text-decoration with
text-decoration-line. I'll upload an updated patch very soon.

[commit-bot: I haz the power, 2013-08-07 19:58:03]

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

[commit-bot: I haz the power, 2013-08-07 22:50:56]

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

[commit-bot: I haz the power, 2013-08-07 22:59:31]

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

[commit-bot: I haz the power, 2013-08-07 22:59:47]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-08-07 23:03:53]

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

[commit-bot: I haz the power, 2013-08-07 23:04:42]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-08-07 23:06:52]

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

[commit-bot: I haz the power, 2013-08-07 23:08:35]

Change committed as 155719

[falken, 2013-08-08 09:03:00]

On 2013/08/07 23:08:35, I haz the power (commit-bot) wrote:
> Change committed as 155719

I think this is causing the following assert failure when I try to open
content_shell:

ASSERTION FAILED: Shorthand property id = 151 wasn't expanded at parsing time
!isExpandedShorthand(id)
../../third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp(1152) :
static void WebCore::StyleBuilder::applyProperty(WebCore::CSSPropertyID,
WebCore::StyleResolverState&, WebCore::CSSValue*)

[abinader, 2013-08-08 17:21:03]

On 2013/08/08 09:03:00, falken wrote:
> On 2013/08/07 23:08:35, I haz the power (commit-bot) wrote:
> > Change committed as 155719
> 
> I think this is causing the following assert failure when I try to open
> content_shell:
> 
> ASSERTION FAILED: Shorthand property id = 151 wasn't expanded at parsing time
> !isExpandedShorthand(id)
> ../../third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp(1152)
:
> static void WebCore::StyleBuilder::applyProperty(WebCore::CSSPropertyID,
> WebCore::StyleResolverState&, WebCore::CSSValue*)

Hmm I see what is happening... my guess is that by adding text-decoration to
CSSShorthands.in, we do not check for runtime feature. It is only a shorthand
property when runtime feature is enabled, otherwise it fallbacks to current
implementation. I'll look for ways to tell CSSShorthands.in to have this
property listed only if it is enabled by runtime feature.