[webkit-reviews] review granted: [Bug 51267] Text emphasis marks should not appear over characters that have ruby annotations : [Attachment 78078] Suppress emphasis marks over text that has ruby annotations

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted mitz at webkit.org's request for review:
Bug 51267: Text emphasis marks should not appear over characters that have ruby
annotations
https://bugs.webkit.org/show_bug.cgi?id=51267

Attachment 78078: Suppress emphasis marks over text that has ruby annotations
https://bugs.webkit.org/attachment.cgi?id=78078&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=78078&action=review

> WebCore/rendering/InlineTextBox.cpp:405
> +bool InlineTextBox::getEmphasisMarkPosition(RenderStyle* style,
TextEmphasisPosition& emphasisPosition) const

This function could use a tiny bit of “why” comments. Otherwise, you can see
those return true and false and it’s hard to judge if they are right or wrong.
Some brief statement of what significance the presence or absence of a line box
has, and what significance the ruby has, and also why we needn’t even check for
a ruby run if the container is not a ruby base.

> WebCore/rendering/InlineTextBox.cpp:409
> +    TextEmphasisMark emphasisMark = style->textEmphasisMark();
> +    if (emphasisMark == TextEmphasisMarkNone)
> +	   return false;

I don’t think the local variable is needed here.

> WebCore/rendering/InlineTextBox.cpp:423
> +    RenderRubyRun* rubyRun =
static_cast<RenderRubyRun*>(containingBlock->parent());
> +    RenderRubyText* rubyText = rubyRun->rubyText();

No need for the local variable rubyRun here. The expression
containingBlock->parent() is used twice, but you chose to repeat it. If you did
make a local variable, I suggest it be before the typecast, but none is OK with
me.