[webkit-reviews] review canceled: [Bug 81332] Cache support for OpenTypeVerticalData : [Attachment 152501] Reflected changes from rniwa's review

[bugzilla-daemon at webkit.org]

Koji Ishii <kojiishi at gmail.com> has canceled Koji Ishii <kojiishi at gmail.com>'s
request for review:
Bug 81332: Cache support for OpenTypeVerticalData
https://bugs.webkit.org/show_bug.cgi?id=81332

Attachment 152501: Reflected changes from rniwa's review
https://bugs.webkit.org/attachment.cgi?id=152501&action=review

------- Additional Comments from Koji Ishii <kojiishi at gmail.com>
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.