Store a copy of generic font settings at CSSFontSelector.

Store a copy of generic font settings at CSSFontSelector.

A CSSFontSelector instance does not expect for generic font settings to
ever change within its lifetime. So, it makes sense to copy and stash
them locally and avoid holding/using a Document pointer for this
purpose.

Eventually, CSSFontSelector hopes to become a per-Document font context,
which knows nothing about the Document itself, but provides an API
that a Document could use.

This also fixes bug 236298, since determining generic font no longer relies on CSSFontSelector::m_document.

BUG=324991, 236298
R=eae,eseidel,ksakamoto@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163130

[dglazkov, 2013-12-03 22:03:44]

This also fixes bug 236298, since determining generic font no longer relies on
CSSFontSelector::m_document.

[eseidel, 2013-12-03 22:05:36]

lgtm

Kerpow!

[commit-bot: I haz the power, 2013-12-03 22:05:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dglazkov@chromium.org/93783005/1

[commit-bot: I haz the power, 2013-12-03 22:05:49]

Failed to apply patch for Source/core/css/CSSFontSelector.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/css/CSSFontSelector.cpp
  Hunk #2 FAILED at 163.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/css/CSSFontSelector.cpp.rej

Patch:       Source/core/css/CSSFontSelector.cpp
Index: Source/core/css/CSSFontSelector.cpp
diff --git a/Source/core/css/CSSFontSelector.cpp
b/Source/core/css/CSSFontSelector.cpp
index
7c8a46d1cfd5e45b711a9841da4eb2cd53593ca1..d44965d40bdd1a31ebbac5687fc1de577b6816a1
100644
--- a/Source/core/css/CSSFontSelector.cpp
+++ b/Source/core/css/CSSFontSelector.cpp
@@ -108,12 +108,14 @@ void FontLoader::clearResourceFetcher()
 CSSFontSelector::CSSFontSelector(Document* document)
     : m_document(document)
     , m_fontLoader(document->fetcher())
+    ,
m_genericFontFamilySettings(document->frame()->settings()->genericFontFamilySettings())
 {
     // FIXME: An old comment used to say there was no need to hold a reference
to m_document
     // because "we are guaranteed to be destroyed before the document". But
there does not
     // seem to be any such guarantee.
 
     ASSERT(m_document);
+    ASSERT(m_document->frame());
     fontCache()->addClient(this);
 }
 
@@ -161,47 +163,41 @@ void CSSFontSelector::removeFontFaceRule(const
StyleRuleFontFace* fontFaceRule)
     m_cssSegmentedFontFaceCache.removeFontFaceRule(fontFaceRule);
 }
 
-static AtomicString familyNameFromSettings(Settings* settings, const
FontDescription& fontDescription, const AtomicString& genericFamilyName)
+static AtomicString familyNameFromSettings(const GenericFontFamilySettings&
settings, const FontDescription& fontDescription, const AtomicString&
genericFamilyName)
 {
-    if (!settings)
-        return emptyAtom;
-
     UScriptCode script = fontDescription.script();
 
     if (fontDescription.genericFamily() == FontDescription::StandardFamily &&
!fontDescription.isSpecifiedFont())
-        return settings->genericFontFamilySettings().standard(script);
+        return settings.standard(script);
 
 #if OS(ANDROID)
     return FontCache::getGenericFamilyNameForScript(genericFamilyName, script);
 #else
     if (genericFamilyName == FontFamilyNames::webkit_serif)
-        return settings->genericFontFamilySettings().serif(script);
+        return settings.serif(script);
     if (genericFamilyName == FontFamilyNames::webkit_sans_serif)
-        return settings->genericFontFamilySettings().sansSerif(script);
+        return settings.sansSerif(script);
     if (genericFamilyName == FontFamilyNames::webkit_cursive)
-        return settings->genericFontFamilySettings().cursive(script);
+        return settings.cursive(script);
     if (genericFamilyName == FontFamilyNames::webkit_fantasy)
-        return settings->genericFontFamilySettings().fantasy(script);
+        return settings.fantasy(script);
     if (genericFamilyName == FontFamilyNames::webkit_monospace)
-        return settings->genericFontFamilySettings().fixed(script);
+        return settings.fixed(script);
     if (genericFamilyName == FontFamilyNames::webkit_pictograph)
-        return settings->genericFontFamilySettings().pictograph(script);
+        return settings.pictograph(script);
     if (genericFamilyName == FontFamilyNames::webkit_standard)
-        return settings->genericFontFamilySettings().standard(script);
+        return settings.standard(script);
 #endif
     return emptyAtom;
 }
 
 PassRefPtr<FontData> CSSFontSelector::getFontData(const FontDescription&
fontDescription, const AtomicString& familyName)
 {
-    if (!m_document || !m_document->frame())
-        return 0;
-
     if (CSSSegmentedFontFace* face =
m_cssSegmentedFontFaceCache.getFontFace(fontDescription, familyName))
         return face->getFontData(fontDescription);
 
     // Try to return the correct font based off our settings, in case we were
handed the generic font family name.
-    AtomicString settingsFamilyName =
familyNameFromSettings(m_document->frame()->settings(), fontDescription,
familyName);
+    AtomicString settingsFamilyName =
familyNameFromSettings(m_genericFontFamilySettings, fontDescription,
familyName);
     if (settingsFamilyName.isEmpty())
         return 0;

[dglazkov, 2013-12-03 22:07:13]

On 2013/12/03 22:05:49, I haz the power (commit-bot) wrote:
> Failed to apply patch for Source/core/css/CSSFontSelector.cpp:

Ah, needs https://codereview.chromium.org/100653002/ to land first.

[commit-bot: I haz the power, 2013-12-03 22:55:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dglazkov@chromium.org/93783005/1

[commit-bot: I haz the power, 2013-12-04 00:34:55]

Change committed as 163130

[Nico, 2013-12-04 00:46:12]

No test!!

[esprehn, 2013-12-04 00:57:22]

This object is pretty big, but I guess we only have one per page/tree scope? Is
there a reason not to store a reference or a pointer to object?

https://codereview.chromium.org/93783005/diff/1/Source/core/css/CSSFontSelect...
File Source/core/css/CSSFontSelector.h (right):

https://codereview.chromium.org/93783005/diff/1/Source/core/css/CSSFontSelect...
Source/core/css/CSSFontSelector.h:108: GenericFontFamilySettings
m_genericFontFamilySettings;
Why not store a reference to it?

[dglazkov, 2013-12-04 04:48:42]

On 2013/12/04 00:46:12, Nico wrote:
> No test!!

Sorry :-\ At this point, it's not testable in a meaningful way. I have some
ideas on how to fix this glaring problem, though.

[dglazkov, 2013-12-04 04:51:34]

On 2013/12/04 00:57:22, esprehn wrote:
> This object is pretty big, but I guess we only have one per page/tree scope?
Is
> there a reason not to store a reference or a pointer to object?
> 
>
https://codereview.chromium.org/93783005/diff/1/Source/core/css/CSSFontSelect...
> File Source/core/css/CSSFontSelector.h (right):
> 
>
https://codereview.chromium.org/93783005/diff/1/Source/core/css/CSSFontSelect...
> Source/core/css/CSSFontSelector.h:108: GenericFontFamilySettings
> m_genericFontFamilySettings;
> Why not store a reference to it?

I actually started writing it as a RefCounted class, but then the aesthetics got
the best of me: when sharing an object, there's a feature/bug where you can
tweak the object in one place and see changes in another place. That would be a
loaded foot gun with this particular class.