[webkit-reviews] review denied: [Bug 95559] [CSS Regions] Implement region styling for font-size : [Attachment 161694] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Andrei Bucur <abucur at adobe.com>'s
request for review:
Bug 95559: [CSS Regions] Implement region styling for font-size
https://bugs.webkit.org/show_bug.cgi?id=95559

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=161694&action=review


r-

> Source/WebCore/rendering/RenderBlock.cpp:6136
> +    updateRegionStyleForOffset(logicalHeight() + paginationStrut());

Add an if (inRenderFlowThread()) check to avoid the function call for normal
layout.

> Source/WebCore/rendering/RenderBlock.cpp:7180
> -    return rootBox->paginatedLineWidth() !=
availableLogicalWidthForContent(rootBox->lineTopWithLeading() + lineDelta);
> +    return rootBox->paginatedLineWidth() !=
availableLogicalWidthForContent(rootBox->lineBottomWithLeading() + lineDelta);

This change is questionable. We use the top of lines for floats, and the intent
here is to match that heuristic. Not sure why a change is necessary here?

> Source/WebCore/rendering/RenderBlock.cpp:7211
> +    // HACK to skip the regions with height 0.
> +    LayoutUnit pageLogicalHeight = pageLogicalHeightForOffset(blockOffset);
> +    if (!pageLogicalHeight)
> +	   return;

Can't we just fix regionAtBlockOffset to never return a zero-height region?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1287
> +    // FIXME: styleToUse can be cached because no region stylable property
is used below

I don't understand this FIXME.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1389
> +		       // If the block got shifted for pagination; we need to
take the strut into consideration.
> +		       if (adjustment || oldPaginationStrut !=
newPaginationStrut) {

Not quite following this. The strut value changed but the adjustment is 0? When
does that occur?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1395
> +			   if (availableLogicalWidthForLine(oldLogicalHeight +
adjustment + newPaginationStrut, layoutState.lineInfo().isFirstLine()) !=
oldLineWidth) {

This seems confusing to me. Why wasn't the newPaginationStrut factored into the
adjustment if you're actually adding it in here?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1681
> +		   updateRegionStyleForOffset(curr->lineBottomWithLeading());
> +		   if (m_lineHeight == -1) {
> +		       layoutState.markForFullLayout();
> +		       break;
> +		   }

You're probably going to want updateRegionStyleForOffset to return a hint
similar to RenderStyle::diff that helps you know what you have to do, e.g., a
repaint, a relayout, etc. Just checking if line height cleared is way too
specific to font-size, and you should be thinking more generally here.

I'd pick lineTopWithLeading rather than the bottom, unless you have a
compelling reason why bottom should be used.

> Source/WebCore/rendering/RenderText.cpp:203
> +void RenderText::styleInRegionDidChange(const RenderStyle* oldStyle)
> +{
> +    RenderObject::styleInRegionDidChange(oldStyle);
> +    RenderBlock* containingBlock = this->containingBlock();
> +    ASSERT(containingBlock);
> +    // FIXME: Is this accurate? We want to invalidate the preferred width
only at layout time.
> +    if (!containingBlock->needsLayout())
> +	   return;
> +    if (oldStyle->fontSize() != style()->fontSize())
> +	   setPreferredLogicalWidthsDirty(true, MarkOnlyThis);
> +}
> +

I really want to avoid class-specific methods for everything like this. You
need to try to implement something that leverages the style diff stuff we've
already got to know if something is a repaint vs. a layout etc. and then do the
right thing based off that hint.