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
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-07-13 03:15 PST by
Modified: 2012-12-21 09:30 PST (History)


Attachments
Patch (12.81 KB, patch)
2010-08-12 20:24 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Patch 2 (12.88 KB, patch)
2010-08-12 22:23 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.40 KB, patch)
2010-09-09 04:59 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Fix Chromium and Win compilation (28.32 KB, patch)
2010-09-09 19:24 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Use enum TriggerLoad (28.90 KB, patch)
2010-12-06 04:06 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Fix Win/Linux Compilation (28.92 KB, patch)
2010-12-06 17:35 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Pre Source/WebCore move (28.84 KB, patch)
2011-01-11 00:34 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Post Source/WebCore move (29.22 KB, patch)
2011-01-11 18:22 PST, Yuzo Fujishima
no flags Review Patch | Details | Formatted Diff | Diff
Address comment #25 (29.73 KB, patch)
2011-01-18 03:57 PST, Yuzo Fujishima
hyatt: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-13 03:15:56 PST
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 From 2010-08-12 20:24:48 PST -------
Created an attachment (id=64292) [details]
Patch
------- Comment #2 From 2010-08-12 22:12:44 PST -------
Attachment 64292 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3767106
------- Comment #3 From 2010-08-12 22:23:28 PST -------
Created an attachment (id=64301) [details]
Patch 2
------- Comment #4 From 2010-08-13 11:47:08 PST -------
(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 #5 From 2010-08-14 14:52:38 PST -------
(From update of attachment 64301 [details])
r-ing based on mitz's comments.
------- Comment #6 From 2010-09-09 04:59:46 PST -------
Created an attachment (id=67022) [details]
Patch
------- Comment #7 From 2010-09-09 05:01:22 PST -------
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] [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 From 2010-09-09 06:12:19 PST -------
Attachment 67022 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3968356
------- Comment #9 From 2010-09-09 19:24:17 PST -------
Created an attachment (id=67140) [details]
Fix Chromium and Win compilation
------- Comment #10 From 2010-09-09 20:17:28 PST -------
Attachment 67022 [details] did not build on win:
Build output: http://queues.webkit.org/results/3912397
------- Comment #11 From 2010-09-12 22:26:12 PST -------
*** Bug 42571 has been marked as a duplicate of this bug. ***
------- Comment #12 From 2010-10-29 00:16:02 PST -------
Ping?
------- Comment #13 From 2010-11-04 05:50:45 PST -------
(From update of attachment 67140 [details])
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 From 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 From 2010-11-30 19:37:01 PST -------
Sorry for the nagging, but can anyone review this?
------- Comment #16 From 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 From 2010-12-03 12:29:42 PST -------
(From update of attachment 67140 [details])
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 From 2010-12-06 04:06:51 PST -------
Created an attachment (id=75669) [details]
Use enum TriggerLoad
------- Comment #19 From 2010-12-06 04:07:25 PST -------
Thank you for the review. Introduced enum TriggerLoad.
------- Comment #20 From 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 From 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 From 2010-12-06 17:35:45 PST -------
Created an attachment (id=75762) [details]
Fix Win/Linux Compilation
------- Comment #23 From 2011-01-11 00:34:13 PST -------
Created an attachment (id=78500) [details]
Pre Source/WebCore move
------- Comment #24 From 2011-01-11 18:22:26 PST -------
Created an attachment (id=78637) [details]
Post Source/WebCore move
------- Comment #25 From 2011-01-12 21:36:40 PST -------
(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.

> 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 From 2011-01-18 03:57:04 PST -------
Created an attachment (id=79258) [details]
Address comment #25
------- Comment #27 From 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] [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 From 2011-05-23 11:11:06 PST -------
Any thoughts Hyatt?  This seems like a relatively small patch.
------- Comment #29 From 2011-06-07 16:47:31 PST -------
See also bug 61561.
------- Comment #30 From 2011-06-14 12:58:56 PST -------
(From update of attachment 79258 [details])
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 From 2011-06-17 01:15:25 PST -------
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 From 2011-06-21 06:25:29 PST -------
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 From 2011-07-03 22:01:56 PST -------
I release the ownership of this bug.
------- Comment #34 From 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?