[webkit-reviews] review denied: [Bug 94472] [CSSRegions]Add support for text-shadow in region styling : [Attachment 159431] 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 159431: Patch
https://bugs.webkit.org/attachment.cgi?id=159431&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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.