[Webkit-unassigned] [Bug 94472] [CSSRegions]Add support for text-shadow in region styling

[bugzilla-daemon at webkit.org]

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


Dave Hyatt <hyatt at apple.com> changed:

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




--- Comment #10 from Dave Hyatt <hyatt at apple.com>  2012-08-27 12:35:47 PST ---
(From update of attachment 160697)
View in context: https://bugs.webkit.org/attachment.cgi?id=160697&action=review

Looking good! Just a few suggestions.

> Source/WebCore/rendering/InlineBox.cpp:257
> +RenderStyle* InlineBox::styleInRegion(RenderRegion* region, bool isFirstLine)

You don't need the second argument. The style bit has been set on the box itself at all the call sites I see you using this function. So you can just use the member on InlineBox.

> Source/WebCore/rendering/InlineBox.cpp:268
> +RenderRegion* InlineBox::region()

Let's rename this to "regionDuringLayout()" to make it clear you can't call it at other times.

> Source/WebCore/rendering/InlineBox.cpp:274
> +    return flowThread->renderRegionForLine(renderer()->containingBlock()->offsetFromLogicalTopOfFirstPage());

You should add in the logicalHeight() of the containing block, so that you're at least at the bottom of the previous line.

> Source/WebCore/rendering/InlineBox.h:103
> +    RenderRegion* region();

regionDuringLayout() rename. Also let's add a comment that indicates that this assumes the box has not been positioned yet, and that it will use the block's current logical height (similar to what floats do).

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