[Webkit-unassigned] [Bug 49334] Implement Default Ruby Overhang Behavior

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Nov 17 23:33:50 PST 2010


https://bugs.webkit.org/show_bug.cgi?id=49334


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74032|review?                     |review-
               Flag|                            |




--- Comment #2 from Dave Hyatt <hyatt at apple.com>  2010-11-17 23:33:50 PST ---
(From update of attachment 74032)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list