Bug 42154 - Web Font is downloaded even when all the characters in the document are outside its unicode-range
: Web Font is downloaded even when all the characters in the document are outsi...
Status: NEW
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Myles C. Maxfield
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-13 03:15 PDT by Yuzo Fujishima
Modified: 2015-01-12 14:52 PST (History)
17 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

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@webkit.org 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 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 2011-05-23 11:11:06 PDT
Any thoughts Hyatt?  This seems like a relatively small patch.
Comment 29 mitz@webkit.org 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@webkit.org 2014-09-03 14:15:17 PDT
<rdar://problem/18030068>