[Webkit-unassigned] [Bug 81332] Cache support for OpenTypeVerticalData

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=81332


Koji Ishii <kojiishi at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #152501|review?                     |
               Flag|                            |




--- Comment #18 from Koji Ishii <kojiishi at gmail.com>  2012-07-20 02:29:21 PST ---
(From update of attachment 152501)
View in context: https://bugs.webkit.org/attachment.cgi?id=152501&action=review

>> Source/WebCore/platform/graphics/FontCache.cpp:40
>> +#endif
> 
> In WebKit, it's more common to put the #if ENABLE check in the .h file and unconditionally include the .h file everywhere.

Thanks, I'll change this.

>> Source/WebCore/platform/graphics/FontCache.cpp:234
>> +typedef HashMap<AtomicString, OpenTypeVerticalData*> FontVerticalDataCache;
> 
> Can this be HashMap<AtomicString, OwnPtr<OpenTypeVerticalData> > ?

Yes, I can. As Ryosuke pointed out above, I just chose consistency with other parts of this file, whichever works for me. It might related with another feedback below though.

>> Source/WebCore/platform/graphics/FontCache.cpp:446
>> +            delete fontVerticalDataCache.take(keysToRemove[i]);
> 
> I see. We remove entries from fontVerticalDataCache when they are removed from gFontDataCache.  This seems really inefficient. Can we mark the keys for removal or just delete them directly when we're removing entries from gFontDataCache?

gFontDataCache contains one for each font instance (one instance for a specific style/size/etc.) while FontVerticalDataCache contains instances  one for each font file, so many SimpleFontData share single instance of FontVerticalData.

I can ref count if you prefer. That makes marking unnecessary, but ref counting doesn't seem to work well with caching to me. I can keep one ref from the cache and sweep entries where refcount == 1. I can make the weak reference cache and cleanup entries where refcount == 0. Either way, the current ref counting doesn't expose refcount directly (if I'm not mistaken) so I need to add a public API to RefPtr class to do this.

I chose GC because:
1. The rest of font cache code uses GC rather than ref counting.
2. As above, the current ref counting and caching doesn't work well without adding at least one public API to ref counting class.
3. This portion of the code runs only if FontVerticalDataCache has at least one entry. It only occurs when vertical flow is used, and the font is East Asian or author uses text-orientation:upright, so it doesn't add cost to the normal code path.

I understand most of WebKit code prefers ref counting, if font cache using GC isn't intentional and should be changed in near future, I can try to rewrite using ref counting. But then this portion is inconsistent with other parts of font cache.

Can you tell me what you think?

>> Source/WebCore/platform/graphics/FontCache.h:54
>> +#endif
> 
> You don't need the #if here. It should be safe to forward declare something that is unused.

Ok, I'll remove it. Thanks for the advise.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.