[webkit-reviews] review denied: [Bug 98317] Only measure text once instead of twice when performing line layout : [Attachment 167001] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has denied Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 98317: Only measure text once instead of twice when performing line layout
https://bugs.webkit.org/show_bug.cgi?id=98317

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

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167001&action=review


> Source/WebCore/rendering/RenderBlockLineLayout.cpp:711
> +    bool isKerningEnabled = renderer->style()->font().typesettingFeatures()
& Kerning;

This should use style(lineInfo.isFirstLine()). Since we need the same Font in
the block above, I’d just move the font local out of that block and use it
there and again here. I don’t we’re not consistent about naming of boolean
variables, but I find that a name that is a statement, like “kerningIsEnabled”,
reads better than a name that is a question like “isKerningEnabled” once you
use it in an expression: if (kerningIsEnabled)…

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:713
> +    if (!lineBox->fitsToGlyphs() && !textMeasures.isEmpty()) {

Needs a comment about why you’re checking fitsToGlyphs().

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:715
> +	   for (size_t i = 0; i < textMeasures.size() && lastEndOffset <
run->m_stop; i++) {

It can be more efficient to get the size once. The new style for doing so is
    for (size_t i = 0, size = textMeasures.size(); i < size && lastEndOffset <
run->m_stop; ++i) {
(we also use pre-increment as a rule).

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:734
> +	       // we don't have enough cached data, we'll measure again

Please format this as a sentence (start with a capital letter and end with a
period).

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2078
> +static ALWAYS_INLINE float textWidth(RenderText* text, unsigned from,
unsigned len, const Font& font, float xPos, bool isFixedPitch, bool
collapseWhiteSpace, HashSet<const SimpleFontData*>* fallbackFonts = 0,
TextLayout* layout = 0)

What’s with the change from inline to ALWAYS_INLINE here?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2263
> +    entry.fallbackFonts.clear();

What’s happening here? I don’t think we want to be copying HashSets all the
time. I think instead the TextMeasureEntry that the code below updates should
already by in the vector. Then if we don’t need it, we can delete it (or reuse
it, I suppose).