WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
REOPENED
6274
text repainting does not account for glyphs which draw outside the typographic bounds of the font
https://bugs.webkit.org/show_bug.cgi?id=6274
Summary
text repainting does not account for glyphs which draw outside the typographi...
Alexey Proskuryakov
Reported
2005-12-28 13:36:03 PST
This test tracks progress of an XMLHttpRequest by modifying p.firstChild.data. At the end, one can see two dots remaining from "g" letters that weren't completely erased.
Attachments
screenshot
(1.70 KB, image/png)
2005-12-29 14:38 PST
,
Alexey Proskuryakov
no flags
Details
Test case
(177 bytes, text/html; charset=macintosh)
2006-02-17 05:43 PST
,
mitz
no flags
Details
First cut
(34.96 KB, patch)
2009-08-28 13:27 PDT
,
mitz
no flags
Details
Formatted Diff
Diff
Slightly better
(39.70 KB, patch)
2009-08-28 17:36 PDT
,
mitz
no flags
Details
Formatted Diff
Diff
WIP
(44.90 KB, patch)
2009-08-29 15:05 PDT
,
mitz
no flags
Details
Formatted Diff
Diff
WIP
(48.70 KB, patch)
2009-08-29 18:44 PDT
,
mitz
no flags
Details
Formatted Diff
Diff
WIP
(52.92 KB, patch)
2009-08-29 19:59 PDT
,
mitz
no flags
Details
Formatted Diff
Diff
patch
(56.01 KB, patch)
2010-04-05 14:26 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Patch2
(101.07 KB, patch)
2010-04-05 14:59 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Patch3
(126.32 KB, patch)
2010-04-06 14:39 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Patch4
(125.47 KB, patch)
2010-04-06 15:46 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Patch5
(126.03 KB, patch)
2010-04-06 16:33 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Patch6
(126.42 KB, patch)
2010-04-06 17:17 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Patch7
(136.24 KB, patch)
2010-04-06 23:56 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Patch8
(136.86 KB, patch)
2010-04-07 06:55 PDT
,
Enrica Casucci
no flags
Details
Formatted Diff
Diff
Account for overflow of glyphs for U+1E00..U+2000 without sending them through the complex text code path
(56.03 KB, patch)
2010-04-28 22:40 PDT
,
mitz
no flags
Details
Formatted Diff
Diff
Account for overflow of glyphs for U+1E00..U+2000 without sending them through the complex text code path
(56.31 KB, patch)
2010-04-29 23:27 PDT
,
mitz
no flags
Details
Formatted Diff
Diff
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
mitz
Comment 1
2005-12-29 14:34:31 PST
This is font-dependent. I was able to reproduce with Lucida Grande 16 selected as standard font.
Alexey Proskuryakov
Comment 2
2005-12-29 14:38:00 PST
Created
attachment 5357
[details]
screenshot ...and my anti-alias setting is Standard (best for CRT)
mitz
Comment 3
2006-02-17 05:43:41 PST
Created
attachment 6563
[details]
Test case With Zapfino the effect is even more dramatic.
mitz
Comment 4
2006-03-25 11:43:31 PST
The fix for
bug 7301
includes a mechanism that could be used to fix this bug, if you figure out how to tell by how much a given font can overflow.
mitz
Comment 5
2006-04-22 02:00:10 PDT
***
Bug 8527
has been marked as a duplicate of this bug. ***
Matt Lilek
Comment 6
2007-06-15 13:35:16 PDT
***
Bug 14174
has been marked as a duplicate of this bug. ***
mitz
Comment 7
2008-07-16 14:24:25 PDT
***
Bug 20060
has been marked as a duplicate of this bug. ***
Eric Seidel (no email)
Comment 8
2008-10-04 01:15:45 PDT
***
Bug 20420
has been marked as a duplicate of this bug. ***
Eric Seidel (no email)
Comment 9
2008-10-04 01:20:22 PDT
Changing the title to reflect more accurately what is going on here (at least according to mitz).
mitz
Comment 10
2008-11-11 07:24:38 PST
***
Bug 22175
has been marked as a duplicate of this bug. ***
David Kilzer (:ddkilzer)
Comment 11
2009-03-05 09:26:20 PST
<
rdar://problem/6649734
>
mitz
Comment 12
2009-08-28 13:27:12 PDT
Created
attachment 38749
[details]
First cut Mac-only and “fast path”-only, not performance-tested, leaves some room for optimization and clean-up.
mitz
Comment 13
2009-08-28 17:36:12 PDT
Created
attachment 38766
[details]
Slightly better Works with custom fonts, slightly cleaned up, fixes a bug in WidthIterator::advance().
mitz
Comment 14
2009-08-29 15:05:40 PDT
Created
attachment 38776
[details]
WIP Works on Windows, fixes a couple of bugs and tries to be faster.
mitz
Comment 15
2009-08-29 15:10:33 PDT
Vertical overflow is very common, especially in small font sizes, because the descender height is rounded down to an integer, so descenders actually overflow by a fraction of a pixel.
mitz
Comment 16
2009-08-29 18:44:52 PDT
Created
attachment 38779
[details]
WIP Added overflow calculation in the Core Text code path.
mitz
Comment 17
2009-08-29 19:59:27 PDT
Created
attachment 38783
[details]
WIP Added overflow calculation in the Uniscribe code path.
mitz
Comment 18
2009-12-14 14:51:46 PST
***
Bug 32506
has been marked as a duplicate of this bug. ***
Enrica Casucci
Comment 19
2010-04-05 14:26:49 PDT
Created
attachment 52574
[details]
patch The patch does not contain a test case, because I'm working on it. I'd like to get it reviewed meanwhile.
Enrica Casucci
Comment 20
2010-04-05 14:31:49 PDT
Comment on
attachment 52574
[details]
patch forgot to mark it as patch
Enrica Casucci
Comment 21
2010-04-05 14:59:16 PDT
Created
attachment 52577
[details]
Patch2 Added layout test.
WebKit Review Bot
Comment 22
2010-04-05 15:21:46 PDT
Attachment 52574
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/1688007
WebKit Review Bot
Comment 23
2010-04-05 16:03:47 PDT
Attachment 52577
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/1687016
WebKit Review Bot
Comment 24
2010-04-05 16:30:36 PDT
Attachment 52577
[details]
did not build on gtk: Build output:
http://webkit-commit-queue.appspot.com/results/1685035
mitz
Comment 25
2010-04-05 16:41:12 PDT
Comment on
attachment 52577
[details]
Patch2
> Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 57096) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,104 @@ > +2010-04-05 Enrica Casucci <
enrica@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.
Enrica Casucci
Comment 26
2010-04-05 18:53:10 PDT
(In reply to
comment #25
)
> (From update of
attachment 52577
[details]
) > > Index: WebCore/ChangeLog > > =================================================================== > > --- WebCore/ChangeLog (revision 57096) > > +++ WebCore/ChangeLog (working copy) > > @@ -1,3 +1,104 @@ > > +2010-04-05 Enrica Casucci <
enrica@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.
Will do.
> > > + float floatWidth(const TextRun&, HashSet<const SimpleFontData*>* fallbackFonts = 0, GlyphOverflow* glyphOverflow = 0) const; > > Don’t need the argument name “glyphOverflow” here.
Ok,
> > > @@ -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. >
Will do,
> > > class GlyphWidthMap : public Noncopyable { > > Should probably rename this (and related files) GlyphMetricsMap. >
I know, it is just a pain :-)
> > +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. >
Then I'm not sure I've fully understood how this code works.
> > + , 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.
I think I've tried that, and still had perf issues. We'll talk in person about this. Thanks.
Enrica Casucci
Comment 27
2010-04-06 14:39:14 PDT
Created
attachment 52670
[details]
Patch3 Missing windows layout tests results. Creating now.
WebKit Review Bot
Comment 28
2010-04-06 14:45:26 PDT
Attachment 52670
[details]
did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.cpp WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.h WebCore/platform/graphics/SimpleFontData.h:30: Alphabetical sorting problem. [build/include_order] [4] WebCore/platform/graphics/FontFastPath.cpp:237: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/GlyphMetricsMap.h:51: More than one command on the same line in if [whitespace/parens] [4] WebCore/platform/graphics/GlyphMetricsMap.cpp:37: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
Early Warning System Bot
Comment 29
2010-04-06 14:51:26 PDT
Attachment 52670
[details]
did not build on qt: Build output:
http://webkit-commit-queue.appspot.com/results/1553318
mitz
Comment 30
2010-04-06 15:15:56 PDT
Comment on
attachment 52670
[details]
Patch3
> -const float cGlyphWidthUnknown = -1; > +const float cGlyphSizeUnknown = -1;
NaN might be a better choice here.
> #include "GlyphPageTreeNode.h" > -#include "GlyphWidthMap.h" > +#include "GlyphMetricsMap.h"
These are not in ASCII order now.
> + if ((metricsMode == GlyphWidthOnly && metrics.horizontalAdvance != cGlyphSizeUnknown) || (metricsMode == GlyphBoundingBox && metrics.boundingBox.width() != cGlyphSizeUnknown && metrics.boundingBox.height() != cGlyphSizeUnknown)) > + return metrics;
Checking bot width() and height() seems like an overkill.
> - width = fontData->widthForGlyph(glyph); > + width = fontData->metricsForGlyph(glyph, GlyphWidthOnly).horizontalAdvance;
This change seems unnecessary. widthForGlyph does just what you want.
> + if (metricsMode == GlyphBoundingBox) {
I would write this as an early return.
> + if (metricsMode == GlyphBoundingBox) {
Ditto. The rendering part of the code needs to be reviewed by someone other than myself, since I wrote it.
WebKit Review Bot
Comment 31
2010-04-06 15:30:41 PDT
Attachment 52670
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/1680092
WebKit Review Bot
Comment 32
2010-04-06 15:37:06 PDT
Attachment 52670
[details]
did not build on gtk: Build output:
http://webkit-commit-queue.appspot.com/results/1596273
Enrica Casucci
Comment 33
2010-04-06 15:38:45 PDT
(In reply to
comment #30
)
> (From update of
attachment 52670
[details]
) > > -const float cGlyphWidthUnknown = -1; > > +const float cGlyphSizeUnknown = -1;
> Ok.
> NaN might be a better choice here. > > > #include "GlyphPageTreeNode.h" > > -#include "GlyphWidthMap.h" > > +#include "GlyphMetricsMap.h" > > These are not in ASCII order now. >
Already fixed.
> > + if ((metricsMode == GlyphWidthOnly && metrics.horizontalAdvance != cGlyphSizeUnknown) || (metricsMode == GlyphBoundingBox && metrics.boundingBox.width() != cGlyphSizeUnknown && metrics.boundingBox.height() != cGlyphSizeUnknown)) > > + return metrics; > > Checking bot width() and height() seems like an overkill.
> Checking only for width.
> > - width = fontData->widthForGlyph(glyph); > > + width = fontData->metricsForGlyph(glyph, GlyphWidthOnly).horizontalAdvance; > > This change seems unnecessary. widthForGlyph does just what you want. > > > + if (metricsMode == GlyphBoundingBox) {
I don't see the advantage here. We still need to have two return statements.
> > I would write this as an early return. > > > + if (metricsMode == GlyphBoundingBox) { > > Ditto. > > The rendering part of the code needs to be reviewed by someone other than > myself, since I wrote it.
Enrica Casucci
Comment 34
2010-04-06 15:46:59 PDT
Created
attachment 52676
[details]
Patch4
WebKit Review Bot
Comment 35
2010-04-06 15:54:04 PDT
Attachment 52676
[details]
did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.cpp WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.h WebCore/platform/graphics/FontFastPath.cpp:237: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/GlyphMetricsMap.cpp:37: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 2 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
Enrica Casucci
Comment 36
2010-04-06 16:33:56 PDT
Created
attachment 52682
[details]
Patch5 Hopefully the laste one :-)
WebKit Review Bot
Comment 37
2010-04-06 16:35:26 PDT
Attachment 52676
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/1671143
WebKit Review Bot
Comment 38
2010-04-06 16:41:12 PDT
Attachment 52682
[details]
did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.cpp WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.h WebCore/platform/graphics/FontFastPath.cpp:237: One space before end of line comments [whitespace/comments] [5] WebCore/platform/graphics/GlyphMetricsMap.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 39
2010-04-06 17:16:14 PDT
Attachment 52676
[details]
did not build on gtk: Build output:
http://webkit-commit-queue.appspot.com/results/1651219
Enrica Casucci
Comment 40
2010-04-06 17:17:14 PDT
Created
attachment 52684
[details]
Patch6 Fixed for chromium.
WebKit Review Bot
Comment 41
2010-04-06 17:22:24 PDT
Attachment 52684
[details]
did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.cpp WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.h WebCore/platform/graphics/FontFastPath.cpp:237: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 42
2010-04-06 18:23:46 PDT
Attachment 52684
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/1650217
WebKit Review Bot
Comment 43
2010-04-06 19:57:36 PDT
Attachment 52684
[details]
did not build on gtk: Build output:
http://webkit-commit-queue.appspot.com/results/1570295
Enrica Casucci
Comment 44
2010-04-06 23:56:24 PDT
Created
attachment 52708
[details]
Patch7 Fixed build errors for other platforms.
WebKit Review Bot
Comment 45
2010-04-07 00:01:19 PDT
Attachment 52708
[details]
did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.cpp WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.h WebCore/platform/graphics/FontFastPath.cpp:237: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 46
2010-04-07 00:28:51 PDT
Attachment 52708
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/1568298
WebKit Review Bot
Comment 47
2010-04-07 01:00:27 PDT
Attachment 52708
[details]
did not build on gtk: Build output:
http://webkit-commit-queue.appspot.com/results/1608276
Enrica Casucci
Comment 48
2010-04-07 06:55:11 PDT
Created
attachment 52734
[details]
Patch8 One more Gtk build error.
WebKit Review Bot
Comment 49
2010-04-07 06:56:35 PDT
Attachment 52734
[details]
did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.cpp WARNING: Skipping non-existent file: WebCore/platform/graphics/GlyphWidthMap.h WebCore/platform/graphics/FontFastPath.cpp:237: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 48 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 50
2010-04-07 07:28:27 PDT
Attachment 52734
[details]
did not build on chromium: Build output:
http://webkit-commit-queue.appspot.com/results/1605365
Dave Hyatt
Comment 51
2010-04-07 09:50:25 PDT
Comment on
attachment 52734
[details]
Patch8 r=me. I gave some comments/feedback on IRC.
mitz
Comment 52
2010-04-07 09:55:48 PDT
A couple of ideas about improving performance: 1) Populate an entire page for glyph metrics at a time instead of glyph-at-a-time 2) Instead of using CGRectApplyAffineTransform(), just multiply everything by pointSize, since we know that the transform is a scale.
mitz
Comment 53
2010-04-07 09:57:01 PDT
(In reply to
comment #52
)
> A couple of ideas about improving performance:
The idea is that by doing this (and other improvements) this could be enabled for the fast path without a significant performance hit.
Enrica Casucci
Comment 54
2010-04-07 10:07:19 PDT
Committed revision 57215. Added FIXME in canUseGlyphCache for characters with stacked diacritics. Leaving the bug open because it is not 100% fixed, but it is a big step forward in the right direction.
Eric Seidel (no email)
Comment 55
2010-04-18 16:29:22 PDT
Attachment 52734
[details]
was posted by a committer and has review+, assigning to Enrica Casucci for commit.
Ojan Vafai
Comment 56
2010-04-28 13:54:32 PDT
http://trac.webkit.org/changeset/58426
broke this again. It didn't fully rollback
http://trac.webkit.org/changeset/57215
though, just the part that caused a known perf regression. See
bug 37292
.
mitz
Comment 57
2010-04-28 22:40:42 PDT
Created
attachment 54676
[details]
Account for overflow of glyphs for U+1E00..U+2000 without sending them through the complex text code path
WebKit Review Bot
Comment 58
2010-04-28 22:44:55 PDT
Attachment 54676
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/graphics/WidthIterator.h:41: More than one command on the same line [whitespace/newline] [4] WebCore/platform/graphics/WidthIterator.h:42: More than one command on the same line [whitespace/newline] [4] WebCore/platform/graphics/WidthIterator.h:43: More than one command on the same line [whitespace/newline] [4] WebCore/platform/graphics/WidthIterator.h:44: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Adele Peterson
Comment 59
2010-04-28 22:53:48 PDT
Comment on
attachment 54676
[details]
Account for overflow of glyphs for U+1E00..U+2000 without sending them through the complex text code path Although I realize it just mirrors the existing CodePath enum, I wish the codePath function name included a more descriptive term. The rest looks good. r=me!
mitz
Comment 60
2010-04-29 23:27:35 PDT
Created
attachment 54784
[details]
Account for overflow of glyphs for U+1E00..U+2000 without sending them through the complex text code path Changed codePath() to allowedOptimizations() which returns a bitfield. Is this better?
WebKit Review Bot
Comment 61
2010-04-29 23:29:13 PDT
Attachment 54784
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/graphics/WidthIterator.h:41: More than one command on the same line [whitespace/newline] [4] WebCore/platform/graphics/WidthIterator.h:42: More than one command on the same line [whitespace/newline] [4] WebCore/platform/graphics/WidthIterator.h:43: More than one command on the same line [whitespace/newline] [4] WebCore/platform/graphics/WidthIterator.h:44: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
mitz
Comment 62
2010-04-30 10:51:25 PDT
Attachment 54676
[details]
committed as <
http://trac.webkit.org/projects/webkit/changeset/58585
>.
Ojan Vafai
Comment 63
2010-05-04 15:55:16 PDT
Just to close the loop, there is no perf regression with this fix.
mitz
Comment 64
2010-05-04 17:15:54 PDT
(In reply to
comment #63
)
> Just to close the loop, there is no perf regression with this fix.
That's great. Thanks for the update, Ojan!
Eric Seidel (no email)
Comment 65
2010-05-17 00:52:45 PDT
If I'm reading the comments correctly, looks like this should be closed. Please re-open if I'm wrong.
mitz
Comment 66
2010-05-17 07:51:37 PDT
Not fixed in general. The test case from this report is still failing.
Eric Seidel (no email)
Comment 67
2010-05-17 11:06:22 PDT
Comment on
attachment 52734
[details]
Patch8 Obsoleting since this was landed.
Eric Seidel (no email)
Comment 68
2010-05-17 11:07:31 PDT
Comment on
attachment 54676
[details]
Account for overflow of glyphs for U+1E00..U+2000 without sending them through the complex text code path Obsoleting since this was landed. Given the number of comments and attachments on this bug, I suspect it would be easier to deal with future fixes if they were attached to new bugs. :) But I leave that up to you. Cleared the r+ on the two attachments which were landed so that pending-commit stops listing this as needing landing.
mitz
Comment 69
2010-10-14 08:57:03 PDT
***
Bug 47657
has been marked as a duplicate of this bug. ***
mitz
Comment 70
2011-02-17 13:07:58 PST
***
Bug 54599
has been marked as a duplicate of this bug. ***
Tim Horton
Comment 71
2011-08-04 16:58:11 PDT
***
Bug 65732
has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 72
2012-02-25 21:43:14 PST
***
Bug 79582
has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 73
2012-02-25 21:44:12 PST
Bug 79582
has a test case where even optimizeLegibility helps only partially (tested with Safari 5.1.3 on OS X 10.7.3).
Alexey Proskuryakov
Comment 74
2013-06-06 15:34:52 PDT
***
Bug 117253
has been marked as a duplicate of this bug. ***
David Kilzer (:ddkilzer)
Comment 75
2014-09-03 11:22:14 PDT
***
Bug 17026
has been marked as a duplicate of this bug. ***
mitz
Comment 76
2015-12-16 15:51:16 PST
***
Bug 152292
has been marked as a duplicate of this bug. ***
Myles C. Maxfield
Comment 77
2016-01-25 16:26:50 PST
***
Bug 153393
has been marked as a duplicate of this bug. ***
Myles C. Maxfield
Comment 78
2016-05-12 12:17:30 PDT
***
Bug 108929
has been marked as a duplicate of this bug. ***
Tobi Reif
Comment 79
2016-05-13 02:10:04 PDT
When this issue here has been fixed, please also make sure that
https://tobireif.com/centerslate/
gets rendered as intended / as Chrome and Firefox do.
Jon Lee
Comment 80
2017-04-14 13:55:30 PDT
***
Bug 170801
has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 81
2019-03-03 18:26:37 PST
***
Bug 195044
has been marked as a duplicate of this bug. ***
Frank
Comment 82
2021-01-27 02:05:38 PST
***
Bug 220850
has been marked as a duplicate of this bug. ***
Myles C. Maxfield
Comment 83
2021-08-19 23:30:54 PDT
***
Bug 229179
has been marked as a duplicate of this bug. ***
Myles C. Maxfield
Comment 86
2021-09-14 01:34:29 PDT
***
Bug 225641
has been marked as a duplicate of this bug. ***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug