[webkit-reviews] review granted: [Bug 56388] Implement the CSS3 line-box-contain property : [Attachment 86341] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dave Hyatt
<hyatt at apple.com>'s request for review:
Bug 56388: Implement the CSS3 line-box-contain property
https://bugs.webkit.org/show_bug.cgi?id=56388

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86341&action=review

r+ but I would like to see a property parsing test.

> Source/WebCore/GNUmakefile.am:1014
> +	   Source/WebCore/css/CSSLineBoxContainValue.cpp \
> +	Source/WebCore/css/CSSLineBoxContainValue.h \

Odd indentation here. Extra tab?

> Source/WebCore/css/CSSLineBoxContainValue.h:38
> +enum LineBoxContain { LineBoxContainNone = 0x0, LineBoxContainBlock = 0x1,
LineBoxContainInline = 0x2, LineBoxContainFont = 0x4, LineBoxContainGlyphs =
0x8,
> +			 LineBoxContainReplaced = 0x10, LineBoxContainInlineBox
= 0x20 };

I have a minor preference for 0, 1 << 0, 1 << 1 etc. Makes it super-obvious
that it's a bitmask.

I think you should also typedef the bitflags:

enum LineBoxContainFlags { ... }
typedef unsigned LineBoxContain

> Source/WebCore/css/CSSLineBoxContainValue.h:50
> +    unsigned value() const { return m_value; }

This could use the typedef then.

> Source/WebCore/platform/graphics/Font.h:72
> +    bool computeBounds;

It's not obvious to me what this means on a struct. It suggests that bounds
have to be computed, but by whom, and when? If it's true, does that affect the
other members?

> Source/WebCore/platform/graphics/FontFastPath.cpp:457
> +	   glyphOverflow->top = max<int>(glyphOverflow->top,
ceilf(-it.minGlyphBoundingBoxY()) - (glyphOverflow->computeBounds ? 0  :
fontMetrics().ascent()));
> +	   glyphOverflow->bottom = max<int>(glyphOverflow->bottom,
ceilf(it.maxGlyphBoundingBoxY()) - (glyphOverflow->computeBounds ? 0  :
fontMetrics().descent()));

Extra space before the :

> Source/WebCore/platform/graphics/mac/FontComplexTextMac.cpp:115
> +	   glyphOverflow->bottom = max<int>(glyphOverflow->bottom,
ceilf(controller.maxGlyphBoundingBoxY()) - (glyphOverflow->computeBounds ? 0  :
fontMetrics().descent()));

Ditto

> Source/WebCore/rendering/RootInlineBox.cpp:558
> +static void setAscentAndDescent(int& ascent, int& descent, int newAscent,
int newDescent, bool& ascentDescentSet)

I'd expect the out params at the end of the parameter list. Maybe rename this
to 'storeAscentAndDescent'?

> Source/WebCore/rendering/RootInlineBox.cpp:594
> +	   glyphOverflow = it == textBoxDataMap.end() ? 0 : &it->second.second;


Maybe assign "it == textBoxDataMap.end()" to a local variable?

> Source/WebCore/rendering/RootInlineBox.cpp:727
> +	   if (verticalAlign == SUB)
> +	       verticalPosition += fontSize / 5 + 1;
> +	   else if (verticalAlign == SUPER)
> +	       verticalPosition -= fontSize / 3 + 1;
> +	   else if (verticalAlign == TEXT_TOP)
> +	       verticalPosition += renderer->baselinePosition(baselineType(),
firstLine, lineDirection) - fontMetrics.ascent(baselineType());
> +	   else if (verticalAlign == MIDDLE)
> +	       verticalPosition += -static_cast<int>(fontMetrics.xHeight() / 2)
- renderer->lineHeight(firstLine, lineDirection) / 2 +
renderer->baselinePosition(baselineType(), firstLine, lineDirection);
> +	   else if (verticalAlign == TEXT_BOTTOM) {
> +	       verticalPosition += fontMetrics.descent(baselineType());
> +	       // lineHeight - baselinePosition is always 0 for replaced
elements (except inline blocks), so don't bother wasting time in that case.
> +	       if (!renderer->isReplaced() ||
renderer->isInlineBlockOrInlineTable())
> +		   verticalPosition -= (renderer->lineHeight(firstLine,
lineDirection) - renderer->baselinePosition(baselineType(), firstLine,
lineDirection));
> +	   } else if (verticalAlign == BASELINE_MIDDLE)
> +	       verticalPosition += -renderer->lineHeight(firstLine,
lineDirection) / 2 + renderer->baselinePosition(baselineType(), firstLine,
lineDirection);
> +	   else if (verticalAlign == LENGTH)
> +	       verticalPosition -=
renderer->style()->verticalAlignLength().calcValue(renderer->lineHeight(firstLi
ne, lineDirection));

Make this a switch? (Yeah, I know you just moved it).

> Source/WebCore/rendering/RootInlineBox.cpp:742
> +    unsigned lineBoxContain = renderer()->style()->lineBoxContain();

Could use the typedef here, and below.

> Source/WebCore/rendering/RootInlineBox.cpp:754
> +    // For now map "glyphs" to "font" in vertical text mode until the bounds
returned by glyphs aren't garbage.

Does that need a bug?

> Source/WebCore/rendering/style/RenderStyle.h:776
> +    unsigned lineBoxContain() { return rareInheritedData->m_lineBoxContain;
}

Should be const, and use the typedef.

> LayoutTests/ChangeLog:81
> +	   * platform/mac/fast/block/lineboxcontain/replaced-expected.png:
Added.
> +	   * platform/mac/fast/block/lineboxcontain/replaced-expected.txt:
Added.
> +

I think you should add a property parsing test also.