Apply a heuristic to first lines to avoid needlessly growing the line-height. This would stop the ugly rendering of first lines in books when a larger font is used with only capital letters (e.g., as a first letter or first sentence on the line).
Created attachment 85825 [details] Patch
Attachment 85825 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/InlineFlowBox.cpp:576: Should only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/rendering/InlineFlowBox.cpp:577: Should only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/rendering/InlineFlowBox.cpp:578: Should only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 3 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
<rdar://problem/7958703> Extra spacing below line with drop cap
Created attachment 85826 [details] Patch that passes style queue
Comment on attachment 85826 [details] Patch that passes style queue Clearing flag for now, since some layout tests fail and I want to study them and make sure it's ok before proceeding.
Created attachment 85851 [details] Patch
Attachment 85851 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/style/RenderStyle.cpp:399: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/rendering/RenderBlockLineLayout.cpp:367: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 2 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 85851 [details] Patch Clearing flag. Going to get some www-style discussion going about this.
Created attachment 86231 [details] Patch Not for review. Just testing builds.
Attachment 86231 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.am', u'Source/W..." exit_code: 1 Source/WebCore/rendering/style/RenderStyle.cpp:399: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/rendering/RenderBlockLineLayout.cpp:365: Should only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 2 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 86340 [details] Patch
Attachment 86340 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/RenderBlockLineLayout.cpp:365: Should only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/rendering/style/RenderStyle.cpp:399: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 2 in 79 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 86341 [details] Patch
Attachment 86341 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/RenderBlockLineLayout.cpp:365: Should only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/rendering/style/RenderStyle.cpp:399: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 2 in 79 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 86341 [details] Patch 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(firstLine, 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.
Fixed in r81684.
http://trac.webkit.org/changeset/81684 might have broken EFL Linux Release (Build)