[webkit-reviews] review denied: [Bug 49334] Implement Default Ruby Overhang Behavior : [Attachment 74032] Implement default Ruby Overhang behavior.

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Eric Mader <emader at apple.com>'s request
for review:
Bug 49334: Implement Default Ruby Overhang Behavior
https://bugs.webkit.org/show_bug.cgi?id=49334

Attachment 74032: Implement default Ruby Overhang behavior.
https://bugs.webkit.org/attachment.cgi?id=74032&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=74032&action=review

I guess I wouldn't bother generating pixel results.  The committer will
probably just have to do it.  I see a bunch of new pixel results for tests that
have nothing to do with ruby.

> WebCore/ChangeLog:10
> +	   (WebCore::RenderBlock::findNextLineBreak): Compute overhang margtins
for RenderRubyRun objects.

Typo. margtins -> margins.

> WebCore/rendering/RenderBlockLineLayout.cpp:362
> +#if 0
> +	       // This call resets ruby overhang calculations
> +	       // done in findNextLineBreak().
>	       renderBox->computeLogicalWidth();
> +#endif

Don't #if 0 this.  Just take the code out.

> WebCore/rendering/RenderBlockLineLayout.cpp:1612
> +		       RenderRubyRun* rr = static_cast<RenderRubyRun*> (o);

You have an extra space between <RenderRubyRun*> and (o).

> WebCore/rendering/RenderBlockLineLayout.cpp:1615
> +		       rr->setOverhangMargins(last, bidiNext(this, o));
> +		   }

I don't see a need for a local here, since you don't use it for anything.  You
can just say

static_cast<RenderRubyRun*>(o)->setOverhangMargins(last, bidiNext(this, o));

> WebCore/rendering/RenderRubyRun.cpp:67
> +    int rubyLength = text? text->maxPreferredLogicalWidth() : 0;
> +    int baseLength = base? base->maxPreferredLogicalWidth() : 0;

It seems like you should just be using logicalWidth() here, but maybe I'm
missing something.

> WebCore/rendering/RenderRubyRun.cpp:73
> +#if 0
> +    // Remove height of ruby text from overall height.
> +    if (text)
> +	   setMarginBefore(-text->height());
> +#endif

No #if 0 code please.  Just take it out.

> WebCore/rendering/RenderRubyRun.cpp:83
> +	       setMarginStart(marginLeft() - (overhang / 2));
> +	   

marginLeft() is wrong for vertical text.  It should be marginStart().

> WebCore/rendering/RenderRubyRun.cpp:85
> +	   if (nextIsText)
> +	       setMarginEnd(marginRight() - (overhang / 2));

marginRight() should be marginEnd().

> WebCore/rendering/RenderRubyRun.cpp:262
> +    

Extra whitespace introduced in two places in RenderRubyRun.  Look at the pretty
diff to see it so you can get rid of it.

> WebCore/rendering/RenderRubyRun.h:77
> +    

Accidental extra whitespace here.