[webkit-reviews] review granted: [Bug 126611] text-emphasis-position CSS property doesn't recognize 'left' and 'right' : [Attachment 220589] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 126611: text-emphasis-position CSS property doesn't recognize 'left' and
'right'
https://bugs.webkit.org/show_bug.cgi?id=126611

Attachment 220589: Patch
https://bugs.webkit.org/attachment.cgi?id=220589&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220589&action=review


> Source/WebCore/ChangeLog:15
> +	   are specified, we should draw as if the appropriate value was

if neither "left" nor "right" *is* specified, we should draw as if the
appropriate value *were*

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1469
> +    ASSERT(!((textEmphasisPosition & TextEmphasisPositionOver) &&
(textEmphasisPosition & TextEmphasisPositionUnder)));
> +    ASSERT(!((textEmphasisPosition & TextEmphasisPositionLeft) &&
(textEmphasisPosition & TextEmphasisPositionRight)));

Shame the types can't enforce this for us.

> Source/WebCore/rendering/InlineFlowBox.cpp:872
> +	   if (emphasisMarkIsAbove == (!lineStyle.isFlippedLinesWritingMode()))


No need for parens around !lineStyle.isFlippedLinesWritingMode()

> Source/WebCore/rendering/InlineTextBox.cpp:475
> +    if (!(emphasisPosition & TextEmphasisPositionLeft) && !(emphasisPosition
& TextEmphasisPositionRight))

An inline function would make this condition easier to understand.

> Source/WebCore/rendering/InlineTextBox.h:92
> +    bool emphasisMarkIsAbove(const RenderStyle&, bool& above) const;

Very confusing to have two bools returned. What do they each mean?

> Source/WebCore/rendering/style/RenderStyleConstants.h:508
> +enum TextEmphasisPositionItems {

TextEmphasisPositions?