[webkit-reviews] review denied: [Bug 94472] [CSSRegions]Add support for text-shadow in region styling : [Attachment 160697] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Andrei Onea <onea at adobe.com>'s request
for review:
Bug 94472: [CSSRegions]Add support for text-shadow in region styling
https://bugs.webkit.org/show_bug.cgi?id=94472

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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()->offsetFromLogica
lTopOfFirstPage());

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).