[Oilpan]: Moving the FontSelector/FontCacheClient, CSSSegmentedFontFace, and FontFaceCache to the o…

Source/core/css/FontFaceCache.cpp

Index: Source/core/css/FontFaceCache.cpp
diff --git a/Source/core/css/FontFaceCache.cpp b/Source/core/css/FontFaceCache.cpp
index a85e9fa38b717455679aedeec71e2b3a1d82f5d8..c4ab2a56dc3c635810b38bde73caa9212c3f8424 100644
--- a/Source/core/css/FontFaceCache.cpp
+++ b/Source/core/css/FontFaceCache.cpp
@@ -58,11 +58,11 @@ void FontFaceCache::addFontFace(CSSFontSelector* cssFontSelector, PassRefPtr<Fon
 {
     RefPtr<FontFace> fontFace = prpFontFace;
 
-    OwnPtr<TraitsMap>& familyFontFaces = m_fontFaces.add(fontFace->family(), nullptr).storedValue->value;
+    OwnPtrWillBeMember<TraitsMap>& familyFontFaces = m_fontFaces.add(fontFace->family(), nullptr).storedValue->value;
     if (!familyFontFaces)
-        familyFontFaces = adoptPtr(new TraitsMap);
+        familyFontFaces = adoptPtrWillBeNoop(new TraitsMap);

[haraken, 2014/04/07 14:10:12]

In our experimental branch, I think we were using a get() & set() pattern
instead of an add() & new() pattern.

  Item* item = map.get(name);
  if (!item)
    map.set(name, new Item);

In my understanding, previously we had to use the get() & set() pattern because
we didn't have conservative scanning. Now that we have the conservative
scanning, we can just use the add() & new() pattern. Am I understanding
correctly?

[wibling-chromium, 2014/04/08 09:35:59]

I am not sure we need conservative scanning to keep the object alive since it
would have been kept alive from the Member anyway if it was already part of the
map, but maybe I am missing a corner case.
Either way I decided to use the add/new pattern since it was the one already
being used in the code.

[haraken, 2014/04/08 11:14:40]

Sorry, this issue is not related to conservative scanning.

The reason why we replaced add/new with get/set in the experiment branch is this
one:
https://codereview.chromium.org/18375005/diff/8001/Source/core/css/CSSFontSel...
nullptr).iterator->value;
In my understanding, this issue still exists in the current oilpan, so we need
to use the get/set pattern instead of the add/new pattern.

Would you ask it to mads@ offline (and tell me the answer:-)?

[wibling-chromium, 2014/04/08 12:47:23]

Just talked to Mads about this:)
As he also mentioned in the review you referred above, the reason is that
anything could potentially happen when a GC is doing finalization since a
destructor could add/remove to the same set which could cause a rehashing or
even a move of the backing making the reference that I hold above pointing into
invalid memory.
If we can guarantee that no resizing of the collection happens from destructors
the add/new pattern is okay, but for now we might as well use the get/set
pattern since that is trivially safe.

[tkent, 2014/04/08 13:45:38]

Shouldn't we apply the add-new idiom to HeapHashMap with WeakMember in general?

 
-    RefPtr<CSSSegmentedFontFace>& segmentedFontFace = familyFontFaces->add(fontFace->traits().mask(), nullptr).storedValue->value;
+    RefPtrWillBeMember<CSSSegmentedFontFace>& segmentedFontFace = familyFontFaces->add(fontFace->traits().mask(), nullptr).storedValue->value;
     if (!segmentedFontFace)
         segmentedFontFace = CSSSegmentedFontFace::create(cssFontSelector, fontFace->traits());
 
@@ -92,7 +92,7 @@ void FontFaceCache::removeFontFace(FontFace* fontFace, bool cssConnected)
     TraitsMap::iterator familyFontFacesIter = familyFontFaces->find(fontFace->traits().mask());
     if (familyFontFacesIter == familyFontFaces->end())
         return;
-    RefPtr<CSSSegmentedFontFace> segmentedFontFace = familyFontFacesIter->value;
+    RefPtrWillBeRawPtr<CSSSegmentedFontFace> segmentedFontFace = familyFontFacesIter->value;
 
     segmentedFontFace->removeFontFace(fontFace);
     if (segmentedFontFace->isEmpty()) {
@@ -193,12 +193,12 @@ CSSSegmentedFontFace* FontFaceCache::get(const FontDescription& fontDescription,
     if (!familyFontFaces || familyFontFaces->isEmpty())
         return 0;
 
-    OwnPtr<TraitsMap>& segmentedFontFaceCache = m_fonts.add(family, nullptr).storedValue->value;
+    OwnPtrWillBeMember<TraitsMap>& segmentedFontFaceCache = m_fonts.add(family, nullptr).storedValue->value;
     if (!segmentedFontFaceCache)
-        segmentedFontFaceCache = adoptPtr(new TraitsMap);
+        segmentedFontFaceCache = adoptPtrWillBeNoop(new TraitsMap);
 
     FontTraits traits = fontDescription.traits();
-    RefPtr<CSSSegmentedFontFace>& face = segmentedFontFaceCache->add(traits.mask(), nullptr).storedValue->value;
+    RefPtrWillBeMember<CSSSegmentedFontFace>& face = segmentedFontFaceCache->add(traits.mask(), nullptr).storedValue->value;
     if (!face) {
         for (TraitsMap::const_iterator i = familyFontFaces->begin(); i != familyFontFaces->end(); ++i) {
             CSSSegmentedFontFace* candidate = i->value.get();
@@ -214,4 +214,10 @@ CSSSegmentedFontFace* FontFaceCache::get(const FontDescription& fontDescription,
     return face.get();
 }
 
+void FontFaceCache::trace(Visitor* visitor)
+{
+    visitor->trace(m_fontFaces);
+    visitor->trace(m_fonts);
+}
+
 }