[Webkit-unassigned] [Bug 6274] text repainting does not account for glyphs which draw outside the typographic bounds of the font
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Apr 5 16:41:13 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=6274
--- Comment #25 from mitz at webkit.org 2010-04-05 16:41:12 PST ---
(From update of attachment 52577)
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 57096)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,104 @@
> +2010-04-05 Enrica Casucci <enrica at apple.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Text repainting does not account for glyphs which draw outside the typographic bounds of the font (6274).
> + <rdar://problem/6649734>
> + <https://bugs.webkit.org/show_bug.cgi?id=6274>
> +
> + In order to be able to handle successfully this case, it is necessary to change the glyph width cache to store
> + the bounding box for the glyph instead of the simply caching the glyph width.
> + Retrieving the bounding box for the glyph is expensive, therefore we do it only
> + when we are rendering text using the complex text path to minimize the performance impact.
> + To support characters with stacked diacritics, the method canUseGlyphCache has been modified to
> + return false for the range of characters with stacked diacritics.
> +
> + The original version of this patch has been written by Dan Bernstein.
> +
> + Test: fast/repaint/stacked-diacritics.html
> +
> + * WebCore.base.exp:
> + * platform/graphics/Font.cpp:
> + (WebCore::Font::floatWidth):
> + * platform/graphics/Font.h:
> + (WebCore::GlyphOverflow::GlyphOverflow):
> + (WebCore::Font::width):
> + * platform/graphics/FontFastPath.cpp:
> + (WebCore::Font::canUseGlyphCache):
> + (WebCore::Font::floatWidthForSimpleText):
> + * platform/graphics/GlyphWidthMap.cpp:
> + (WebCore::GlyphWidthMap::locatePageSlowCase):
> + * platform/graphics/GlyphWidthMap.h:
> + (WebCore::GlyphWidthMap::metricsForGlyph):
> + (WebCore::GlyphWidthMap::widthForGlyph):
> + (WebCore::GlyphWidthMap::setMetricsForGlyph):
> + (WebCore::GlyphWidthMap::GlyphWidthPage::metricsForGlyph):
> + (WebCore::GlyphWidthMap::GlyphWidthPage::setMetricsForGlyph):
> + (WebCore::GlyphWidthMap::GlyphWidthPage::setMetricsForIndex):
> + * platform/graphics/SimpleFontData.cpp:
> + (WebCore::SimpleFontData::platformGlyphInit):
> + * platform/graphics/SimpleFontData.h:
> + (WebCore::):
> + (WebCore::SimpleFontData::widthForGlyph):
> + (WebCore::SimpleFontData::metricsForGlyph):
> + * platform/graphics/WidthIterator.cpp:
> + (WebCore::WidthIterator::WidthIterator):
> + (WebCore::WidthIterator::advance):
> + * platform/graphics/WidthIterator.h:
> + (WebCore::WidthIterator::maxGlyphBoundingBoxY):
> + (WebCore::WidthIterator::minGlyphBoundingBoxY):
> + (WebCore::WidthIterator::firstGlyphOverflow):
> + (WebCore::WidthIterator::lastGlyphOverflow):
> + * platform/graphics/mac/ComplexTextController.cpp:
> + (WebCore::ComplexTextController::ComplexTextController):
> + (WebCore::ComplexTextController::adjustGlyphsAndAdvances):
> + * platform/graphics/mac/ComplexTextController.h:
> + (WebCore::ComplexTextController::minGlyphBoundingBoxX):
> + (WebCore::ComplexTextController::maxGlyphBoundingBoxX):
> + (WebCore::ComplexTextController::minGlyphBoundingBoxY):
> + (WebCore::ComplexTextController::maxGlyphBoundingBoxY):
> + * platform/graphics/mac/FontComplexTextMac.cpp:
> + (WebCore::Font::floatWidthForComplexText):
> + * platform/graphics/mac/SimpleFontDataMac.mm:
> + (WebCore::SimpleFontData::platformMetricsForGlyph):
> + * platform/graphics/win/FontWin.cpp:
> + (WebCore::Font::floatWidthForComplexText):
> + * platform/graphics/win/SimpleFontDataCGWin.cpp:
> + (WebCore::SimpleFontData::platformMetricsForGlyph):
> + * platform/graphics/win/SimpleFontDataWin.cpp:
> + (WebCore::SimpleFontData::metricsForGDIGlyph):
> + * platform/graphics/win/UniscribeController.cpp:
> + (WebCore::UniscribeController::UniscribeController):
> + (WebCore::UniscribeController::shapeAndPlaceItem):
> + * platform/graphics/win/UniscribeController.h:
> + (WebCore::UniscribeController::minGlyphBoundingBoxX):
> + (WebCore::UniscribeController::maxGlyphBoundingBoxX):
> + (WebCore::UniscribeController::minGlyphBoundingBoxY):
> + (WebCore::UniscribeController::maxGlyphBoundingBoxY):
> + * rendering/InlineFlowBox.cpp:
> + (WebCore::InlineFlowBox::placeBoxesHorizontally):
> + (WebCore::InlineFlowBox::computeLogicalBoxHeights):
> + (WebCore::InlineFlowBox::computeVerticalOverflow):
> + * rendering/InlineTextBox.cpp:
> + (WebCore::InlineTextBox::setFallbackFonts):
> + (WebCore::InlineTextBox::fallbackFonts):
> + (WebCore::InlineTextBox::setGlyphOverflow):
> + (WebCore::InlineTextBox::glyphOverflow):
> + * rendering/InlineTextBox.h:
> + (WebCore::InlineTextBox::clearGlyphOverflowAndFallbackFontMap):
> + * rendering/RenderBlockLineLayout.cpp:
> + (WebCore::RenderBlock::computeHorizontalPositionsForLine):
> + (WebCore::RenderBlock::createLineBoxesForResolver):
> + * rendering/RenderText.cpp:
> + (WebCore::RenderText::RenderText):
> + (WebCore::RenderText::styleDidChange):
> + (WebCore::RenderText::widthFromCache):
> + (WebCore::RenderText::trimmedPrefWidths):
> + (WebCore::RenderText::calcPrefWidths):
> + (WebCore::RenderText::setText):
> + (WebCore::RenderText::width):
> + * rendering/RenderText.h:
I think some specific comments on at least some of the functions would be
helpful.
> + float floatWidth(const TextRun&, HashSet<const SimpleFontData*>* fallbackFonts = 0, GlyphOverflow* glyphOverflow = 0) const;
Don’t need the argument name “glyphOverflow” here.
> @@ -234,6 +234,11 @@ bool Font::canUseGlyphCache(const TextRu
> if (c <= 0x194F)
> return false;
>
> + if (c < 0x1E00)
> + continue;
> + if (c <= 0x2000)
> + return false;
> +
It is important to add a comment about why this range is included (even more
than any of the other ranges, because the reason it’s included is unique).
There is also some trailing whitespace up there.
> class GlyphWidthMap : public Noncopyable {
Should probably rename this (and related files) GlyphMetricsMap.
> +ALWAYS_INLINE GlyphMetrics SimpleFontData::metricsForGlyph(Glyph glyph, GlyphMetricsMode metricsMode) const
> {
> - float width = m_glyphToWidthMap.widthForGlyph(glyph);
> - if (width != cGlyphWidthUnknown)
> - return width;
> -
> - width = platformWidthForGlyph(glyph);
> - m_glyphToWidthMap.setWidthForGlyph(glyph, width);
> -
> - return width;
> + GlyphMetrics metrics = m_glyphToWidthMap.metricsForGlyph(glyph);
> + if (metrics.horizontalAdvance != cGlyphWidthUnknown)
> + return metrics;
> +
> + metrics = platformMetricsForGlyph(glyph, metricsMode);
> + m_glyphToWidthMap.setMetricsForGlyph(glyph, metrics);
> +
> + return metrics;
> }
This confuses me. What’s to stop us from caching “width only” metrics in the
map at first, and then on a subsequent request for “bounding box” metrics
return the cached value instead of actually getting the bounding box? This
seems to be assuming that the fast path and the complex path will never hit the
same glyph. I don’t think it’s true, especially since the complex path can be
forced for any text by specifying text-rendering: optimizeLegibility.
> + , m_maxGlyphBoundingBoxY(numeric_limits<float>::min())
> + , m_minGlyphBoundingBoxY(numeric_limits<float>::max())
> + , m_firstGlyphOverflow(0)
> + , m_lastGlyphOverflow(0)
Seems like you don’t need this (nor any of the changes to WidthIterator) now
that this is limited to the complex code path.
Seeing as you have kept that code in, and just by avoiding actually getting the
glyph bounds you managed to have no performance impact on the fast path, I am
thinking that perhaps instead of forcing the complex path for the range of
glyphs with stacked diacritics, you could still use the fast path for those,
but get and use the real bounding rects in that case. This would essentially
make canUseGlyphCache() select between three modes: fast, fast with real glyph
bounds, and complex. For “fast with real glyph bounds”, Font::floatWidth()
would pass the glyphOverflow pointer through to floatWidthForSimpleText(). For
plain “fast”, it would pass 0, and the will instruct WidthIterator down the
line to not get real bounding boxes.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list