Bug 90958 - [css3-text] Add support for text-decoration-style
: [css3-text] Add support for text-decoration-style
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://www.w3.org/TR/css3-text/#text-...
: WebExposed
: 90959 93863 94093 94094
: 58491 92000 92868 94110 94111 94112 94114
  Show dependency treegraph
 
Reported: 2012-07-11 02:39 PST by
Modified: 2012-10-22 10:36 PST (History)


Attachments
Patch (65.39 KB, patch)
2012-07-31 07:50 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (64.89 KB, patch)
2012-07-31 15:46 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (65.41 KB, patch)
2012-08-06 19:08 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (66.31 KB, patch)
2012-08-06 20:41 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (340.70 KB, application/zip)
2012-08-06 22:10 PST, WebKit Review Bot
no flags Details
Patch (95.05 KB, patch)
2012-08-07 08:56 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (431.51 KB, application/zip)
2012-08-07 12:40 PST, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-04 (360.27 KB, application/zip)
2012-08-07 13:42 PST, WebKit Review Bot
no flags Details
Patch (94.72 KB, patch)
2012-08-07 15:16 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (94.88 KB, patch)
2012-08-08 13:00 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (95.66 KB, patch)
2012-08-09 14:57 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (72.79 KB, patch)
2012-08-10 08:14 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Patch (74.73 KB, patch)
2012-08-12 10:33 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (552.59 KB, application/zip)
2012-08-12 11:52 PST, WebKit Review Bot
no flags Details
Patch (74.77 KB, patch)
2012-08-12 21:18 PST, Bruno Abinader (history only)
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-07-11 02:39:13 PST
WebKit currently only has support for CSS text-decoration property, but CSS3's text-decoration-style. This is an individual task for bug 58491.
------- Comment #1 From 2012-07-14 01:09:34 PST -------
Marking bug 90959 as dependency.
------- Comment #2 From 2012-07-18 10:25:52 PST -------
Adding W3C draft link, and Mozilla compliance (w/ -moz prefix):

https://developer.mozilla.org/en/CSS/text-decoration-style
------- Comment #3 From 2012-07-31 07:50:18 PST -------
Created an attachment (id=155536) [details]
Patch

Proposed patch. Please note that patches from bugs 92000 and bug 90958 (respectively) should land before this.
------- Comment #4 From 2012-07-31 07:57:19 PST -------
Note: EWS is not able to apply patch because it depends on patches from bug 92000 and bug 90959 to be landed first.
------- Comment #5 From 2012-07-31 15:46:22 PST -------
Created an attachment (id=155653) [details]
Patch

Moved style data from StyleRareInheritedData to StyleRareNonInheritedData. Please note that patch from bug 92000 should land before this.
------- Comment #6 From 2012-08-06 19:08:16 PST -------
Created an attachment (id=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.
------- Comment #7 From 2012-08-06 19:56:53 PST -------
(From update of attachment 156828 [details])
Attachment 156828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13439942
------- Comment #8 From 2012-08-06 20:41:24 PST -------
Created an attachment (id=156849) [details]
Patch

Added Skia text decoration style enumerations boilerplate.
------- Comment #9 From 2012-08-06 22:10:47 PST -------
(From update of attachment 156849 [details])
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
------- Comment #10 From 2012-08-06 22:10:53 PST -------
Created an attachment (id=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
------- Comment #11 From 2012-08-07 08:56:58 PST -------
Created an attachment (id=156955) [details]
Patch

Added chromium-linux expected results.
------- Comment #12 From 2012-08-07 12:40:41 PST -------
(From update of attachment 156955 [details])
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
------- Comment #13 From 2012-08-07 12:40:47 PST -------
Created an attachment (id=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
------- Comment #14 From 2012-08-07 13:42:21 PST -------
(From update of attachment 156955 [details])
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
------- Comment #15 From 2012-08-07 13:42:29 PST -------
Created an attachment (id=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
------- Comment #16 From 2012-08-07 15:16:04 PST -------
Created an attachment (id=157019) [details]
Patch

The local generated image expectations for chromium-linux had font aliasing issues, now using ones generated by build bot.
------- Comment #17 From 2012-08-07 19:47:16 PST -------
(From update of attachment 157019 [details])
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.
------- Comment #18 From 2012-08-08 09:43:18 PST -------
(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.
------- Comment #19 From 2012-08-08 10:00:35 PST -------
(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

> 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?
------- Comment #20 From 2012-08-08 11:29:56 PST -------
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] [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.
------- Comment #21 From 2012-08-08 11:37:06 PST -------
Thank you again for the review Kenneth :) Just answering the review questions below:

(In reply to comment #19)
> (From update of attachment 157019 [details] [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.
------- Comment #22 From 2012-08-08 13:00:34 PST -------
Created an attachment (id=157275) [details]
Patch

Fixed issues pointed out by Kenneth.
------- Comment #23 From 2012-08-08 16:29:36 PST -------
(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

> 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?
------- Comment #24 From 2012-08-08 17:07:37 PST -------
(From update of attachment 157275 [details])
Attachment 157275 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13451945
------- Comment #25 From 2012-08-08 20:46:09 PST -------
(In reply to comment #23)
> (From update of attachment 157275 [details] [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).
------- Comment #26 From 2012-08-08 20:48:45 PST -------
(In reply to comment #24)
> (From update of attachment 157275 [details] [details])
> Attachment 157275 [details] [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.
------- Comment #27 From 2012-08-09 14:57:10 PST -------
Created an attachment (id=157557) [details]
Patch

Added detailed info about wavy value open bugs on ChangeLog, rebased after patch from bug 90959 got landed.
------- Comment #28 From 2012-08-09 15:49:06 PST -------
(From update of attachment 157557 [details])
Attachment 157557 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13471288
------- Comment #29 From 2012-08-10 01:38:27 PST -------
(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?

> 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.
------- Comment #30 From 2012-08-10 06:17:09 PST -------
(In reply to comment #29)
> (From update of attachment 157557 [details] [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.
------- Comment #31 From 2012-08-10 08:14:33 PST -------
Created an attachment (id=157732) [details]
Patch

Fixed issues pointed out by Kenneth.
------- Comment #32 From 2012-08-10 09:27:32 PST -------
(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.
------- Comment #33 From 2012-08-10 09:46:12 PST -------
(In reply to comment #32)
> (From update of attachment 157732 [details] [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?
------- Comment #34 From 2012-08-10 09:52:36 PST -------
(From update of attachment 157732 [details])
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
------- Comment #35 From 2012-08-10 10:03:11 PST -------
(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.

> 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
------- Comment #36 From 2012-08-10 10:10:46 PST -------
(From update of attachment 157732 [details])
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.
------- Comment #37 From 2012-08-10 11:12:48 PST -------
Thanks for the comments Julien!
------- Comment #38 From 2012-08-12 10:10:02 PST -------
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] [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
------- Comment #39 From 2012-08-12 10:33:38 PST -------
Created an attachment (id=157905) [details]
Patch

Fixed issues pointed out by Julien and Kenneth.
------- Comment #40 From 2012-08-12 11:52:49 PST -------
(From update of attachment 157905 [details])
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
------- Comment #41 From 2012-08-12 11:52:56 PST -------
Created an attachment (id=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
------- Comment #42 From 2012-08-12 12:07:41 PST -------
(From update of attachment 157905 [details])
Attachment 157905 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13475848
------- Comment #43 From 2012-08-12 17:52:15 PST -------
(From update of attachment 157905 [details])
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.
------- Comment #44 From 2012-08-12 21:18:14 PST -------
Created an attachment (id=157915) [details]
Patch

Added missing assertions, "double" value works on Skia platform.
------- Comment #45 From 2012-08-13 08:01:21 PST -------
(From update of attachment 157915 [details])
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.
------- Comment #46 From 2012-10-22 10:36:13 PST -------
Both parsing (bug 94093) and rendering (bug 94094) patches have landed. Marking bug as fixed.