Bug 102491

Created attachment 174668 [details] Patch Proposed patch

Created attachment 179922 [details] Patch Updated to current git revision

Comment on attachment 179922 [details] Patch Attachment 179922 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15400222 New failing tests: fast/frames/sandboxed-iframe-attribute-parsing.html

Created attachment 183092 [details] Parse text-underline-position Fix getComputedStyle-text-underline-position-expected.txt

Created attachment 183104 [details] Parse text-underline-position (EWS only version) Same patch with css3-text enabled by default so that bots can really test it.

Comment on attachment 183104 [details] Parse text-underline-position (EWS only version) Attachment 183104 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15909703

Comment on attachment 183104 [details] Parse text-underline-position (EWS only version) Attachment 183104 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15914671

Comment on attachment 183104 [details] Parse text-underline-position (EWS only version) Attachment 183104 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15912670 New failing tests: fast/css3-text/css3-text-decoration/text-decoration-style.html fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html

Comment on attachment 183104 [details] Parse text-underline-position (EWS only version) Attachment 183104 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15908834 New failing tests: fast/css3-text/css3-text-decoration/text-decoration-style.html fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html

Comment on attachment 183104 [details] Parse text-underline-position (EWS only version) Attachment 183104 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15908986

Created attachment 183171 [details] Parse text-underline-position (EWS only version) Enable css3-conditional-rules in some missing points.

Comment on attachment 183171 [details] Parse text-underline-position (EWS only version) Attachment 183171 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/15906898

Comment on attachment 183171 [details] Parse text-underline-position (EWS only version) Attachment 183171 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15913760

Comment on attachment 183171 [details] Parse text-underline-position (EWS only version) Attachment 183171 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15926694

Created attachment 183222 [details] Parse text-underline-position (EWS only version) Attempt to fix compilation problem in some bots

Comment on attachment 183222 [details] Parse text-underline-position (EWS only version) Attachment 183222 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15944080

Comment on attachment 183222 [details] Parse text-underline-position (EWS only version) Attachment 183222 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15943096 New failing tests: fast/css3-text/css3-text-decoration/text-decoration-style.html fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html

Created attachment 183309 [details] Parse text-underline-position (EWS only version) Disable css3-text decoration unit tests since they lack -expected.png files. See https://bugs.webkit.org/show_bug.cgi?id=100546.

Comment on attachment 183309 [details] Parse text-underline-position (EWS only version) Attachment 183309 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15947195

Created attachment 189797 [details] Parse text-underline-position Update to current git revision.

Created attachment 189798 [details] Parse text-underline-position (EWS only version) Same patch with css3-text enabled.

Comment on attachment 189798 [details] Parse text-underline-position (EWS only version) Attachment 189798 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16728997

Comment on attachment 189797 [details] Parse text-underline-position View in context: https://bugs.webkit.org/attachment.cgi?id=189797&action=review You are on the right tracks. This will need another round as you allow cases that are not compliant (and not tested). > Source/WebCore/ChangeLog:24 > + (WebCore): These entries are not useful and should be removed. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1361 > + if (list->length()) > + return list; > + > + ASSERT_NOT_REACHED(); > + return cssValuePool().createExplicitInitialValue(); We should probably just have: ASSERT(list->length()); return list; > Source/WebCore/css/CSSParser.cpp:9100 > + m_valueList->next(); Do you need this? > Source/WebCore/css/CSSParser.cpp:9104 > + if (value->id == CSSValueAlphabetic) { This could be added to the 'if' above. > Source/WebCore/css/CSSParser.cpp:9106 > + m_valueList->next(); Ditto. > Source/WebCore/css/CSSParser.cpp:9111 > + bool isValid = true; I don't think we need this |isValid| variable: if you are not valid, I would expect you to bail out from the function as there is no point in continuing. > Source/WebCore/css/CSSParser.cpp:9113 > + while (isValid && value) { We could probably get away without the loop here as we really expect to parse only 2 values but in any order. > Source/WebCore/css/CSSParser.cpp:9132 > + You would wrongly accept: * -webkit-text-underline-position: under under; * -webkit-text-underline-position: under under under; * -webkit-text-underline-position: left under under; These cases should be added to your test. > Source/WebCore/css/CSSValueKeywords.in:949 > +#if !(defined(ENABLE_SVG) && ENABLE_SVG) && defined(ENABLE_CSS3_TEXT) && ENABLE_CSS3_TEXT It's probably a bad idea to have the same property defined in 2 places. I would rather see it defined here only. > Source/WebCore/css/CSSValueKeywords.in:952 > +#endif // CSS3_TEXT Nit: No need for the "// CSS3_TEXT", especially since it's wrong. > Source/WebCore/css/StyleBuilder.cpp:1960 > + setPropertyHandler(CSSPropertyWebkitTextUnderlinePosition, ApplyPropertyTextUnderlinePosition::createHandler()); If you handle the value here (which is fine), there is a giant switch to update in StyleResolver.cpp to say that you do so. Unfortunately, it doesn't break the build because of SVG defining the 'default' case. > Source/WebCore/rendering/style/RenderStyleConstants.h:368 > +inline TextUnderlinePosition operator| (TextUnderlinePosition a, TextUnderlinePosition b) { return TextUnderlinePosition(int(a) | int(b)); } > +inline TextUnderlinePosition& operator|=(TextUnderlinePosition& a, TextUnderlinePosition b) { return a = a | b; } Not a huge fan of these operators for 2 reasons: * What you get is *not* a TextUnderlinePosition as it isn't a defined value. * This doesn't match our current style, which is to use flags. It's a smart way of handling this situation but I think a simple typedef unsigned TextUnderlinePositionFlags; (or a plain unsigned) would do a better job. > LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js:99 > + Missing the bad cases: left left, left right, right right.

Created attachment 191578 [details] Parse text-underline-position Fix issues reported.

Created attachment 191580 [details] Parse text-underline-position (EWS only version) Same patch with css3-text enabled.

(In reply to comment #23) > These entries are not useful and should be removed. > > Source/WebCore/css/CSSParser.cpp:9100 > > + m_valueList->next(); > > Do you need this? No, but I have changed the parser in the latest patch, so this is not a issue anymore. > > Source/WebCore/css/CSSValueKeywords.in:949 > > +#if !(defined(ENABLE_SVG) && ENABLE_SVG) && defined(ENABLE_CSS3_TEXT) && ENABLE_CSS3_TEXT > > It's probably a bad idea to have the same property defined in 2 places. I would rather see it defined here only. Can we define a value and not use it? There are the cases where both ENABLE_SVG and ENABLE_CSS3_TEXT are enabled, when both are disabled and when only one of them is enabled. I added the extra ENABLE_SVG to prevent alphabetic being defined twice when both ENABLE_SVG and ENABLE_CSS3_TEXT are enabled. I do not see how I can define it here without causing a conflict in the svg file that also defines the 'alphabetic' value without such a check, unless we define it regardless of the svg and css3-text feature flags values. > > Source/WebCore/css/CSSValueKeywords.in:952 > > +#endif // CSS3_TEXT > > Nit: No need for the "// CSS3_TEXT", especially since it's wrong. Ok, the feature flag was renamed some months ago and I forgot to change the text above. > > Source/WebCore/css/StyleBuilder.cpp:1960 > > + setPropertyHandler(CSSPropertyWebkitTextUnderlinePosition, ApplyPropertyTextUnderlinePosition::createHandler()); > > If you handle the value here (which is fine), there is a giant switch to update in StyleResolver.cpp to say that you do so. Unfortunately, it doesn't break the build because of SVG defining the 'default' case. Which of the switches in StyleResolver.cpp are you talking about? > > Source/WebCore/rendering/style/RenderStyleConstants.h:368 > > +inline TextUnderlinePosition operator| (TextUnderlinePosition a, TextUnderlinePosition b) { return TextUnderlinePosition(int(a) | int(b)); } > > +inline TextUnderlinePosition& operator|=(TextUnderlinePosition& a, TextUnderlinePosition b) { return a = a | b; } > > Not a huge fan of these operators for 2 reasons: > * What you get is *not* a TextUnderlinePosition as it isn't a defined value. > * This doesn't match our current style, which is to use flags. > > It's a smart way of handling this situation but I think a simple > > typedef unsigned TextUnderlinePositionFlags; > (or a plain unsigned) > > would do a better job. That does not do the trick but I managed remove those operators and refactor the only place that use them. The other issues reported have been fixed in the latest patch.

Comment on attachment 191580 [details] Parse text-underline-position (EWS only version) Attachment 191580 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/16999094

Created attachment 191775 [details] Parse text-underline-position According to http://www.w3.org/TR/CSS/#partial we should not accept values that do not have rendering support. 'under left' and 'under right' are not implement in https://bugs.webkit.org/show_bug.cgi?id=102795 so remove the support for them in this patch.

Created attachment 191778 [details] Parse text-underline-position (EWS only version) Same patch with css3-text enabled.

Comment on attachment 191778 [details] Parse text-underline-position (EWS only version) Attachment 191778 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/16987183

Comment on attachment 191775 [details] Parse text-underline-position View in context: https://bugs.webkit.org/attachment.cgi?id=191775&action=review > Source/WebCore/ChangeLog:3 > + [css3-text] Add parsing support text-underline-position property from CSS3 Text This should be renamed as you don't totally match the specification in this change. > Source/WebCore/ChangeLog:12 > + OBS: values 'under left' and 'under right' are not implemented in this OBS? If you meant observation, either state it or use 'note'. Also, you should just mention what you did here instead of just observing it: This patch extends the existing parsing to support 'alphabetic' and 'under'. We don't fully match the specification as we don't support [ left | right ] and this is left for another implementation as the rendering will need to be added. > Source/WebCore/ChangeLog:42 > + * rendering/style/RenderStyleConstants.h: > + (WebCore::operator| ): > + (WebCore::operator|=): > + Added bitwise TextUnderlinePosition enum with operator functions | and |= operator (used in StyleBuilder). The ChangeLog is stale. It should be updated before landing. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1362 > +static PassRefPtr<CSSValue> renderTextUnderlinePositionFlagsToCSSValue(TextUnderlinePosition textUnderlinePosition) > +{ > + if (textUnderlinePosition == TextUnderlinePositionAuto) > + return cssValuePool().createIdentifierValue(CSSValueAuto); > + > + if (textUnderlinePosition == TextUnderlinePositionAlphabetic) > + return cssValuePool().createIdentifierValue(CSSValueAlphabetic); > + > + if (textUnderlinePosition == TextUnderlinePositionUnder) > + return cssValuePool().createIdentifierValue(CSSValueUnder); > + > + // TODO: Implement support for 'under left' and 'under right' values. > + > + ASSERT_NOT_REACHED(); > + return cssValuePool().createExplicitInitialValue(); > +} Nit: This could be replaced with defining CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition) in CSSPrimitiveValueMapping.h AFAICT. > Source/WebCore/css/CSSParser.cpp:9130 > + break; No need for the break. > Source/WebCore/css/CSSPrimitiveValueMappings.h:2372 > + // TODO: Implement support for 'under left' and 'under right' values. We don't have TODO's in WebKit, only FIXME's. > Source/WebCore/css/CSSValueKeywords.in:949 > +#if !(defined(ENABLE_SVG) && ENABLE_SVG) && defined(ENABLE_CSS3_TEXT) && ENABLE_CSS3_TEXT This will basically work but you still have 2 definitions of the same property which means that they can get out of sync. I would rather go with a single definition than 2: either by defining the property here only using (my preference) #if (defined(ENABLE_SVG) && ENABLE_SVG) || (defined(ENABLE_CSS3_TEXT) && ENABLE_CSS3_TEXT) and removing the SVG bit or just enabling it always (not great but better than having to maintain 2 definitions). > Source/WebCore/rendering/style/RenderStyleConstants.h:363 > + // TODO: Implement support for 'under left' and 'under right' values. s/TODO/FIXME.

Created attachment 192542 [details] Parse text-underline-position Patch for landing.

Comment on attachment 192542 [details] Parse text-underline-position Clearing flags on attachment: 192542 Committed r145450: <http://trac.webkit.org/changeset/145450>

All reviewed patches have been landed. Closing bug.