Summary: | text repainting does not account for glyphs which draw outside the typographic bounds of the font | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||||||||||||||||||||||||||||||
Component: | Text | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||||||||||||||||||||||
Severity: | Minor | CC: | adele, a.mueller, bertheau, bfulgham, bugzillawatcher, daniele.metilli, ddkilzer, dglazkov, enrica, eric, floam, gustavo, Hironori.Fujii, hrhlzhlqermt, ian, ian, ifiz.dev, james, joepeck, kiel.oleson, marshfamily407, mitz, mmaxfield, patorjk, pdr, pogon, simo2409, steve, webkit-bug-importer, webkit.review.bot, xan.lopez, xidorn-webkit, yuzo | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||
Version: | 420+ | ||||||||||||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||||||||||||||||||||||
URL: | http://www.hixie.ch/tests/adhoc/dom/web-apps/XMLHttpRequest/009.html | ||||||||||||||||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=234816 https://bugs.webkit.org/show_bug.cgi?id=235106 |
||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2005-12-28 13:36:03 PST
This is font-dependent. I was able to reproduce with Lucida Grande 16 selected as standard font. Created attachment 5357 [details]
screenshot
...and my anti-alias setting is Standard (best for CRT)
Created attachment 6563 [details]
Test case
With Zapfino the effect is even more dramatic.
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. *** Bug 14174 has been marked as a duplicate of this bug. *** *** Bug 20060 has been marked as a duplicate of this bug. *** *** Bug 20420 has been marked as a duplicate of this bug. *** Changing the title to reflect more accurately what is going on here (at least according to mitz). *** Bug 22175 has been marked as a duplicate of this bug. *** Created attachment 38749 [details]
First cut
Mac-only and “fast path”-only, not performance-tested, leaves some room for optimization and clean-up.
Created attachment 38766 [details]
Slightly better
Works with custom fonts, slightly cleaned up, fixes a bug in WidthIterator::advance().
Created attachment 38776 [details]
WIP
Works on Windows, fixes a couple of bugs and tries to be faster.
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. Created attachment 38779 [details]
WIP
Added overflow calculation in the Core Text code path.
Created attachment 38783 [details]
WIP
Added overflow calculation in the Uniscribe code path.
*** Bug 32506 has been marked as a duplicate of this bug. *** 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.
Comment on attachment 52574 [details]
patch
forgot to mark it as patch
Created attachment 52577 [details]
Patch2
Added layout test.
Attachment 52574 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1688007 Attachment 52577 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1687016 Attachment 52577 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1685035 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. (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. Created attachment 52670 [details]
Patch3
Missing windows layout tests results.
Creating now.
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.
Attachment 52670 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/1553318 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. Attachment 52670 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1680092 Attachment 52670 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1596273 (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. Created attachment 52676 [details]
Patch4
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.
Created attachment 52682 [details]
Patch5
Hopefully the laste one :-)
Attachment 52676 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1671143 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.
Attachment 52676 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1651219 Created attachment 52684 [details]
Patch6
Fixed for chromium.
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.
Attachment 52684 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1650217 Attachment 52684 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1570295 Created attachment 52708 [details]
Patch7
Fixed build errors for other platforms.
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.
Attachment 52708 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1568298 Attachment 52708 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/1608276 Created attachment 52734 [details]
Patch8
One more Gtk build error.
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.
Attachment 52734 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1605365 Comment on attachment 52734 [details]
Patch8
r=me. I gave some comments/feedback on IRC.
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. (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. 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. Attachment 52734 [details] was posted by a committer and has review+, assigning to Enrica Casucci for commit.
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. Created attachment 54676 [details]
Account for overflow of glyphs for U+1E00..U+2000 without sending them through the complex text code path
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.
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!
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?
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.
Just to close the loop, there is no perf regression with this fix. (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! If I'm reading the comments correctly, looks like this should be closed. Please re-open if I'm wrong. Not fixed in general. The test case from this report is still failing. Comment on attachment 52734 [details]
Patch8
Obsoleting since this was landed.
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.
*** Bug 47657 has been marked as a duplicate of this bug. *** *** Bug 54599 has been marked as a duplicate of this bug. *** *** Bug 65732 has been marked as a duplicate of this bug. *** *** Bug 79582 has been marked as a duplicate of this bug. *** Bug 79582 has a test case where even optimizeLegibility helps only partially (tested with Safari 5.1.3 on OS X 10.7.3). *** Bug 117253 has been marked as a duplicate of this bug. *** *** Bug 17026 has been marked as a duplicate of this bug. *** *** Bug 152292 has been marked as a duplicate of this bug. *** *** Bug 153393 has been marked as a duplicate of this bug. *** *** Bug 108929 has been marked as a duplicate of this bug. *** 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. *** Bug 170801 has been marked as a duplicate of this bug. *** *** Bug 195044 has been marked as a duplicate of this bug. *** *** Bug 220850 has been marked as a duplicate of this bug. *** *** Bug 229179 has been marked as a duplicate of this bug. *** *** Bug 225641 has been marked as a duplicate of this bug. *** |