Bug 42154 – Web Font is downloaded even when all the characters in the document are outside its unicode-range

• Component: CSS
• Status: RESOLVED FIXED

[Yuzo Fujishima]

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.

[Yuzo Fujishima]

Created attachment 64292 [details]
Patch

[WebKit Review Bot]

Attachment 64292 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3767106

[Yuzo Fujishima]

Created attachment 64301 [details]
Patch 2

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.

[Sam Weinig]

Comment on attachment 64301 [details]
Patch 2

r-ing based on mitz's comments.

[Yuzo Fujishima]

Created attachment 67022 [details]
Patch

[Yuzo Fujishima]

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.

[WebKit Review Bot]

Attachment 67022 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3968356

[Yuzo Fujishima]

Created attachment 67140 [details]
Fix Chromium and Win compilation

[WebKit Review Bot]

Attachment 67022 [details] did not build on win:
Build output: http://queues.webkit.org/results/3912397

[Yuzo Fujishima]

*** Bug 42571 has been marked as a duplicate of this bug. ***

[Yuzo Fujishima]

Ping?

[Nikolas Zimmermann]

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.

[Yuzo Fujishima]

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.

[Yuzo Fujishima]

Sorry for the nagging, but can anyone review this?

[Dimitri Glazkov (Google)]

(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!

[Eric Seidel (no email)]

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?

[Yuzo Fujishima]

Created attachment 75669 [details]
Use enum TriggerLoad

[Yuzo Fujishima]

Thank you for the review. Introduced enum TriggerLoad.

[WebKit Review Bot]

Attachment 75669 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6743065

[Build Bot]

Attachment 75669 [details] did not build on win:
Build output: http://queues.webkit.org/results/6758067

[Yuzo Fujishima]

Created attachment 75762 [details]
Fix Win/Linux Compilation

[Yuzo Fujishima]

Created attachment 78500 [details]
Pre Source/WebCore move

[Yuzo Fujishima]

Created attachment 78637 [details]
Post Source/WebCore move

[Dimitri Glazkov (Google)]

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?

[Yuzo Fujishima]

Created attachment 79258 [details]
Address comment #25

[Yuzo Fujishima]

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.

[Eric Seidel (no email)]

Any thoughts Hyatt?  This seems like a relatively small patch.

See also bug 61561.

[Dave Hyatt]

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.

[Yuzo Fujishima]

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?

[Yuzo Fujishima]

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.

[Yuzo Fujishima]

I release the ownership of this bug.

[Jake Archibald]

I'm pretty sure this used to do the right thing in stable about a year ago. Regression?

[Chris Adams]

*** Bug 135976 has been marked as a duplicate of this bug. ***

<rdar://problem/18030068>

[Chris Adams]

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/

[Jon Lee]

rdar://problem/17779042

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

*** Bug 122851 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Created attachment 278237 [details]
Needs tests

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

<rdar://problem/17779042>

[Myles C. Maxfield]

Created attachment 278297 [details]
WIP

[Myles C. Maxfield]

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!

[Myles C. Maxfield]

Created attachment 278333 [details]
WIP

[Myles C. Maxfield]

Created attachment 278341 [details]
Patch

[Myles C. Maxfield]

Created attachment 278342 [details]
Patch

[Myles C. Maxfield]

Created attachment 278343 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

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

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

Created attachment 278409 [details]
Patch for committing

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

Created attachment 278410 [details]
Patch for committing

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 278421 [details]
Patch for committing

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 278450 [details]
Patch for committing

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 278450 [details]
Patch for committing

Clearing flags on attachment: 278450

Committed r200601: <http://trac.webkit.org/changeset/200601>

[Simon Fraser (smfr)]

Your test is failing:
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r200610%20(5059)/results.html

[Myles C. Maxfield]

The test fails if run after fast/text/font-face-set-javascript.html

[Divya Manian]

<rdar://problem/26335045>

[Claudio Saavedra]

I see that this was already committed in https://trac.webkit.org/changeset/200601/webkit, not sure why this is still open?