[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 #159431|review?                     |review-
               Flag|                            |




--- Comment #6 from Dave Hyatt <hyatt at apple.com>  2012-08-23 11:47:45 PST ---
(From update of attachment 159431)
View in context: https://bugs.webkit.org/attachment.cgi?id=159431&action=review

r- (Comments below)

> Source/WebCore/rendering/InlineFlowBox.cpp:166
>              RenderStyle* childStyle = child->renderer()->style(isFirstLineStyle());
> +            if (renderer()->inRenderFlowThread()) {
> +                RenderFlowThread* flowThread = renderer()->enclosingRenderFlowThread();
> +                childStyle = flowThread->renderStyleForLine(this);
> +            }

I would bundle all of this together into a styleInRegion() function on InlineBox. Then the code here would just read:

RenderStyle* childStyle = child->styleInRegion(child->region(), isFirstLineStyle());

Technically if the first line style bit is properly set on the child box, you don't even need to pass in isFirstLineStyle().

In other words, let's separate fetching the style for a specific region from the function that figures out what region the child is in. I'll explain why below.

> Source/WebCore/rendering/InlineFlowBox.cpp:832
>      RenderStyle* style = textBox->renderer()->style(isFirstLineStyle());
>      
> +    if (renderer()->inRenderFlowThread()) {
> +        RenderFlowThread* flowThread = renderer()->enclosingRenderFlowThread();
> +        style = flowThread->renderStyleForLine(this);
> +    }

Same here. All of this could just be inside InlineBox helpers, since you just did the exact same thing as above.

> Source/WebCore/rendering/RenderFlowThread.cpp:405
> +    LayoutUnit offsetFromLogicalTop = toRenderBlock(renderer)->offsetFromLogicalTopOfFirstPage();
> +    if (flowBox->prevLineBox())
> +        offsetFromLogicalTop += flowBox->prevLineBox()->logicalTop();

This is not correct. You're treating the first line as though it started right at the very top of the RenderBlock, but there could be border/padding/floats, etc., any # of things that push even the first line down. Why aren't you just adding in your own logicalTop? I don't understand why the previous line box has to be involved at all.

Also, this is very fragile code, since it only works during layout. We don't know the offsetFromLogicalTopOfFirstPage at paint time for example (we'll have the RenderRegion in the painting info in that case).

This is why I'd like to see styleInRegion() and region() as two different helpers on InlineBox. Then at paint time, you'll be able to still ask for the region-specific style and just pass in the RenderRegion from the painting info (since you won't be able to rely on offsetFromLogicalTopOfFirstPage).

> Source/WebCore/rendering/RenderFlowThread.cpp:409
> +    return region->computeStyleInRegion(renderer).get();

Where is first line styling taken into account? It looks like it's being dropped on the floor.

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