Bug 28024 – Need AtomicStrings for the various font family names

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Need AtomicStrings for the various font family names

In:
static int identifierForFamily(const AtomicString& family)
{
    DEFINE_STATIC_LOCAL(AtomicString, cursiveFamily, ("-webkit-cursive")); 
    DEFINE_STATIC_LOCAL(AtomicString, fantasyFamily, ("-webkit-fantasy")); 
    DEFINE_STATIC_LOCAL(AtomicString, monospaceFamily, ("-webkit-monospace")); 
    DEFINE_STATIC_LOCAL(AtomicString, sansSerifFamily, ("-webkit-sans-serif")); 
    DEFINE_STATIC_LOCAL(AtomicString, serifFamily, ("-webkit-serif")); 

we use static locals.

A bunch of other places we just use c-string constants:

void CSSFontSelector::addFontFaceRule(const CSSFontFaceRule* fontFaceRule)
static FontData* fontDataForGenericFamily(Document* document, const FontDescription& fontDescription, const AtomicString& familyName)
void CSSStyleSelector::applyProperty(int id, CSSValue *value)

This is error-prone and slower than necessary.

Should be easy to fix with a little .in file wired up through make_names.pl in DerivedSources.make.  Also need to make sure that Page() calls FontFamilyNames::init().

[Eric Seidel (no email)]

Turns out that the various Names::init() calls are made from Frame(...), I'm surprised by this. It's probably "wrong" in that we could have a Page which wanted to use the various Names* calls before the first Frame was created... but oh well.

[Andras Becsi]

Created attachment 97237 [details]
proposed patch

[Andras Becsi]

Created attachment 97249 [details]
updated patch (removed debug variables from perl script)

[Eric Seidel (no email)]

Comment on attachment 97249 [details]
updated patch (removed debug variables from perl script)

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

> Source/WebCore/dom/make_names.pl:90
> +if (length($fontNamesIn)) {
> +    my $names = new IO::File;
> +    my $familyNamesFileBase = "WebKitFontFamilyNames";

Why are we forking all this perl code here?  Can't we share with the other names generation?

[Andras Becsi]

Created attachment 97811 [details]
proposed patch v2

[Andras Becsi]

(In reply to comment #4)
> (From update of attachment 97249 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97249&action=review
> 
> > Source/WebCore/dom/make_names.pl:90
> > +if (length($fontNamesIn)) {
> > +    my $names = new IO::File;
> > +    my $familyNamesFileBase = "WebKitFontFamilyNames";
> 
> Why are we forking all this perl code here?  Can't we share with the other names generation?

The common parts appeared to be not that many to be worth obfuscating the understandability of the patch, but I tried now to restructure the common parts of the generation routines into separate functions.

Eric, could you take another look?

[Andras Becsi]

Created attachment 104026 [details]
updated patch

[Csaba Osztrogonác]

Comment on attachment 104026 [details]
updated patch

r=me

[Andras Becsi]

Comment on attachment 104026 [details]
updated patch

Clearing flags on attachment: 104026

Committed r93097: <http://trac.webkit.org/changeset/93097>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 104026 [details]
updated patch

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

> Source/WebCore/CodeGenerators.pri:772
> +entities.depends = $$PWD/dom/make_names.pl $$FONT_NAMES

This has entities.depends, not fontnames.depends. Is that intended?