Bug 28024 - Need AtomicStrings for the various font family names
Summary: Need AtomicStrings for the various font family names
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Andras Becsi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-05 12:18 PDT by Eric Seidel (no email)
Modified: 2013-09-13 13:10 PDT (History)
4 users (show)

See Also:


Attachments
proposed patch (30.81 KB, patch)
2011-06-14 23:33 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (removed debug variables from perl script) (30.73 KB, patch)
2011-06-15 01:07 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
proposed patch v2 (33.76 KB, patch)
2011-06-20 09:34 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (35.35 KB, patch)
2011-08-16 05:18 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-08-05 12:18:09 PDT
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().
Comment 1 Eric Seidel (no email) 2009-08-05 12:58:32 PDT
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.
Comment 2 Andras Becsi 2011-06-14 23:33:09 PDT
Created attachment 97237 [details]
proposed patch
Comment 3 Andras Becsi 2011-06-15 01:07:00 PDT
Created attachment 97249 [details]
updated patch (removed debug variables from perl script)
Comment 4 Eric Seidel (no email) 2011-06-15 08:45:03 PDT
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?
Comment 5 Andras Becsi 2011-06-20 09:34:36 PDT
Created attachment 97811 [details]
proposed patch v2
Comment 6 Andras Becsi 2011-06-20 09:44:45 PDT
(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?
Comment 7 Andras Becsi 2011-08-16 05:18:46 PDT
Created attachment 104026 [details]
updated patch
Comment 8 Csaba Osztrogonác 2011-08-16 06:46:45 PDT
Comment on attachment 104026 [details]
updated patch

r=me
Comment 9 Andras Becsi 2011-08-16 07:26:38 PDT
Comment on attachment 104026 [details]
updated patch

Clearing flags on attachment: 104026

Committed r93097: <http://trac.webkit.org/changeset/93097>
Comment 10 Andras Becsi 2011-08-16 07:26:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Joseph Pecoraro 2013-09-13 13:10:03 PDT
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?