New issue
Advanced search Search tips

Issue 795225 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Mac
Pri: 2
Type: Bug

Blocking:
issue 794896



Sign in to add a comment

Font code should not perform fallback character lookup for personal use area codepoints

Project Member Reported by drott@chromium.org, Dec 15 2017

Issue description

While investigating  issue 794896  we find that FontCacheLinux performs per-character system fallback for the PUA characters encountered on the browse_news_cnn, browse_news_hackernews, and browse_social_twitter sites. 

The aggregated list of the PUA characters encountered on those sites is:
data:text/html;charset=utf-8,%26%23xf107%3B%26%23xf10d%3B%26%23xf133%3B%26%23xf13d%3B%26%23xf147%3B%26%23xf160%3B%26%23xf175%3B%26%23xf177%3B%26%23xf179%3B%26%23xf196%3B

Or, just spelled out here:
         

On my system, and on Ubuntu based Linux desktops having installed the Chandas font from the ttf-devanagari-fonts package, this leads to using Chandas for the characters from the PUA area.

This is not what is intended here.

Testing on FF Linux, this test string is displayed as Last Resort boxes with the hex code point printed in them.

Edge on Win, Safari on Mac also display boxes for this test string.

 

Comment 1 by kojii@chromium.org, Dec 15 2017

Looks like a UTF-8 sequence of:
EF 84 87 EF 84 8
? Ah...can't read %-escaped sequence very well.

"EF 84 87" is U+F107. Maybe Font Awesome fa-angle-down[1] but the font isn't loaded yet?

[1] http://fontawesome.io/icon/angle-down/

Comment 2 by drott@chromium.org, Dec 15 2017

Yes, I agree, it's most likely in the page because of symbol font usage and looked up during load. But I don't think we should try to find a system font for PUA, as that is slow and the result is undefined.

Comment 4 by drott@chromium.org, Dec 19 2017

Blocking: 794896

Comment 5 by drott@chromium.org, Dec 20 2017

Labels: Merge-Request-64
Project Member

Comment 6 by sheriffbot@chromium.org, Dec 20 2017

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: M64 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by drott@chromium.org, Dec 20 2017

I would like to merge this instead of https://bugs.chromium.org/p/chromium/issues/detail?id=782220#c30 for the same reason: Fixing a performance regression we found when rolling HarfBuzz. Thanks for your consideration.

Comment 8 by drott@chromium.org, Dec 20 2017

Labels: -Hotlist-Merge-Review -Merge-Review-64
Unfortunately, the CL has not landed yet - I'll ping and tag this issue again once it was in Canary. 
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/725d3699e90b2ad35a7b9d139b890a55457d9a80

commit 725d3699e90b2ad35a7b9d139b890a55457d9a80
Author: Dominik Röttsches <drott@chromium.org>
Date: Thu Dec 21 12:07:14 2017

Do not perform per-character font fallback for PUA codepoints

The analysis of a recent HarfBuzz performance regression showed that on
Linux fontconfig may return obscure system fonts that have coverage in
the Unicode private use area. Returning these has undefined semantics
and is not what we want. This lookup may occur when icon fonts with PUA
coverage are used as web fonts and have not been loaded yet. If we do
not perform character fallback in these cases, the last resort font will
be used instead.

This speeds up timeToFirstPaint on pages which use PUA area icon fonts
and where system fonts are installed which have coverage in this area -
for example, it affects our browse:news:cnn, browse:news:hackernews and
browse:social:twitter benchmark stories.

Bug:  795225 
Change-Id: Id5f87157aa7077cca8e1c0dc894bb76d2dd132c3
Reviewed-on: https://chromium-review.googlesource.com/829378
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525673}
[add] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/fallback-pua-last-resort-expected.txt
[add] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/fallback-pua-last-resort.js
[add] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/LayoutTests/platform/mac/inspector-protocol/layout-fonts/fallback-pua-last-resort-expected.txt
[add] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/LayoutTests/platform/win7/inspector-protocol/layout-fonts/fallback-pua-last-resort-expected.txt
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/fonts/FontCache.cpp
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/fonts/FontCache.h
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/fonts/FontCacheTest.cpp
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/fonts/android/FontCacheAndroid.cpp
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/fonts/fuchsia/FontCacheFuchsia.cpp
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/text/Character.cpp
[modify] https://crrev.com/725d3699e90b2ad35a7b9d139b890a55457d9a80/third_party/WebKit/Source/platform/text/Character.h

Comment 10 by drott@chromium.org, Dec 28 2017

Status: Fixed (was: Assigned)
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 9 2018

Labels: merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6

commit 3ac50fb73d2d97b9a0870114a5d93e007ffa05b6
Author: Dominik Röttsches <drott@chromium.org>
Date: Tue Jan 09 04:51:33 2018

[Merge] Do not perform per-character font fallback for PUA codepoints

The analysis of a recent HarfBuzz performance regression showed that on
Linux fontconfig may return obscure system fonts that have coverage in
the Unicode private use area. Returning these has undefined semantics
and is not what we want. This lookup may occur when icon fonts with PUA
coverage are used as web fonts and have not been loaded yet. If we do
not perform character fallback in these cases, the last resort font will
be used instead.

This speeds up timeToFirstPaint on pages which use PUA area icon fonts
and where system fonts are installed which have coverage in this area -
for example, it affects our browse:news:cnn, browse:news:hackernews and
browse:social:twitter benchmark stories.

TBR=drott@chromium.org

(cherry picked from commit 725d3699e90b2ad35a7b9d139b890a55457d9a80)

Bug:  795225 ,  782220 
Change-Id: Id5f87157aa7077cca8e1c0dc894bb76d2dd132c3
Reviewed-on: https://chromium-review.googlesource.com/829378
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/855996
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#454}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[add] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/fallback-pua-last-resort-expected.txt
[add] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/fallback-pua-last-resort.js
[add] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/LayoutTests/platform/mac/inspector-protocol/layout-fonts/fallback-pua-last-resort-expected.txt
[add] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/LayoutTests/platform/win7/inspector-protocol/layout-fonts/fallback-pua-last-resort-expected.txt
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/FontCache.cpp
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/FontCache.h
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/FontCacheTest.cpp
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/android/FontCacheAndroid.cpp
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/fuchsia/FontCacheFuchsia.cpp
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/text/Character.cpp
[modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/text/Character.h

Cc: ksakamoto@chromium.org
 Issue 803305  has been merged into this issue.

Sign in to add a comment