Bug 90958 – [css3-text] Add support for text-decoration-style

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bruno Abinader (history only)]

WebKit currently only has support for CSS text-decoration property, but CSS3's text-decoration-style. This is an individual task for bug 58491.

[Bruno Abinader (history only)]

Marking bug 90959 as dependency.

[Bruno Abinader (history only)]

Adding W3C draft link, and Mozilla compliance (w/ -moz prefix):

https://developer.mozilla.org/en/CSS/text-decoration-style

[Bruno Abinader (history only)]

Created attachment 155536 [details]
Patch

Proposed patch. Please note that patches from bugs 92000 and bug 90958 (respectively) should land before this.

[Bruno Abinader (history only)]

Note: EWS is not able to apply patch because it depends on patches from bug 92000 and bug 90959 to be landed first.

[Bruno Abinader (history only)]

Created attachment 155653 [details]
Patch

Moved style data from StyleRareInheritedData to StyleRareNonInheritedData. Please note that patch from bug 92000 should land before this.

[Bruno Abinader (history only)]

Created attachment 156828 [details]
Patch

Various fixes, including: simpler design, fixed failing layout tests, does not interfere with editing feature, fixed relayout/repaint when property differs, does not depend on bug 92000 anymore.

[WebKit Review Bot]

Comment on attachment 156828 [details]
Patch

Attachment 156828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13439942

[Bruno Abinader (history only)]

Created attachment 156849 [details]
Patch

Added Skia text decoration style enumerations boilerplate.

[WebKit Review Bot]

Comment on attachment 156849 [details]
Patch

Attachment 156849 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13446584

New failing tests:
fast/css/text-decoration-style.html

[WebKit Review Bot]

Created attachment 156862 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Bruno Abinader (history only)]

Created attachment 156955 [details]
Patch

Added chromium-linux expected results.

[WebKit Review Bot]

Comment on attachment 156955 [details]
Patch

Attachment 156955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13444858

New failing tests:
fast/css/text-decoration-style.html

[WebKit Review Bot]

Created attachment 156977 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 156955 [details]
Patch

Attachment 156955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13450559

New failing tests:
fast/css/text-decoration-style.html

[WebKit Review Bot]

Created attachment 156991 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Bruno Abinader (history only)]

Created attachment 157019 [details]
Patch

The local generated image expectations for chromium-linux had font aliasing issues, now using ones generated by build bot.

[Bruno Abinader (history only)]

Comment on attachment 157019 [details]
Patch

This patch, as part of the new CSS3 text decoration properties work, is now ready for review. The implementation has changed significantly since first patch proposal. Please not this is rebased against master, so for commit purposes, it is recommended that patch from bug 90959 gets landed first, then we rebase it against it before landing.

[Allan Sandfeld Jensen]

Comment on attachment 157019 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        The "wavy" value is parsed, but not rendered (requires platform support).

Maybe I am misunderstanding you here, but wouldn't the correct thing to do, be to not parse it, when it is not supported? If you parse it but do not render it, it could override the fallback value of text-decoration-style.
* {
  property: fallback;
  property: new-stuff;
} 

In this case 'fallback' should apply whenever 'new-stuff' is not supported.

[Kenneth Rohde Christiansen]

Comment on attachment 157019 [details]
Patch

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

This looks pretty good to me

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1209
> +static PassRefPtr<CSSValue> renderTextDecorationStyleFlagsToCSSValue(int textDecorationStyle)

why int and not the enum? I see it is done like that elsewehre I just wondered why

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:410
> +        case WavyStroke:

notImplemented() ? bug report? add bug link?

> Source/WebCore/rendering/InlineTextBox.cpp:914
> +    DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1.0f));

doesnt the style guide say to use 1 and not 1.0f ?

> Source/WebCore/rendering/InlineTextBox.cpp:968
> +        // Double acts like Solid, only twice

punctuation mark

> Source/WebCore/rendering/InlineTextBox.cpp:977
> +        // FIXME: Currently not supported

For all ports? bug link?

[Bruno Abinader (history only)]

My intention is to add all necessary boilerplate for "wavy" value support. As for the fallback support, this is what actually happens - paint it as a regular "solid" (initial value) text decoration line until all platforms have proper rendering support. You can check the fast/css/text-decoration-style.html expected result (the wavy value is painted as a solid line).

That said, if not parsing "wavy" value at all is the best approach, I can modify the code accordingly. What do you think?

(In reply to comment #18)
> (From update of attachment 157019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157019&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        The "wavy" value is parsed, but not rendered (requires platform support).
> 
> Maybe I am misunderstanding you here, but wouldn't the correct thing to do, be to not parse it, when it is not supported? If you parse it but do not render it, it could override the fallback value of text-decoration-style.
> * {
>   property: fallback;
>   property: new-stuff;
> } 
> 
> In this case 'fallback' should apply whenever 'new-stuff' is not supported.

[Bruno Abinader (history only)]

Thank you again for the review Kenneth :) Just answering the review questions below:

(In reply to comment #19)
> (From update of attachment 157019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157019&action=review
> 
> This looks pretty good to me
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1209
> > +static PassRefPtr<CSSValue> renderTextDecorationStyleFlagsToCSSValue(int textDecorationStyle)
> 
> why int and not the enum? I see it is done like that elsewehre I just wondered why

I am following the common used style as much as possible, and I asked myself the same question when implementing it :) I agree with you, this should use the enum value to avoid erroneous usage at build time.

> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:410
> > +        case WavyStroke:
> 
> notImplemented() ? bug report? add bug link?

There is a bug currently opened for it (bug 92868), which depends on this bug. I forgot to mention that, sorry :/ Will add a comment about it.

> 
> > Source/WebCore/rendering/InlineTextBox.cpp:914
> > +    DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1.0f));
> 
> doesnt the style guide say to use 1 and not 1.0f ?

I used 1.0f because that was the previous value used, but indeed, using 1 is just more clear :)

> > Source/WebCore/rendering/InlineTextBox.cpp:977
> > +        // FIXME: Currently not supported
> 
> For all ports? bug link?

I've only tested the code for Qt and Chromium (Skia) ports. I've implemented support for all other missing values except "wavy" on Qt, but Skia also does not support "double" value yet. The bug link is the same described above (bug 92868), but thinking better now I should split this task in two new bugs, one related to Qt and other to Skia.

[Bruno Abinader (history only)]

Created attachment 157275 [details]
Patch

Fixed issues pointed out by Kenneth.

[Kenneth Rohde Christiansen]

Comment on attachment 157275 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:411
> +            // FIXME: Needs platform implementation.

Please file bug after this has landed

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:181
>      unsigned m_appearance : 6; // EAppearance
>      unsigned m_borderFit : 1; // EBorderFit
>      unsigned m_textCombine : 1; // CSS3 text-combine properties
> +    unsigned m_textDecorationStyle : TextDecorationStyleBits; // TextDecorationStyle
>  
>      unsigned m_wrapFlow: 3; // WrapFlow
>      unsigned m_wrapThrough: 1; // WrapThrough

Why do this differently that everyone else? and not just write 3?

[Build Bot]

Comment on attachment 157275 [details]
Patch

Attachment 157275 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13451945

[Bruno Abinader (history only)]

(In reply to comment #23)
> (From update of attachment 157275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157275&action=review
> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:411
> > +            // FIXME: Needs platform implementation.
> 
> Please file bug after this has landed

Right, the bug is already filled - bug 93509 ([Skia] Add support for text decoration "wavy" and "double" styles). I can add this information on the code if necessary.

> 
> > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:181
> >      unsigned m_appearance : 6; // EAppearance
> >      unsigned m_borderFit : 1; // EBorderFit
> >      unsigned m_textCombine : 1; // CSS3 text-combine properties
> > +    unsigned m_textDecorationStyle : TextDecorationStyleBits; // TextDecorationStyle
> >  
> >      unsigned m_wrapFlow: 3; // WrapFlow
> >      unsigned m_wrapThrough: 1; // WrapThrough
> 
> Why do this differently that everyone else? and not just write 3?

This follows the guideline from ETextDecoration enum, defined in Source/WebCore/rendering/style/RenderStyleConstants.h and used in Source/WebCore/rendering/style/StyleVisualData.h:52. I personally believe this is better approach because whenever "text-decoration-style" values changes in the specification, we can easily update the required bits in the same place where the values are updated, thus avoiding future errors only detected in runtime (the compiler doesn't check if the maximum value of enum can be stored on a 'n' bitsized unsigned variable).

[Bruno Abinader (history only)]

(In reply to comment #24)
> (From update of attachment 157275 [details])
> Attachment 157275 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/13451945

Inside http://queues.webkit.org/results/13451945 we can find:

The following build commands failed:
	CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/GraphicsContextCG.o platform/graphics/cg/GraphicsContextCG.cpp normal x86_64 c++ com.apple.compilers.llvmgcc42
(1 failure)

This does not seem to be related with the proposed patch, likely to be a build bot platform issue.

[Bruno Abinader (history only)]

Created attachment 157557 [details]
Patch

Added detailed info about wavy value open bugs on ChangeLog, rebased after patch from bug 90959 got landed.

[Build Bot]

Comment on attachment 157557 [details]
Patch

Attachment 157557 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13471288

[Kenneth Rohde Christiansen]

Comment on attachment 157557 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:2110
> +        // solid | double | dotted | dashed | wavy | inherit

isn't this supposed to be

[ solid | ... |  wary ] | inherit?

I am new to CSS so bear with me :-) The specs says "Inherited:	 no" so that just mean that it is not inherited by default? and not related to whether you can inherit? Firefox seems to support  "inherit" here

Is this being tested?

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178
> +    unsigned m_textDecorationStyle : TextDecorationStyleBits; // TextDecorationStyle

I still find it a bit weird that you are doing this, when noone else is doing it like that in this file. Then it would be better to convert it all in a separate patch and just follow consistency for now.

[Bruno Abinader (history only)]

(In reply to comment #29)
> (From update of attachment 157557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157557&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:2110
> > +        // solid | double | dotted | dashed | wavy | inherit
> 
> isn't this supposed to be
> 
> [ solid | ... |  wary ] | inherit?
> 
> I am new to CSS so bear with me :-) The specs says "Inherited:     no" so that just mean that it is not inherited by default? and not related to whether you can inherit? Firefox seems to support  "inherit" here
> 
> Is this being tested?

Ugh, I've copied the comment style of other properties and forgot to remove the "inherit" value :S You are right, it is explicitly said on the spec for the property not to be inherited and it is implemented as such (not inherited). Yes, It is currently being tested :) I am fixing the comment accordingly.

> 
> > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178
> > +    unsigned m_textDecorationStyle : TextDecorationStyleBits; // TextDecorationStyle
> 
> I still find it a bit weird that you are doing this, when noone else is doing it like that in this file. Then it would be better to convert it all in a separate patch and just follow consistency for now.

Alright, removing TextDecorationStyleBits as well.

[Bruno Abinader (history only)]

Created attachment 157732 [details]
Patch

Fixed issues pointed out by Kenneth.

[Julien Chaffraix]

Comment on attachment 157732 [details]
Patch

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

Sorry for the delay in looking at your patches.

> Source/WebCore/rendering/InlineTextBox.cpp:914
> +    DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1));

It's not OK to use DEFINE_STATIC_LOCAL to define constants (const float textDecorationThickness = 1.f would be better). I couldn't find the rationale behind this use so I am holding the patch until I get a clarification here.

[Bruno Abinader (history only)]

(In reply to comment #32)
> (From update of attachment 157732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157732&action=review
> 
> Sorry for the delay in looking at your patches.
> 
> > Source/WebCore/rendering/InlineTextBox.cpp:914
> > +    DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1));
> 
> It's not OK to use DEFINE_STATIC_LOCAL to define constants (const float textDecorationThickness = 1.f would be better). I couldn't find the rationale behind this use so I am holding the patch until I get a clarification here.

The rationale for this variable is that its value is now used in multiple places (see Source/WebCore/rendering/InlineTextBox.cpp lines 934, 1003 and 1010) thus avoiding inconsistencies. I defined it as static because the given source file follows this style (lines 716, 742), and also because as a constant value, I though it wouldn't make sense to construct/destroy this variable every time the function is called (which looks like a lot of times).

Said this, shall I change it for the suggested approach?

[Kenneth Rohde Christiansen]

Comment on attachment 157732 [details]
Patch

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

>>> Source/WebCore/rendering/InlineTextBox.cpp:914
>>> +    DEFINE_STATIC_LOCAL(float, textDecorationThickness, (1));
>> 
>> It's not OK to use DEFINE_STATIC_LOCAL to define constants (const float textDecorationThickness = 1.f would be better). I couldn't find the rationale behind this use so I am holding the patch until I get a clarification here.
> 
> The rationale for this variable is that its value is now used in multiple places (see Source/WebCore/rendering/InlineTextBox.cpp lines 934, 1003 and 1010) thus avoiding inconsistencies. I defined it as static because the given source file follows this style (lines 716, 742), and also because as a constant value, I though it wouldn't make sense to construct/destroy this variable every time the function is called (which looks like a lot of times).
> 
> Said this, shall I change it for the suggested approach?

Yes, please change. The macro ends up calling new and then dereferencing. 

> #define DEFINE_STATIC_LOCAL(type, name, arguments) \
>     static type& name = *new type arguments
> #endif

This is probably not what you intended

[Julien Chaffraix]

Comment on attachment 157732 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValueMappings.h:2214
> +    default:

I would remove the default label, that would enable the compiler to catch any mistakes at compile time.

> Source/WebCore/rendering/InlineTextBox.cpp:757
> +    int textStyle = styleToUse->textDecorationStyle();

An enum is technically an int but you lose a lot of information by not using the enum here. I would advocate passing the enum directly to paintDecoration (this means that textDecorations could modified in the same way).

> Source/WebCore/rendering/InlineTextBox.cpp:911
> +void InlineTextBox::paintDecoration(GraphicsContext* context, const FloatPoint& boxOrigin, int deco, int style, const ShadowData* shadow)

style is too general and usually refer to RenderStyle() in the render tree. textStyle or textStrokeStyle is better.

> Source/WebCore/rendering/InlineTextBox.cpp:980
> +    default:

I would rather have a case value for each of the possible values for the enum so that the compiler can catch any forgotten case at compile time.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178
> +    unsigned m_textDecorationStyle : 3; // TextDecorationStyle

You don't implement any invalidation logic currently. This means that any dynamic change to textDecorationStyle will not be repainted. This needs to be fixed and tested: the right fix is to implement a check in RenderStyle::diff and return StyleDifferenceRepaint

[Julien Chaffraix]

Comment on attachment 157732 [details]
Patch

A general comment, it's better to split parsing from actually implementing the feature which requires some flag (either compile or runtime) to avoid breaking CSS feature detection. This makes it easier for different domain experts to comment separately on the patch. On top of that, you wouldn't end up with a massive patch that reviewers usually avoid.

Here as you expose the feature to the web, it is expected to work properly which is more difficult to get right than an incremental approach. Under this assumption, this patch needs another round.

[Kenneth Rohde Christiansen]

Thanks for the comments Julien!

[Bruno Abinader (history only)]

Hi Julien, thank you for your time :) I am about to send an updated version of the patch with the fixes pointed out by your review. I do, however, have small comments on that:

(In reply to comment #35)
> (From update of attachment 157732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157732&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValueMappings.h:2214
> > +    default:
> 
> I would remove the default label, that would enable the compiler to catch any mistakes at compile time.

Good idea :) I removed this already from this patch, but almost all other switches on this source file uses "default", so I am going to handle these separately on bug 93781.

> > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:178
> > +    unsigned m_textDecorationStyle : 3; // TextDecorationStyle
> 
> You don't implement any invalidation logic currently. This means that any dynamic change to textDecorationStyle will not be repainted. This needs to be fixed and tested: the right fix is to implement a check in RenderStyle::diff and return StyleDifferenceRepaint

I guess you skipped that out of the patch :) There is a line added already to Source/WebCore/rendering/style/StyleRender:

diff --git a/Source/WebCore/rendering/style/RenderStyle.cpp b/Source/WebCore/rendering/style/RenderStyle.cpp
index 2303ffa..a6bb525 100644
--- a/Source/WebCore/rendering/style/RenderStyle.cpp
+++ b/Source/WebCore/rendering/style/RenderStyle.cpp
@@ -646,6 +646,7 @@ StyleDifference RenderStyle::diff(const RenderStyle* other, unsigned& changedCon
         || rareInheritedData->userSelect != other->rareInheritedData->userSelect
         || rareNonInheritedData->userDrag != other->rareNonInheritedData->userDrag
         || rareNonInheritedData->m_borderFit != other->rareNonInheritedData->m_borderFit
+        || rareNonInheritedData->m_textDecorationStyle != other->rareNonInheritedData->m_textDecorationStyle
         || rareInheritedData->textFillColor != other->rareInheritedData->textFillColor
         || rareInheritedData->textStrokeColor != other->rareInheritedData->textStrokeColor
         || rareInheritedData->textEmphasisColor != other->rareInheritedData->textEmphasisColor

[Bruno Abinader (history only)]

Created attachment 157905 [details]
Patch

Fixed issues pointed out by Julien and Kenneth.

[WebKit Review Bot]

Comment on attachment 157905 [details]
Patch

Attachment 157905 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13473967

New failing tests:
animations/suspend-resume-animation-events.html

[WebKit Review Bot]

Created attachment 157906 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 157905 [details]
Patch

Attachment 157905 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13475848

[Bruno Abinader (history only)]

Comment on attachment 157905 [details]
Patch

The following build commands failed:
	CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/DOMSVGPathSegMovetoRel.o /Volumes/Data/WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMSVGPathSegMovetoRel.mm normal x86_64 objective-c++ com.apple.compilers.llvmgcc42
	CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/DOMSVGRect.o /Volumes/Data/WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMSVGRect.mm normal x86_64 objective-c++ com.apple.compilers.llvmgcc42
	CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/DOMSVGScriptElement.o /Volumes/Data/WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMSVGScriptElement.mm normal x86_64 objective-c++ com.apple.compilers.llvmgcc42
	CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/DOMSVGStopElement.o /Volumes/Data/WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMSVGStopElement.mm normal x86_64 objective-c++ com.apple.compilers.llvmgcc42
	CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/FrameLoader.o loader/FrameLoader.cpp normal x86_64 c++ com.apple.compilers.llvmgcc42
	CompileC /Volumes/Data/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/GraphicsContextCG.o platform/graphics/cg/GraphicsContextCG.cpp normal x86_64 c++ com.apple.compilers.llvmgcc42
(6 failures)

This mac error and previous chromium build errors are not related to this patch - platform build issues (chromium already passing now), requesting flag again.

[Bruno Abinader (history only)]

Created attachment 157915 [details]
Patch

Added missing assertions, "double" value works on Skia platform.

[Julien Chaffraix]

Comment on attachment 157915 [details]
Patch

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

I am not supportive of such massive changes (especially since there are several of them) and would prefer if those were split. You are a new contributor to WebCore which means that there are rough edges and there is so much that can be caught by inspection when the patch is so big.

I would rather see you introduce an umbrella feature flag (runtime probably) for all the CSS 3 text changes and land your changes bits by bits so that each patch is atomic and well tested. That would also bring the average time for review down.

The current patch is lacking enough testing to be landed on top of that.

> Source/WebCore/ChangeLog:53
> +        * rendering/style/StyleRareNonInheritedData.h:

It's preferred to fill the function level entries.

> LayoutTests/ChangeLog:27
> +        * svg/css/getComputedStyle-basic-expected.txt:

All in all, your testing is extremely poor. You implemented a way to get the value back from JavaScript (using getComputedStyle), yet it's not tested. So is the invalidation logic (which was indeed implemented in the previous patch, my bad) which would require a very simple repaint test.

[Bruno Abinader (history only)]

Both parsing (bug 94093) and rendering (bug 94094) patches have landed. Marking bug as fixed.