Bug 147057 – [Freetype] Always allow font matching for strong aliases

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Michael Catanzaro]

Currently in FontCache::createFontPlatformData we allow font family substitution only during the configuration step but not during the matching step. This results in us missing obvious metric-compatible font matches, resulting in subpar font selection. E.g. headings on slashdot.com use font-family: Arial, serif, (an obvious bug), so we render them in serif, but we should use Liberation Sans instead because of this strong ("same") binding in 30-metric-aliases.conf:

	<!-- Microsoft -->
	<alias binding="same">
	  <family>Arial</family>
	  <accept>
	    <family>Arimo</family>
	    <family>Liberation Sans</family>
	    <family>Albany</family>
	    <family>Albany AMT</family>
	  </accept>
	</alias>

We should continue to disallow font substitution except for strong bindings.

[Michael Catanzaro]

Actually same (acceptable) and strong (preferred) are different; Chrome wants to match both, so we should do that too. We just don't want to match any others, since that could result in getting a crap font back. We can use the API that will be added to fontconfig in https://bugs.freedesktop.org/show_bug.cgi?id=19375 or we could copy Skia's code for this (see https://bugs.freedesktop.org/show_bug.cgi?id=90330)

[Martin Robinson]

Michael, are you planning on implementing the version of this for older versions of Fontconfig?

[Michael Catanzaro]

(In reply to comment #2)
> Michael, are you planning on implementing the version of this for older
> versions of Fontconfig?

I'm planning to try the same hack Skia has used, yes.

[Jeff Fortin]

Created attachment 257077 [details]
screenshot of ville.montreal.qc.ca

Just in case it might prove useful for testing, http://ville.montreal.qc.ca is another illustration of the need for this. It also uses "Arial,Helvetica,sans-serif" and, as you can see in the lower portion of this screenshot, when you're not using metrically equivalent font substitutions (ie when you get DejaVu instead of Liberation), that website's whole header menu blows up, making navigation impossible.

[Michael Catanzaro]

Created attachment 257326 [details]
Better much?

This was probably the hardest copying I've ever done; glad Skia is BSD licensed.

[Michael Catanzaro]

Created attachment 257327 [details]
Patch

[Michael Catanzaro]

Martin, this is ready for preliminary review. I did not mark it r? because the fontconfig configuration snippet I added to our layout test configuration causes the web process to crash, so all our layout tests fail. Obviously I need to figure out why modifying the configuration file breaks everything.

But it works outside of layout tests. :D

[Michael Catanzaro]

isFirstAliasWeak() should be named strengthOfFirstAlias()

[Michael Catanzaro]

Created attachment 257329 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 257329 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:35
> +// TODO(mcatanzaro): These should all be removed when no longer needed for the hacks below.

Use FIXME instead of TODO.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:44
> +template<> struct GPtrDeleter<FcFontSet> {
> +    void operator() (FcFontSet* ptr) const
> +    {
> +        if (ptr)
> +            FcFontSetDestroy(ptr);
> +    }
> +};

I think we could move all these to its own header with smart pointers for fontconfig. We used to have an OwnPtr in OwnPtrCairo.h for FcFontSet indeed.

This could be just 

WTF_DEFINE_GPTR_DELETER(FcFontSet, FcFontSetDestroy)

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:52
> +template<> struct GPtrDeleter<FcLangSet> {
> +    void operator() (FcLangSet* ptr) const
> +    {
> +        if (ptr)
> +            FcLangSetDestroy(ptr);
> +    }
> +};

Ditto.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:202
> +// TODO(mcatanzaro): This should be removed when no longer needed for the hacks below.

Use FIXME

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:211
> +// TODO(mcatanzaro): This is horrible. It should be deleted once Fontconfig can do this itself.

FIXME

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:270
> +static Vector<String> getStrongAliases(const String& family)

This should be strongAliases(). Or you could keep the name and make the Vector<String> an output parameter.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:296
> +        AliasStrength result = strengthOfFirstAlias(adoptRef(FcPatternDuplicate(minimal.get())));

This is a bit weird, I think it would be clearer if we pass the pointer and strengthOfFirstAlias does the duplication. I think we could even avoid the duplication in case there's no match.

[Carlos Garcia Campos]

Comment on attachment 257329 [details]
Patch

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

>> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:44
>> +};
> 
> I think we could move all these to its own header with smart pointers for fontconfig. We used to have an OwnPtr in OwnPtrCairo.h for FcFontSet indeed.
> 
> This could be just 
> 
> WTF_DEFINE_GPTR_DELETER(FcFontSet, FcFontSetDestroy)

Actually, we don't use glib in FreeType impl, so these shouldn't be GUniquePtr deleters, but std::unique_ptr directly.

[Michael Catanzaro]

(In reply to comment #10) 
> Use FIXME instead of TODO.

OK 

> I think we could move all these to its own header with smart pointers for
> fontconfig. We used to have an OwnPtr in OwnPtrCairo.h for FcFontSet indeed.
> 
> This could be just 
> 
> WTF_DEFINE_GPTR_DELETER(FcFontSet, FcFontSetDestroy)

OK, I will put the template for FcConfig with the other Cairo RefPtrs. The disadvantage of that is that when this code is removed, that template will be forgotten and remain unused, but that's hardly a big deal.

I will think about what to do for FcFontSet and FcObjectSet, since below you ask me to use std::unique_ptr instead.

> Ditto.

OK

> Use FIXME

OK

> FIXME

OK

> This should be strongAliases(). Or you could keep the name and make the
> Vector<String> an output parameter.

OK

> This is a bit weird, I think it would be clearer if we pass the pointer and
> strengthOfFirstAlias does the duplication. I think we could even avoid the
> duplication in case there's no match.

Agreed.

(In reply to comment #11)
> Actually, we don't use glib in FreeType impl, so these shouldn't be
> GUniquePtr deleters, but std::unique_ptr directly.

Good point.

[Michael Catanzaro]

This unclosed tag caused the web process to crash:

	    <!-- Test three different ways to replace one font with another. -->
	    <match target="pattern">
	        <test qual="all" name="family" compare="eq">
	            <string>FamilyReplacedWithFreeMono</string>
	        </test>
	        <edit name="family" mode="append_last">
	            <string>FreeMono/string>
	        </edit>
	    </match>

But in testing with Epiphany instead of WebKitTestRunner, Epiphany handled it robustly. It's hard to debug WebKitTestRunner so I don't know what's wrong.

I changed it to this:

    <match target="pattern">
        <test qual="all" name="family" compare="eq">
            <string>FamilyReplacedWithFreeMono</string>
        </test>
        <edit name="family" mode="assign" binding="same">
            <string>FreeMono</string>
        </edit>
    </match>

But that introduced roughly 200 layout test failures that I can't explain. I am just going to remove that configuration snippet and the associated test so as to unblock this; it was only ever a bonus test, the other two tests are sufficient to cover the new functionality, since the code I added does not touch the code that executes configuration substitutions. And none of our current tests would work if configuration substitutions were not respected, since we use tons of those in our fonts.conf.

Also note: Firefox seems to respect weak aliases as well. I am explicitly choosing to match Skia and not Firefox.

Lastly note: you must be mentally prepared to handle Liberation Sans on Bugzilla if you review this change!

[Michael Catanzaro]

Created attachment 257497 [details]
Patch

[WebKit Commit Bot]

Attachment 257497 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/freetype/UniquePtrFreeType.cpp:27:  default_delete::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/freetype/UniquePtrFreeType.cpp:32:  default_delete::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/freetype/UniquePtrFreeType.cpp:37:  default_delete::operator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

(In reply to comment #15) 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Filed by #147286.

[Carlos Garcia Campos]

Comment on attachment 257497 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/UniquePtrFreeType.cpp:27
> +namespace std {
> +
> +void default_delete<FcFontSet>::operator()(FcFontSet* ptr)

I'm not sure this is a good idea. According to 17.6.4.2.1 Namespace std: " program may add a template specialization for any standard library template to namespace std only if the declaration depends on a user-defined type and the specialization meets the standard library requirements for the original template and is not explicitly prohibited." What we have done in other cases is defining a deleter (in WebCore or WTF namespace) and use it directly as std::unique_ptr<Foo, FooDeleter>, or using template alias to give a name to the class-deleter pair making it more convenient to use.

> Source/WebCore/platform/graphics/freetype/UniquePtrFreeType.cpp:29
> +    FcFontSetDestroy(ptr);

Is FcFontSetDestroy null safe? I think you need a null check here.

> Source/WebCore/platform/graphics/freetype/UniquePtrFreeType.cpp:34
> +    FcLangSetDestroy(ptr);

Ditto.

> Source/WebCore/platform/graphics/freetype/UniquePtrFreeType.cpp:39
> +    FcObjectSetDestroy(ptr);

Ditto.

[Michael Catanzaro]

(In reply to comment #17)
> I'm not sure this is a good idea. According to 17.6.4.2.1 Namespace std: "
> program may add a template specialization for any standard library template
> to namespace std only if the declaration depends on a user-defined type and
> the specialization meets the standard library requirements for the original
> template and is not explicitly prohibited."

All of those conditions are satisfied in this case; std::unique_ptr wouldn't be much fun if you couldn't safely specialize std::default_deleter!

> What we have done in other cases
> is defining a deleter (in WebCore or WTF namespace) and use it directly as
> std::unique_ptr<Foo, FooDeleter>

My first attempt looked something like:

auto fcFontSetDeleter = [](FcFontSet* ptr) {
    FcFontSetDestroy(ptr);
};

But it was quite messy to use:

std::unique_ptr<FcFontSet, decltype(fcFontSetDeleter)> set(FcFontSetCreate(), fcFontSetDeleter);

Compared to:

std::unique_ptr<FcFontSet> set(FcFontSetCreate());

>  or using template alias to give a name to
> the class-deleter pair making it more convenient to use.

I took a look at XUniquePtr, since it was at the top of my grep for unique_ptr. What you did with XUniquePtr is much better than my first attempt with the lambda, and I'm fine with adding, say, FcUniquePtr to parallel XUniquePtr: that would work fine. But I still prefer simply specializing std::default_delete, that way we can use std::unique_ptr for everything, instead of creating a bunch of different pointer typedefs. (I would even suggest removing XUniquePtr.) Please let me know what you want me to do!

> > Source/WebCore/platform/graphics/freetype/UniquePtrFreeType.cpp:29
> > +    FcFontSetDestroy(ptr);
> 
> Is FcFontSetDestroy null safe? I think you need a null check here.

It doesn't need to be null safe (nor do the deleters in XUniquePtr). Looking in unique_ptr.h, the default specialization of std::default_delete is not null safe either:

      /// Calls @c delete[] @p __ptr
      void
      operator()(_Tp* __ptr) const
      {
	static_assert(!is_void<_Tp>::value,
		      "can't delete pointer to incomplete type");
	static_assert(sizeof(_Tp)>0,
		      "can't delete pointer to incomplete type");
	delete __ptr;
      }

It will only ever be called if ptr is nonnull:

      /// Destructor, invokes the deleter if the stored pointer is not null.
      ~unique_ptr() noexcept
      {
	auto& __ptr = std::get<0>(_M_t);
	if (__ptr != nullptr)
	  get_deleter()(__ptr);
	__ptr = pointer();
      }

I expect (but am not positive) that is required by the standard, since I can't find anything on the Internet to suggest the deleter should be null safe.

[Zan Dobersek]

Comment on attachment 257497 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:251
> +    int numIds;

I'd rather see this initialized to 0 before someone storms in and changes the proceeding loop to break without numIds assigned a proper value.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:253
> +        AliasStrength result = strengthOfFirstAlias(*minimal.get());

Just `*minimal` is enough.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:254
> +        if (AliasStrength::Done == result) {

The `variable == expectedValue` style is more common in WebKit code.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:258
> +        if (AliasStrength::Strong == result)

Same here.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:279
> +    char* patternChars(reinterpret_cast<char*>(FcPatternFormat(pattern.get(), reinterpret_cast<const FcChar8*>("%{family}"))));

Similarly, never seen a pointer initialized different than with the equals-sign notation.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:290
> +    for (String family : strongAliasesForFamily(familyA)) {

for (auto& family : ...) -- no need to copy each iterated element.

[Zan Dobersek]

About using std::default_delete<> instead of an aliased std::unique_ptr<> with a specified custom deleter -- didn't test this out, but when using std::unique_ptr<> with a custom deleter, you should get a link-time error if you use e.g. FcUniquePtr<> for a type that doesn't have the custom deleter appropriately specialized.

If you were using std::unique_ptr<> for a type that didn't have std::default_delete<> properly specialized, std::unique_ptr<> would fall back to the generic one, which would just `delete` the managed pointer.

Not a huge problem I guess, but it implies the human tax of keeping things under control.

Also, the patch doesn't build on EFL.

[Michael Catanzaro]

(In reply to comment #20)
> About using std::default_delete<> instead of an aliased std::unique_ptr<>
> with a specified custom deleter -- didn't test this out, but when using
> std::unique_ptr<> with a custom deleter, you should get a link-time error if
> you use e.g. FcUniquePtr<> for a type that doesn't have the custom deleter
> appropriately specialized.
> 
> If you were using std::unique_ptr<> for a type that didn't have
> std::default_delete<> properly specialized, std::unique_ptr<> would fall
> back to the generic one, which would just `delete` the managed pointer.
> 
> Not a huge problem I guess, but it implies the human tax of keeping things
> under control.

I realized this while washing dishes, but you beat me to it. I think that's actually a very strong point in favor of FcUniquePtr. I will do that instead.

> Also, the patch doesn't build on EFL.

It's broken without this patch.

(In reply to comment #19)
> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:251
> > +    int numIds;
> 
> I'd rather see this initialized to 0 before someone storms in and changes
> the proceeding loop to break without numIds assigned a proper value.

OK

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:253
> > +        AliasStrength result = strengthOfFirstAlias(*minimal.get());
> 
> Just `*minimal` is enough.

OK

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:254
> > +        if (AliasStrength::Done == result) {
> 
> The `variable == expectedValue` style is more common in WebKit code.

Accidental Yoda conditional, oops.

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:258
> > +        if (AliasStrength::Strong == result)
> 
> Same here.

OK.

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:279
> > +    char* patternChars(reinterpret_cast<char*>(FcPatternFormat(pattern.get(), reinterpret_cast<const FcChar8*>("%{family}"))));
> 
> Similarly, never seen a pointer initialized different than with the
> equals-sign notation.

OK. It's because I had originally used GUniquePtr there, and didn't clean up fully.

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:290
> > +    for (String family : strongAliasesForFamily(familyA)) {
> 
> for (auto& family : ...) -- no need to copy each iterated element.

Oops.

[Michael Catanzaro]

(In reply to comment #20)
> About using std::default_delete<> instead of an aliased std::unique_ptr<>
> with a specified custom deleter -- didn't test this out, but when using
> std::unique_ptr<> with a custom deleter, you should get a link-time error if
> you use e.g. FcUniquePtr<> for a type that doesn't have the custom deleter
> appropriately specialized.

Turns out you can make it a compile time error with a relatively comprehensible error message by deleting the non-specialized declaration:

template<typename T>
struct FcPtrDeleter {
    void operator()(T* ptr) const = delete;
};

[Michael Catanzaro]

Created attachment 257538 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #18)
> > > Source/WebCore/platform/graphics/freetype/UniquePtrFreeType.cpp:29
> > > +    FcFontSetDestroy(ptr);
> > 
> > Is FcFontSetDestroy null safe? I think you need a null check here.
> 
> It doesn't need to be null safe (nor do the deleters in XUniquePtr). Looking
> in unique_ptr.h, the default specialization of std::default_delete is not
> null safe either:
> 
>       /// Calls @c delete[] @p __ptr
>       void
>       operator()(_Tp* __ptr) const
>       {
> 	static_assert(!is_void<_Tp>::value,
> 		      "can't delete pointer to incomplete type");
> 	static_assert(sizeof(_Tp)>0,
> 		      "can't delete pointer to incomplete type");
> 	delete __ptr;
>       }
> 
> It will only ever be called if ptr is nonnull:
> 
>       /// Destructor, invokes the deleter if the stored pointer is not null.
>       ~unique_ptr() noexcept
>       {
> 	auto& __ptr = std::get<0>(_M_t);
> 	if (__ptr != nullptr)
> 	  get_deleter()(__ptr);
> 	__ptr = pointer();
>       }

That's because the default deleter falls back to "delete" which is null-safe. If your delete implementation is not null safe you will get a crash sooner or later.
 
> I expect (but am not positive) that is required by the standard, since I
> can't find anything on the Internet to suggest the deleter should be null
> safe.

I don't think that's the case.

[Michael Catanzaro]

(In reply to comment #24)
> > It will only ever be called if ptr is nonnull:
> > 
> >       /// Destructor, invokes the deleter if the stored pointer is not null.
> >       ~unique_ptr() noexcept
> >       {
> > 	auto& __ptr = std::get<0>(_M_t);
> > 	if (__ptr != nullptr)
> > 	  get_deleter()(__ptr);
> > 	__ptr = pointer();
> >       }
> 
> That's because the default deleter falls back to "delete" which is
> null-safe. If your delete implementation is not null safe you will get a
> crash sooner or later.

Somehow I didn't know that! But still, a crash would be impossible with libstdc++ at least: the deleter is clearly never called if the pointer is null.

> > I expect (but am not positive) that is required by the standard, since I
> > can't find anything on the Internet to suggest the deleter should be null
> > safe.
> 
> I don't think that's the case.

cppreference.com says "If get() == nullptr there are no effects. Otherwise, the owned object is destroyed via get_deleter()(get())." Less-trustworthy cplusplus.com says "If the object is an empty unique_ptr (i.e., get()==nullptr), the destructor has no effect." I think they're probably right!

[Myles C. Maxfield]

I don't think I can review this :/

[Martin Robinson]

Comment on attachment 257538 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/RefPtrCairo.cpp:120
> +    if (LIKELY(ptr != 0))

0 should be nullptr here.

> Source/WebCore/platform/graphics/cairo/RefPtrCairo.cpp:126
> +    if (LIKELY(ptr != 0))

Ditto.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:192
> +    FcResult result;
> +    FcValue value;
> +    result = FcPatternGet(&original, FC_FAMILY, 0, &value);
> +    if (result != FcResultMatch)
> +        return AliasStrength::Done;
> +

'result' should be defined when it is first used.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:204
> +    FcLangSetAdd(strongLangSet.get(), reinterpret_cast<const FcChar8*>("nomatchlang"));

I'm kind of surprised the reinterpret_cast is necessary here.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:300
> +    for (auto& family : strongAliasesForFamily(familyA)) {
> +        if (family == familyB)
> +            return true;
> +    }
> +    return false;

I wonder if it makes sense to cache aliases? Do you have a sense of the amount of time it takes to run strongAliasesForFamily?

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:359
> +        && !areStronglyAliased(familyNameAfterConfiguration, familyNameAfterMatching)
>          && !(equalIgnoringCase(familyNameString, "sans") || equalIgnoringCase(familyNameString, "sans-serif")
>            || equalIgnoringCase(familyNameString, "serif") || equalIgnoringCase(familyNameString, "monospace")
>            || equalIgnoringCase(familyNameString, "fantasy") || equalIgnoringCase(familyNameString, "cursive")))

Let's save the strongly aliased check for last since it's the most expensive and we want to avoid it with short-circuit logic if possible.

[Michael Catanzaro]

Note: the fontconfig maintainer wrote a patch: http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz19375

Might be better to wait for their new API if it lands soon, use it conditionally, and say that this is only supported with newer fontconfig; that way we can avoid this horrible mess. Or we could add this code just for compatibility with old fontconfig, but I don't think it's important to do so.

(In reply to comment #26)
> I don't think I can review this :/

We don't expect you to, unless you are secretly a fontconfig expert. I dunno why commit-queue added you!

(In reply to comment #27)
> > Source/WebCore/platform/graphics/cairo/RefPtrCairo.cpp:120
> > +    if (LIKELY(ptr != 0))
> 
> 0 should be nullptr here.

OK; I'll update the rest of this file in another patch.

> > Source/WebCore/platform/graphics/cairo/RefPtrCairo.cpp:126
> > +    if (LIKELY(ptr != 0))
> 
> Ditto.

Ditto.

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:192
> > +    FcResult result;
> > +    FcValue value;
> > +    result = FcPatternGet(&original, FC_FAMILY, 0, &value);
> > +    if (result != FcResultMatch)
> > +        return AliasStrength::Done;
> > +
> 
> 'result' should be defined when it is first used.

OK

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:204
> > +    FcLangSetAdd(strongLangSet.get(), reinterpret_cast<const FcChar8*>("nomatchlang"));
> 
> I'm kind of surprised the reinterpret_cast is necessary here.

I am surprised too, but:

../../Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:203:39: error: static_cast from 'const char *' to 'const FcChar8 *' (aka 'const unsigned char *') is not allowed
    FcLangSetAdd(strongLangSet.get(), static_cast<const FcChar8*>("nomatchlang"));
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Keeping in mind that char, unsigned char, and signed char are three different types (of which char will behave identically to either unsigned char or signed char), it is still a bit surprising that static_cast doesn't work.

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:300
> > +    for (auto& family : strongAliasesForFamily(familyA)) {
> > +        if (family == familyB)
> > +            return true;
> > +    }
> > +    return false;
> 
> I wonder if it makes sense to cache aliases? Do you have a sense of the
> amount of time it takes to run strongAliasesForFamily?

I guess it is the slowest portion of the code. A cache might speed it up, indeed.

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:359
> > +        && !areStronglyAliased(familyNameAfterConfiguration, familyNameAfterMatching)
> >          && !(equalIgnoringCase(familyNameString, "sans") || equalIgnoringCase(familyNameString, "sans-serif")
> >            || equalIgnoringCase(familyNameString, "serif") || equalIgnoringCase(familyNameString, "monospace")
> >            || equalIgnoringCase(familyNameString, "fantasy") || equalIgnoringCase(familyNameString, "cursive")))
> 
> Let's save the strongly aliased check for last since it's the most expensive
> and we want to avoid it with short-circuit logic if possible.

Agreed.

[Michael Catanzaro]

Created attachment 257667 [details]
Patch

[Martin Robinson]

Comment on attachment 257667 [details]
Patch

Great work. Thanks!

[WebKit Commit Bot]

Attachment 257667 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Committed r187527: <http://trac.webkit.org/changeset/187527>