Bug 42154 - Web Font is downloaded even when all the characters in the document are outside its unicode-range
Summary: Web Font is downloaded even when all the characters in the document are outsi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 42571 122851 135976 (view as bug list)
Depends on: 157661
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-13 03:15 PDT by Yuzo Fujishima
Modified: 2019-02-28 19:27 PST (History)
24 users (show)

See Also:


Attachments
Patch (12.81 KB, patch)
2010-08-12 20:24 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Patch 2 (12.88 KB, patch)
2010-08-12 22:23 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Patch (26.40 KB, patch)
2010-09-09 04:59 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Fix Chromium and Win compilation (28.32 KB, patch)
2010-09-09 19:24 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Use enum TriggerLoad (28.90 KB, patch)
2010-12-06 04:06 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Fix Win/Linux Compilation (28.92 KB, patch)
2010-12-06 17:35 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Pre Source/WebCore move (28.84 KB, patch)
2011-01-11 00:34 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Post Source/WebCore move (29.22 KB, patch)
2011-01-11 18:22 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Address comment #25 (29.73 KB, patch)
2011-01-18 03:57 PST, Yuzo Fujishima
hyatt: review-
Details | Formatted Diff | Diff
Needs tests (12.08 KB, patch)
2016-05-05 23:40 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (15.38 KB, patch)
2016-05-06 17:36 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (16.03 KB, patch)
2016-05-07 12:11 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (144.40 KB, patch)
2016-05-07 14:51 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (144.24 KB, patch)
2016-05-07 14:58 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (144.42 KB, patch)
2016-05-07 15:14 PDT, Myles C. Maxfield
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (987.72 KB, application/zip)
2016-05-07 16:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.59 MB, application/zip)
2016-05-07 16:19 PDT, Build Bot
no flags Details
Patch for committing (143.85 KB, patch)
2016-05-09 10:41 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (143.86 KB, patch)
2016-05-09 10:57 PDT, Myles C. Maxfield
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (909.80 KB, application/zip)
2016-05-09 11:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (819.20 KB, application/zip)
2016-05-09 11:50 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (663.78 KB, application/zip)
2016-05-09 11:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.42 MB, application/zip)
2016-05-09 12:00 PDT, Build Bot
no flags Details
Patch for committing (143.78 KB, patch)
2016-05-09 12:13 PDT, Myles C. Maxfield
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (970.19 KB, application/zip)
2016-05-09 12:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (935.99 KB, application/zip)
2016-05-09 12:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (661.86 KB, application/zip)
2016-05-09 13:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.46 MB, application/zip)
2016-05-09 13:21 PDT, Build Bot
no flags Details
Patch for committing (143.63 KB, patch)
2016-05-09 15:48 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuzo Fujishima 2010-07-13 03:15:56 PDT
http://dev.w3.org/csswg/css3-fonts/#unicode-range-desc says:
"User agents that download fonts for characters outside the defined unicode-range are considered non-conformant."

WebKit r63184 always downloads Web Fonts, even when all the characters are outside the unicode-range.
For example, in opening the following, WebKit downloads myfont.ttf, which it must not.

<style>
@font-face {
  font-family:myfont;
  src: url(myfont.ttf);
  unicode-range: U+FFFF;
}
</style>
<span style="font-family:myfont">
No characters here are in the unicode-range.
</span>

The behavior can be confirmed by either
- Setting a breakpoint at WebCore/css/CSSFontFaceSource.cpp:181,
- Checking the server log, or
- Monitoring the network traffic.
Comment 1 Yuzo Fujishima 2010-08-12 20:24:48 PDT
Created attachment 64292 [details]
Patch
Comment 2 WebKit Review Bot 2010-08-12 22:12:44 PDT
Attachment 64292 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3767106
Comment 3 Yuzo Fujishima 2010-08-12 22:23:28 PDT
Created attachment 64301 [details]
Patch 2
Comment 4 mitz 2010-08-13 11:47:08 PDT
Comment on attachment 64301 [details]
Patch 2

I don’t think that the render tree is the right level to implement this. I also suspect that this approach is incomplete, because not all text that gets drawn comes from RenderText. I think the right approach would be to trigger the load from getGlyphDataForCharacter. Glyph pages could be populated with “faults”, and getGlyphDataForCharacter could “fire the faults”, which would kick off loading, and meanwhile return the temporary glyph data. SimpleFontData or a subclass thereof could hold the knowledge of how to fire the fault, probably just by holding a pointer to an instance of an abstract class (which CSSFontFaceSource would inherit from) that has a single method for doing this.
Comment 5 Sam Weinig 2010-08-14 14:52:38 PDT
Comment on attachment 64301 [details]
Patch 2

r-ing based on mitz's comments.
Comment 6 Yuzo Fujishima 2010-09-09 04:59:46 PDT
Created attachment 67022 [details]
Patch
Comment 7 Yuzo Fujishima 2010-09-09 05:01:22 PDT
Hi, I've created a new patch based on your comment.
Can you take a look?

(In reply to comment #4)
> (From update of attachment 64301 [details])
> I don’t think that the render tree is the right level to implement this. I also suspect that this approach is incomplete, because not all text that gets drawn comes from RenderText. I think the right approach would be to trigger the load from getGlyphDataForCharacter. Glyph pages could be populated with “faults”, and getGlyphDataForCharacter could “fire the faults”, which would kick off loading, and meanwhile return the temporary glyph data. SimpleFontData or a subclass thereof could hold the knowledge of how to fire the fault, probably just by holding a pointer to an instance of an abstract class (which CSSFontFaceSource would inherit from) that has a single method for doing this.
Comment 8 WebKit Review Bot 2010-09-09 06:12:19 PDT
Attachment 67022 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3968356
Comment 9 Yuzo Fujishima 2010-09-09 19:24:17 PDT
Created attachment 67140 [details]
Fix Chromium and Win compilation
Comment 10 WebKit Review Bot 2010-09-09 20:17:28 PDT
Attachment 67022 [details] did not build on win:
Build output: http://queues.webkit.org/results/3912397
Comment 11 Yuzo Fujishima 2010-09-12 22:26:12 PDT
*** Bug 42571 has been marked as a duplicate of this bug. ***
Comment 12 Yuzo Fujishima 2010-10-29 00:16:02 PDT
Ping?
Comment 13 Nikolas Zimmermann 2010-11-04 05:50:45 PDT
Comment on attachment 67140 [details]
Fix Chromium and Win compilation

View in context: https://bugs.webkit.org/attachment.cgi?id=67140&action=review

I'm not commenting on the patch as such, rather on the test technique.

> LayoutTests/fast/css/custom-font-xheight.html:61
> +function test() {
> +    // Wait until the font download completes.
> +    // If there is better alternative, we should not use timer here.
> +    window.setTimeout(testMain, 100);

This looks really suspicious. I wonder if we should delay firing the onload event, in case there's a web font used.
According to http://paulirish.com/2009/fighting-the-font-face-fout/, this is what IE and Firefox are doing.
Comment 14 Yuzo Fujishima 2010-11-21 22:26:28 PST
Ping, reviewers?

As for the onload event firing commented in #13,
I think it's too big to handle in this patch, if we need to.
Comment 15 Yuzo Fujishima 2010-11-30 19:37:01 PST
Sorry for the nagging, but can anyone review this?
Comment 16 Dimitri Glazkov (Google) 2010-11-30 20:06:55 PST
(In reply to comment #15)
> Sorry for the nagging, but can anyone review this?

This sounds like something Mitz would love to review. Mieeeeetz! Mieeeeetz!
Comment 17 Eric Seidel (no email) 2010-12-03 12:29:42 PST
Comment on attachment 67140 [details]
Fix Chromium and Win compilation

View in context: https://bugs.webkit.org/attachment.cgi?id=67140&action=review

> WebCore/platform/graphics/GlyphPageTreeNode.h:82
> +    GlyphData glyphDataForCharacter(UChar32 c, bool mayLoad) const;

An enum is always better than a Bool (since the callsites are much more readable).  Seems we might also want a default value of DoNotTriggerLoads or similar?
Comment 18 Yuzo Fujishima 2010-12-06 04:06:51 PST
Created attachment 75669 [details]
Use enum TriggerLoad
Comment 19 Yuzo Fujishima 2010-12-06 04:07:25 PST
Thank you for the review. Introduced enum TriggerLoad.
Comment 20 WebKit Review Bot 2010-12-06 04:19:06 PST
Attachment 75669 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6743065
Comment 21 Build Bot 2010-12-06 05:10:37 PST
Attachment 75669 [details] did not build on win:
Build output: http://queues.webkit.org/results/6758067
Comment 22 Yuzo Fujishima 2010-12-06 17:35:45 PST
Created attachment 75762 [details]
Fix Win/Linux Compilation
Comment 23 Yuzo Fujishima 2011-01-11 00:34:13 PST
Created attachment 78500 [details]
Pre Source/WebCore move
Comment 24 Yuzo Fujishima 2011-01-11 18:22:26 PST
Created attachment 78637 [details]
Post Source/WebCore move
Comment 25 Dimitri Glazkov (Google) 2011-01-12 21:36:40 PST
Comment on attachment 78637 [details]
Post Source/WebCore move

View in context: https://bugs.webkit.org/attachment.cgi?id=78637&action=review

I decided to get things moving myself, and I think I understand what's happening. Comments below:

> Source/WebCore/ChangeLog:53
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * css/CSSFontFaceSource.cpp:
> +        (WebCore::CSSFontFaceSource::getFontData):
> +        * platform/graphics/FontFallbackList.cpp:
> +        (WebCore::FontFallbackList::releaseFontData):
> +        (WebCore::FontFallbackList::loadingCustomFonts):
> +        (WebCore::FontFallbackList::beforeDeleteSegmentedFontData):
> +        (WebCore::FontFallbackList::fontDataAt):
> +        (WebCore::FontFallbackList::setPlatformFont):
> +        (WebCore::FontFallbackList::appendFontData):
> +        * platform/graphics/FontFallbackList.h:
> +        * platform/graphics/FontFastPath.cpp:
> +        (WebCore::Font::glyphDataForCharacter):
> +        * platform/graphics/GlyphPageTreeNode.cpp:
> +        (WebCore::GlyphPage::glyphDataForCharacter):
> +        * platform/graphics/GlyphPageTreeNode.h:
> +        * platform/graphics/SegmentedFontData.cpp:
> +        (WebCore::SegmentedFontData::~SegmentedFontData):
> +        (WebCore::SegmentedFontData::isLoading):
> +        (WebCore::SegmentedFontData::registerObserver):
> +        (WebCore::SegmentedFontData::unregisterObserver):
> +        * platform/graphics/SegmentedFontData.h:
> +        (WebCore::SegmentedFontDataObserver::~SegmentedFontDataObserver):
> +        * platform/graphics/SimpleFontData.cpp:
> +        (WebCore::SimpleFontData::SimpleFontData):
> +        (WebCore::SimpleFontData::initCharWidths):
> +        (WebCore::SimpleFontData::platformGlyphInit):
> +        (WebCore::SimpleFontData::beginLoadIfNeeded):
> +        * platform/graphics/SimpleFontData.h:
> +        (WebCore::SimpleFontData::isUsed):
> +        * platform/graphics/chromium/SimpleFontDataLinux.cpp:
> +        (WebCore::SimpleFontData::platformInit):
> +        * platform/graphics/mac/SimpleFontDataMac.mm:
> +        (WebCore::SimpleFontData::platformInit):
> +        * platform/graphics/win/SimpleFontDataCGWin.cpp:
> +        (WebCore::SimpleFontData::platformInit):

For a non-trivial patch like this, it would be good to explain changes for each file/method. See http://trac.webkit.org/changeset/75543 for an example.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:833
> -		371F4FFC0D25E7F300ECE0D5 /* SegmentedFontData.h in Headers */ = {isa = PBXBuildFile; fileRef = 371F4FFA0D25E7F300ECE0D5 /* SegmentedFontData.h */; };
> +		371F4FFC0D25E7F300ECE0D5 /* SegmentedFontData.h in Headers */ = {isa = PBXBuildFile; fileRef = 371F4FFA0D25E7F300ECE0D5 /* SegmentedFontData.h */; settings = {ATTRIBUTES = (Private, ); }; };

Why is this necessary?

> Source/WebCore/platform/graphics/FontFallbackList.cpp:176
> +        static_cast<const SegmentedFontData*>(fontData)->registerObserver(const_cast<SegmentedFontDataObserver*>(static_cast<const SegmentedFontDataObserver*>(this)));

This double-static_cast seems sad. Could this be made more elegant? And don't say const_cast :)

> Source/WebCore/platform/graphics/FontFallbackList.h:41
> +class FontFallbackList : public RefCounted<FontFallbackList>, public SegmentedFontDataObserver {

Sounds like FontFallbackList is the only implementation of SegmentedFontDataObserver? Are we expecting to have more? If not, maybe a whole different interface is not necessary? But maybe having a nice separation of concerns is a good thing.

> Source/WebCore/platform/graphics/GlyphPageTreeNode.h:63
> +enum TriggerLoad {

This could be more descriptive. LoadBehavior? I dunno.

> Source/WebCore/platform/graphics/SegmentedFontData.h:87
> +    virtual void beforeDeleteSegmentedFontData(const SegmentedFontData*) = 0;

In WebKit, it's typical to use "will" prefix, as in "willDeleteSegmentedFontData".

> Source/WebCore/platform/graphics/SimpleFontData.h:273
> +    CachedResourceLoader* m_cachedResourceLoader;

I am worried about the lifetime of CachedResourceLoader vs. SimpleFontData. Can you explain how it's safe to hold to a raw ptr here?
Comment 26 Yuzo Fujishima 2011-01-18 03:57:04 PST
Created attachment 79258 [details]
Address comment #25
Comment 27 Yuzo Fujishima 2011-01-18 04:06:04 PST
Thank you very much for the review.

(In reply to comment #25)
> (From update of attachment 78637 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78637&action=review
> 
> I decided to get things moving myself, and I think I understand what's happening. Comments below:
> 
> > Source/WebCore/ChangeLog:53
> > +        * WebCore.xcodeproj/project.pbxproj:
> > +        * css/CSSFontFaceSource.cpp:
> > +        (WebCore::CSSFontFaceSource::getFontData):
> > +        * platform/graphics/FontFallbackList.cpp:
> > +        (WebCore::FontFallbackList::releaseFontData):
> > +        (WebCore::FontFallbackList::loadingCustomFonts):
> > +        (WebCore::FontFallbackList::beforeDeleteSegmentedFontData):
> > +        (WebCore::FontFallbackList::fontDataAt):
> > +        (WebCore::FontFallbackList::setPlatformFont):
> > +        (WebCore::FontFallbackList::appendFontData):
> > +        * platform/graphics/FontFallbackList.h:
> > +        * platform/graphics/FontFastPath.cpp:
> > +        (WebCore::Font::glyphDataForCharacter):
> > +        * platform/graphics/GlyphPageTreeNode.cpp:
> > +        (WebCore::GlyphPage::glyphDataForCharacter):
> > +        * platform/graphics/GlyphPageTreeNode.h:
> > +        * platform/graphics/SegmentedFontData.cpp:
> > +        (WebCore::SegmentedFontData::~SegmentedFontData):
> > +        (WebCore::SegmentedFontData::isLoading):
> > +        (WebCore::SegmentedFontData::registerObserver):
> > +        (WebCore::SegmentedFontData::unregisterObserver):
> > +        * platform/graphics/SegmentedFontData.h:
> > +        (WebCore::SegmentedFontDataObserver::~SegmentedFontDataObserver):
> > +        * platform/graphics/SimpleFontData.cpp:
> > +        (WebCore::SimpleFontData::SimpleFontData):
> > +        (WebCore::SimpleFontData::initCharWidths):
> > +        (WebCore::SimpleFontData::platformGlyphInit):
> > +        (WebCore::SimpleFontData::beginLoadIfNeeded):
> > +        * platform/graphics/SimpleFontData.h:
> > +        (WebCore::SimpleFontData::isUsed):
> > +        * platform/graphics/chromium/SimpleFontDataLinux.cpp:
> > +        (WebCore::SimpleFontData::platformInit):
> > +        * platform/graphics/mac/SimpleFontDataMac.mm:
> > +        (WebCore::SimpleFontData::platformInit):
> > +        * platform/graphics/win/SimpleFontDataCGWin.cpp:
> > +        (WebCore::SimpleFontData::platformInit):
> 
> For a non-trivial patch like this, it would be good to explain changes for each file/method. See http://trac.webkit.org/changeset/75543 for an example.

Added comments.

> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:833
> > -		371F4FFC0D25E7F300ECE0D5 /* SegmentedFontData.h in Headers */ = {isa = PBXBuildFile; fileRef = 371F4FFA0D25E7F300ECE0D5 /* SegmentedFontData.h */; };
> > +		371F4FFC0D25E7F300ECE0D5 /* SegmentedFontData.h in Headers */ = {isa = PBXBuildFile; fileRef = 371F4FFA0D25E7F300ECE0D5 /* SegmentedFontData.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Why is this necessary?

SegmentedFontData.h is now included in FontFallbackList.h, which is copied to PrivateHeaders directory at build time.

If we don't copy SegmentedFontData.h also, compilation errors would result.

> 
> > Source/WebCore/platform/graphics/FontFallbackList.cpp:176
> > +        static_cast<const SegmentedFontData*>(fontData)->registerObserver(const_cast<SegmentedFontDataObserver*>(static_cast<const SegmentedFontDataObserver*>(this)));
> 
> This double-static_cast seems sad. Could this be made more elegant? And don't say const_cast :)

Removed casting by not using SegmentedFontDataObserver interface.

> 
> > Source/WebCore/platform/graphics/FontFallbackList.h:41
> > +class FontFallbackList : public RefCounted<FontFallbackList>, public SegmentedFontDataObserver {
> 
> Sounds like FontFallbackList is the only implementation of SegmentedFontDataObserver? Are we expecting to have more? If not, maybe a whole different interface is not necessary? But maybe having a nice separation of concerns is a good thing.

As mentioned above, stopped using the interface.

> 
> > Source/WebCore/platform/graphics/GlyphPageTreeNode.h:63
> > +enum TriggerLoad {
> 
> This could be more descriptive. LoadBehavior? I dunno.

Renamed to CharacterSource, which the caller should know.
(Caller may not have any idea if load will be triggered or not, on the other hand.)

> 
> > Source/WebCore/platform/graphics/SegmentedFontData.h:87
> > +    virtual void beforeDeleteSegmentedFontData(const SegmentedFontData*) = 0;
> 
> In WebKit, it's typical to use "will" prefix, as in "willDeleteSegmentedFontData".

Renamed as suggested.

> 
> > Source/WebCore/platform/graphics/SimpleFontData.h:273
> > +    CachedResourceLoader* m_cachedResourceLoader;
> 
> I am worried about the lifetime of CachedResourceLoader vs. SimpleFontData. Can you explain how it's safe to hold to a raw ptr here?

m_cachedResourceLoader has the same lifetime as the document,
and SimpleFontData created here never outlives the document.
(Some other SImpleFontData can be put into global cache and live longer than
each document, though.)

But I admit the assumption can be broken by future changes.
Comment 28 Eric Seidel (no email) 2011-05-23 11:11:06 PDT
Any thoughts Hyatt?  This seems like a relatively small patch.
Comment 29 mitz 2011-06-07 16:47:31 PDT
See also bug 61561.
Comment 30 Dave Hyatt 2011-06-14 12:58:56 PDT
Comment on attachment 79258 [details]
Address comment #25

View in context: https://bugs.webkit.org/attachment.cgi?id=79258&action=review

Looks decent. You can avoid having to change SimpleFontData (and all the platform-specific files) with my suggestions below and make the patch much simpler/smaller.

> Source/WebCore/css/CSSFontFaceSource.cpp:181
> +        // Store cached font and cached resource loader in SimpleFontData to be used for on-demand web font loading.
> +        fontData.set(new SimpleFontData(tempData->platformData(), true, true, m_font.get(), fontSelector->cachedResourceLoader()));

You don't need to cache the resource loader in the FontData.

> Source/WebCore/platform/graphics/FontFastPath.cpp:79
> +                GlyphData data = page->glyphDataForCharacter(c, CharacterFromDocument);

Make a static helper function in FontFastPath.cpp that takes the Font and the Page and the character and hands you back the GlyphData. The Font is able to get to the cached resource loader, since the Font can get to the FontSelector. This way you don't have to cache the resource loader ever, and you also don't need to patch glyphDataForCharacter.

> Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:57
> +GlyphData GlyphPage::glyphDataForCharacter(UChar32 c, CharacterSource characterSource) const
> +{
> +    unsigned index = indexForCharacter(c);
> +
> +    if (characterSource == CharacterFromDocument)
> +        if (const SimpleFontData* fontData = m_glyphFontData[index])
> +            fontData->beginLoadIfNeeded();
> +
> +    return GlyphData(m_glyphs[index], m_glyphFontData[index]);
> +}

This can just be a static function in FontFastPath.cpp, and you don't need this CharacterSource stuff.

> Source/WebCore/platform/graphics/SimpleFontData.cpp:63
> +    , m_isUsed(false)
> +    , m_font(font)
> +    , m_cachedResourceLoader(cachedResourceLoader)

You shouldn't need any of this.

> Source/WebCore/platform/graphics/SimpleFontData.cpp:219
> +void SimpleFontData::beginLoadIfNeeded() const
> +{
> +    if (!m_isUsed && isCustomFont() && isLoading() && m_font && m_cachedResourceLoader) {
> +        m_isUsed = true;
> +        m_font->beginLoadIfNeeded(m_cachedResourceLoader);
> +    }
> +}

Can just be in FontFastPath instead in the helper function.

> Source/WebCore/platform/graphics/SimpleFontData.h:75
> +    SimpleFontData(const FontPlatformData&, bool isCustomFont = false, bool isLoading = false, CachedFont* = 0, CachedResourceLoader* = 0);

See previous comments, but you shouldn't have to change SimpleFontData at all.
Comment 31 Yuzo Fujishima 2011-06-17 01:15:25 PDT
Thank you very much for the review.

(In reply to comment #30)

> > Source/WebCore/platform/graphics/FontFastPath.cpp:79
> > +                GlyphData data = page->glyphDataForCharacter(c, CharacterFromDocument);
> 
> Make a static helper function in FontFastPath.cpp that takes the Font and the Page and the character and hands you back the GlyphData. The Font is able to get to the cached resource loader, since the Font can get to the FontSelector. This way you don't have to cache the resource loader ever, and you also don't need to patch glyphDataForCharacter.

I think the following pieces of information would be needed in the helper function:
1. The URL of the font (or a CachedFont instance)
2. Whether the font is already scheduled for loading

Can you advise me where to put them (or how to retrieve them)?
Does it make sense to put them into SimpleFontData?
Comment 32 Yuzo Fujishima 2011-06-21 06:25:29 PDT
Another, probably more difficult, issue is how to know whether all the characters
in the document have been scanned to retrieve the GlyphData.

Without knowing that, it is hard to tell if a certain web font is not used in the document
or characters that use the font are yet to be scanned.  Put differently, it is hard to determine
the return value of SimpleFontData::isLoading().

Perhaps we can determine the font usage at the completion of document loading.
But I'm not sure if it is feasible. I'm also not sure if we can handle text manipulation by JavaScript.

I would appreciate experts' advice on this.
Comment 33 Yuzo Fujishima 2011-07-03 22:01:56 PDT
I release the ownership of this bug.
Comment 34 Jake Archibald 2012-12-21 09:30:05 PST
I'm pretty sure this used to do the right thing in stable about a year ago. Regression?
Comment 35 Chris Adams 2014-08-15 11:26:32 PDT
*** Bug 135976 has been marked as a duplicate of this bug. ***
Comment 36 mitz 2014-09-03 14:15:17 PDT
<rdar://problem/18030068>
Comment 37 Chris Adams 2015-12-02 15:15:59 PST
rdar://18030068 was closed as a duplicate of rdar://17779042. I've asked for a status update since Chrome shipped this over a year ago and Firefox is about to ship it in version 44.

I've updated my test page with the current support status:

http://chris.improbable.org/experiments/browser/webfonts/universal-noto-sans/
Comment 38 Jon Lee 2015-12-02 17:46:06 PST
rdar://problem/17779042
Comment 39 Myles C. Maxfield 2015-12-02 19:08:41 PST
(In reply to comment #37)
> rdar://18030068 was closed as a duplicate of rdar://17779042. I've asked for
> a status update since Chrome shipped this over a year ago and Firefox is
> about to ship it in version 44.
> 
> I've updated my test page with the current support status:
> 
> http://chris.improbable.org/experiments/browser/webfonts/universal-noto-sans/

Thanks for the test page! This will be super useful when implementing.
Comment 40 Myles C. Maxfield 2016-05-05 23:36:06 PDT
*** Bug 122851 has been marked as a duplicate of this bug. ***
Comment 41 Myles C. Maxfield 2016-05-05 23:40:44 PDT
Created attachment 278237 [details]
Needs tests
Comment 42 Myles C. Maxfield 2016-05-05 23:42:32 PDT
Comment on attachment 278237 [details]
Needs tests

View in context: https://bugs.webkit.org/attachment.cgi?id=278237&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=42154

Needs the radar number
Comment 43 Myles C. Maxfield 2016-05-06 13:25:37 PDT
Comment on attachment 278237 [details]
Needs tests

View in context: https://bugs.webkit.org/attachment.cgi?id=278237&action=review

> Source/WebCore/css/CSSSegmentedFontFace.cpp:106
> +        ranges.appendRange(FontRanges::Range(unicodeRanges[j].from(), unicodeRanges[j].to(), WTFMove(fontAccessor)));

This needs to copy the ref, not move it.
Comment 44 Myles C. Maxfield 2016-05-06 14:37:26 PDT
<rdar://problem/17779042>
Comment 45 Myles C. Maxfield 2016-05-06 17:36:16 PDT
Created attachment 278297 [details]
WIP
Comment 46 Myles C. Maxfield 2016-05-07 12:01:44 PDT
Comment on attachment 278297 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=278297&action=review

> Source/WebCore/css/CSSSegmentedFontFace.cpp:136
> +    return WTFMove(fontRanges);

This move also moves out of the cache, making the cache cache empty ranges!
Comment 47 Myles C. Maxfield 2016-05-07 12:11:00 PDT
Created attachment 278333 [details]
WIP
Comment 48 Myles C. Maxfield 2016-05-07 14:51:56 PDT
Created attachment 278341 [details]
Patch
Comment 49 Myles C. Maxfield 2016-05-07 14:58:32 PDT
Created attachment 278342 [details]
Patch
Comment 50 Myles C. Maxfield 2016-05-07 15:14:19 PDT
Created attachment 278343 [details]
Patch
Comment 51 Build Bot 2016-05-07 16:07:15 PDT
Comment on attachment 278343 [details]
Patch

Attachment 278343 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1284221

New failing tests:
fast/text/unicode-range-download.html
Comment 52 Build Bot 2016-05-07 16:07:24 PDT
Created attachment 278346 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 53 Build Bot 2016-05-07 16:19:41 PDT
Comment on attachment 278343 [details]
Patch

Attachment 278343 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1284234

New failing tests:
fast/text/unicode-range-download.html
Comment 54 Build Bot 2016-05-07 16:19:48 PDT
Created attachment 278347 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 55 Darin Adler 2016-05-07 21:50:15 PDT
Comment on attachment 278343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278343&action=review

Looks like some tests are failing. Maybe null dereferences?

> Source/WebCore/css/CSSSegmentedFontFace.cpp:62
> +class CSSFontAccessor : public FontAccessor {

final?

> Source/WebCore/css/CSSSegmentedFontFace.cpp:69
> +    const Font* access() const override

final instead of override?

> Source/WebCore/css/CSSSegmentedFontFace.cpp:85
> +    bool isLoading() const override

final instead of override?

> Source/WebCore/platform/graphics/FontRanges.cpp:45
> +class TrivialFontAccessor : public FontAccessor {

final?

> Source/WebCore/platform/graphics/FontRanges.cpp:47
> +    static Ref<TrivialFontAccessor> create(RefPtr<Font>&& font)

Does this need to be able to take a null? Maybe a Ref instead of a RefPfr for the argument?

> Source/WebCore/platform/graphics/FontRanges.cpp:58
> +    const Font* access() const override

final instead of override?

> Source/WebCore/platform/graphics/FontRanges.cpp:65
> +        return m_font->isLoading();

final instead of override?

> Source/WebCore/platform/graphics/FontRanges.cpp:85
> +            if (const auto* font = range.font()) {

I don’t think the const here has value and I would omit it.

> Source/WebCore/platform/graphics/FontRanges.cpp:102
> -    return m_ranges[0].font();
> +    const Font* font = m_ranges[0].fontAccessor().access();

Should just be:

    auto* m_ranges[0].font();

No reason to write out the direct use of fontAccessor, I don’t think.

> Source/WebCore/platform/graphics/FontRanges.h:49
> +            : m_from(range.from())
> +            , m_to(range.to())

It would be better style to get at the data members directly rather than using the accessor functions. Makes it more clear that the only reason this has to be defined is because of the explicit copyRef that is needed to copy a Ref.

> Source/WebCore/platform/graphics/FontRanges.h:54
> +        Range(Range&&) = default;

There is no good reason to include this. It will be automatically generated whether we mention it or not.

> Source/WebCore/platform/graphics/FontRanges.h:55
> +        Range& operator=(const Range&) = delete;

We don’t need to delete this. The Ref class already won’t support this so it can’t be called, even if we don’t explicitly delete it.

> Source/WebCore/platform/graphics/FontRanges.h:56
> +        Range& operator=(Range&&) = default;

There is no good reason to include this. It will be automatically generated whether we mention it or not.

> Source/WebCore/platform/graphics/FontRanges.h:74
> +    FontRanges(const FontRanges&) = default;
> +    FontRanges& operator=(FontRanges&&) = default;

Can we please omit these? It’s really peculiar to explicitly mention the *copy* constructor and *move* assignment operator. I believe that the move constructor and copy assignment operator will also work; the fact that those two are not mentioned has no effect and the fact that these other two two are mentioned also has no effect.

> Source/WebCore/platform/graphics/FontRanges.h:80
>      unsigned size() const { return m_ranges.size(); }
>      const Range& rangeAt(unsigned i) const { return m_ranges[i]; }

It’s a little peculiar that we use a private vector here. There’s very little encapsulation provided by having this be a class. It might be better to either use a Vector directly and have some helper functions, or have this be a struct with a public ranges data member. The size/rangeAt functions won’t work with a “for” statement for example, or all the other helpful functions that take a vector.

> Source/WebCore/platform/graphics/FontSelector.h:43
> +    virtual const Font* access() const = 0;

I’m not too comfortable with the name “access” here. Is there some other terminology we could use? I think just font() would probably be a better name.
Comment 56 Myles C. Maxfield 2016-05-08 00:12:38 PDT
Comment on attachment 278343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278343&action=review

>> Source/WebCore/platform/graphics/FontRanges.h:74
>> +    FontRanges& operator=(FontRanges&&) = default;
> 
> Can we please omit these? It’s really peculiar to explicitly mention the *copy* constructor and *move* assignment operator. I believe that the move constructor and copy assignment operator will also work; the fact that those two are not mentioned has no effect and the fact that these other two two are mentioned also has no effect.

The build fails if these are omitted.
Comment 57 Myles C. Maxfield 2016-05-08 00:15:45 PDT
(In reply to comment #55)
> Comment on attachment 278343 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278343&action=review
> 
> Looks like some tests are failing. Maybe null dereferences?

The failing test is the one that I added in this patch, which uses testRunner.dumpResourceLoadCallbacks() to provide a log of font downloads (so we can make sure that not all the fonts downloaded). Unfortunately, the test downloads two fonts, which causes a race (which is by design). This is all fine, except that the resulting output may be interleaved in an unexpected way (because of the race).

I will discuss how to solve this with Brady tomorrow. The patch is working correctly, I just need a way for a test to make sure "only n fonts were downloaded" without caring about the exact order of events.
Comment 58 Myles C. Maxfield 2016-05-09 10:39:36 PDT
Comment on attachment 278343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278343&action=review

>> Source/WebCore/platform/graphics/FontRanges.h:80
>>      const Range& rangeAt(unsigned i) const { return m_ranges[i]; }
> 
> It’s a little peculiar that we use a private vector here. There’s very little encapsulation provided by having this be a class. It might be better to either use a Vector directly and have some helper functions, or have this be a struct with a public ranges data member. The size/rangeAt functions won’t work with a “for” statement for example, or all the other helpful functions that take a vector.

This is a good idea, but I think it's out of scope for this patch. https://bugs.webkit.org/show_bug.cgi?id=157476
Comment 59 Myles C. Maxfield 2016-05-09 10:41:11 PDT
(In reply to comment #57)
> (In reply to comment #55)
> > Comment on attachment 278343 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=278343&action=review
> > 
> > Looks like some tests are failing. Maybe null dereferences?
> 
> The failing test is the one that I added in this patch, which uses
> testRunner.dumpResourceLoadCallbacks() to provide a log of font downloads
> (so we can make sure that not all the fonts downloaded). Unfortunately, the
> test downloads two fonts, which causes a race (which is by design). This is
> all fine, except that the resulting output may be interleaved in an
> unexpected way (because of the race).
> 
> I will discuss how to solve this with Brady tomorrow. The patch is working
> correctly, I just need a way for a test to make sure "only n fonts were
> downloaded" without caring about the exact order of events.

Brady says to fix the test so resource completion ordering is deterministic.
Comment 60 Myles C. Maxfield 2016-05-09 10:41:29 PDT
Created attachment 278409 [details]
Patch for committing
Comment 61 WebKit Commit Bot 2016-05-09 10:48:18 PDT
Attachment 278409 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:59:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:70:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:86:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 62 Myles C. Maxfield 2016-05-09 10:57:01 PDT
Created attachment 278410 [details]
Patch for committing
Comment 63 WebKit Commit Bot 2016-05-09 11:01:23 PDT
Attachment 278410 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:59:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:70:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:86:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 64 Build Bot 2016-05-09 11:40:10 PDT
Comment on attachment 278410 [details]
Patch for committing

Attachment 278410 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1294255

New failing tests:
fast/text/unicode-range-download.html
Comment 65 Build Bot 2016-05-09 11:40:15 PDT
Created attachment 278415 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 66 Build Bot 2016-05-09 11:50:07 PDT
Comment on attachment 278410 [details]
Patch for committing

Attachment 278410 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1294298

New failing tests:
fast/text/unicode-range-download.html
Comment 67 Build Bot 2016-05-09 11:50:12 PDT
Created attachment 278417 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 68 Build Bot 2016-05-09 11:57:30 PDT
Comment on attachment 278410 [details]
Patch for committing

Attachment 278410 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1294302

New failing tests:
fast/text/unicode-range-download.html
Comment 69 Build Bot 2016-05-09 11:57:36 PDT
Created attachment 278419 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 70 Build Bot 2016-05-09 12:00:36 PDT
Comment on attachment 278410 [details]
Patch for committing

Attachment 278410 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1294303

New failing tests:
fast/text/unicode-range-download.html
Comment 71 Build Bot 2016-05-09 12:00:44 PDT
Created attachment 278420 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 72 Myles C. Maxfield 2016-05-09 12:13:19 PDT
Created attachment 278421 [details]
Patch for committing
Comment 73 WebKit Commit Bot 2016-05-09 12:16:23 PDT
Attachment 278421 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:59:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:70:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:86:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 74 Build Bot 2016-05-09 12:49:35 PDT
Comment on attachment 278421 [details]
Patch for committing

Attachment 278421 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1294492

New failing tests:
fast/text/unicode-range-download.html
Comment 75 Build Bot 2016-05-09 12:49:41 PDT
Created attachment 278427 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 76 Build Bot 2016-05-09 12:53:15 PDT
Comment on attachment 278421 [details]
Patch for committing

Attachment 278421 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1294502

New failing tests:
fast/text/unicode-range-download.html
Comment 77 Build Bot 2016-05-09 12:53:21 PDT
Created attachment 278429 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 78 Build Bot 2016-05-09 13:14:15 PDT
Comment on attachment 278421 [details]
Patch for committing

Attachment 278421 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1294560

New failing tests:
fast/text/unicode-range-download.html
Comment 79 Build Bot 2016-05-09 13:14:22 PDT
Created attachment 278432 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 80 Build Bot 2016-05-09 13:21:34 PDT
Comment on attachment 278421 [details]
Patch for committing

Attachment 278421 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1294572

New failing tests:
fast/text/unicode-range-download.html
Comment 81 Build Bot 2016-05-09 13:21:41 PDT
Created attachment 278434 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 82 Myles C. Maxfield 2016-05-09 15:48:21 PDT
Created attachment 278450 [details]
Patch for committing
Comment 83 WebKit Commit Bot 2016-05-09 15:50:02 PDT
Attachment 278450 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:59:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/FontRanges.cpp:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:70:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/css/CSSSegmentedFontFace.cpp:86:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 84 WebKit Commit Bot 2016-05-09 17:12:14 PDT
Comment on attachment 278450 [details]
Patch for committing

Clearing flags on attachment: 278450

Committed r200601: <http://trac.webkit.org/changeset/200601>
Comment 86 Myles C. Maxfield 2016-05-10 15:10:09 PDT
The test fails if run after fast/text/font-face-set-javascript.html
Comment 87 Divya Manian 2017-06-21 13:18:09 PDT
<rdar://problem/26335045>
Comment 88 Claudio Saavedra 2019-02-27 06:27:14 PST
I see that this was already committed in https://trac.webkit.org/changeset/200601/webkit, not sure why this is still open?