Bug 81332 – Cache support for OpenTypeVerticalData

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Koji Ishii]

Vertical flow-related data read from OpenType fonts are per-OpenType font, and there's no class in WebKit that represents an OpenType font file instance. SimpleFontData and PlatformFontData are created per size and so forth, so creating OpenTypeVerticalData instance (see bug 81326) per SimpleFontData or PlatformFontData wastes memory.

This bug is for patches all platforms that use OpenTypeVerticalData can share. Patches only for CGWin is in bug 48459.

[Koji Ishii]

Created attachment 132330 [details]
Cache support for OpenTypeVerticalData (in progress)

This patch requires bug 81326 applied.

[Koji Ishii]

Created attachment 133797 [details]
Cache support for OpenTypeVerticalData (done except ChangeLog)

I've got a feedback to my private build that we should ignore leading "@" in the font name so that existing pages won't break. The fix was incorporated into this patch.

[Koji Ishii]

Created attachment 135002 [details]
Cache support for OpenTypeVerticalData

Rather large changes since the last patch:
* Started using ENABLE_OPENTYPE_VERTICAL flag because #if's have started to spread around
* Changed how we collect unused OpenTypeVerticalData for more efficient way
* A few more cleanups and fixes

[Koji Ishii]

Created attachment 135005 [details]
Cache support for OpenTypeVerticalData

The last touch to support non-OpenType fonts (though Apple Windows port doesn't seem to support non-OpenType/TrueType fonts)

[Kenichi Ishibashi]

Comment on attachment 135005 [details]
Cache support for OpenTypeVerticalData

View in context: https://bugs.webkit.org/attachment.cgi?id=135005&action=review

Drive-by comments.

This patch adds massive #ifdefs and I don't think it's a good idea. I think most code added to FontCache can be placed in Source/WebCore/platform/graphics/opentype. For example, you can create a singleton class that manages OpenTypeVerticalData cache or can add static functions to OpenTypeVerticalData. This way you can remove most of #ifdefs except for the one in FontCache::purgeInactiveFontData(). (I'm not sure this function is the right place to do mark & sweep unused verticalData)

> Source/WebCore/platform/graphics/FontCache.cpp:195
> +        return getCachedFontPlatformData(fontDescription, AtomicString(familyName.impl()->substring(1)), checkingAlternateName);

I recommend removing all "@"s in the front name before to avoid unnecessary recursion.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h:61
> +    bool m_inFontCache : 1;

Why this filed is a bit-field?

[Koji Ishii]

Created attachment 137552 [details]
reflected changes from Kenichi's review (removed bit-field)

Two changes from the previous patch:
* Changed bit-field to normal field
* Brought "Reviewed by..." to before description as in review comments in bug 81389

[Koji Ishii]

Comment on attachment 135005 [details]
Cache support for OpenTypeVerticalData

View in context: https://bugs.webkit.org/attachment.cgi?id=135005&action=review

>> Source/WebCore/platform/graphics/FontCache.cpp:195
>> +        return getCachedFontPlatformData(fontDescription, AtomicString(familyName.impl()->substring(1)), checkingAlternateName);
> 
> I recommend removing all "@"s in the front name before to avoid unnecessary recursion.

While I agree with you as a general comment, in this specific case, family name having two or more leading '@' is very unlikely. If that assumption stands, I suppose this code runs more efficient than removing all leading '@' before the call. I can't do it without adding cycles on single case, can I, or do you think I'm assuming too much?

>> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h:61
>> +    bool m_inFontCache : 1;
> 
> Why this filed is a bit-field?

Ah, right, it doesn't save any for now. I'll remove it.

[Kenichi Ishibashi]

Comment on attachment 135005 [details]
Cache support for OpenTypeVerticalData

View in context: https://bugs.webkit.org/attachment.cgi?id=135005&action=review

>>> Source/WebCore/platform/graphics/FontCache.cpp:195
>>> +        return getCachedFontPlatformData(fontDescription, AtomicString(familyName.impl()->substring(1)), checkingAlternateName);
>> 
>> I recommend removing all "@"s in the front name before to avoid unnecessary recursion.
> 
> While I agree with you as a general comment, in this specific case, family name having two or more leading '@' is very unlikely. If that assumption stands, I suppose this code runs more efficient than removing all leading '@' before the call. I can't do it without adding cycles on single case, can I, or do you think I'm assuming too much?

I concerned malicious inputs like "@@@.......". If my understanding is correct, familyName comes from user input. Does the spec (or implementation) limit the length of font name?

[Koji Ishii]

Comment on attachment 135005 [details]
Cache support for OpenTypeVerticalData

View in context: https://bugs.webkit.org/attachment.cgi?id=135005&action=review

>>>> Source/WebCore/platform/graphics/FontCache.cpp:195
>>>> +        return getCachedFontPlatformData(fontDescription, AtomicString(familyName.impl()->substring(1)), checkingAlternateName);
>>> 
>>> I recommend removing all "@"s in the front name before to avoid unnecessary recursion.
>> 
>> While I agree with you as a general comment, in this specific case, family name having two or more leading '@' is very unlikely. If that assumption stands, I suppose this code runs more efficient than removing all leading '@' before the call. I can't do it without adding cycles on single case, can I, or do you think I'm assuming too much?
> 
> I concerned malicious inputs like "@@@.......". If my understanding is correct, familyName comes from user input. Does the spec (or implementation) limit the length of font name?

You're correct and we need to work either way. The point I'm making is which scenario we want our code optimized for. The current code runs better on single case and worse on more. Your proposal make the code better for multiple case by adding slightly additional code on single case. I think we shouldn't try to run fast or efficiently against malicious input, especially if doing so adds cost to the normal code path. I agree that we shouldn't crash though. Is this agreeable?

[Kenichi Ishibashi]

(In reply to comment #9)

> You're correct and we need to work either way. The point I'm making is which scenario we want our code optimized for. The current code runs better on single case and worse on more. Your proposal make the code better for multiple case by adding slightly additional code on single case. I think we shouldn't try to run fast or efficiently against malicious input, especially if doing so adds cost to the normal code path. I agree that we shouldn't crash though. Is this agreeable?

I should have described what I concerned more precisely. I didn't mean that we should make the code fast for malicious and rare inputs. If you believe the code won't cause crash, then I have no objection.

[Koji Ishii]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > You're correct and we need to work either way. The point I'm making is which scenario we want our code optimized for. The current code runs better on single case and worse on more. Your proposal make the code better for multiple case by adding slightly additional code on single case. I think we shouldn't try to run fast or efficiently against malicious input, especially if doing so adds cost to the normal code path. I agree that we shouldn't crash though. Is this agreeable?
> 
> I should have described what I concerned more precisely. I didn't mean that we should make the code fast for malicious and rare inputs. If you believe the code won't cause crash, then I have no objection.

Thank you for the clarification. It just makes another recursive call on malicious input, which isn't convertible to tail recursion due to requiring destructor unfortunately. It shouldn't crash unless we run out of stack.

I actually tried to write code to do that with loops without adding cycles to the normal code path, but I didn't get it done, so this looks the best way to go to me.

[Koji Ishii]

Created attachment 137841 [details]
Changed how it removes leading "@" from family name

Sorry for back and forth but after more thoughts, I found one of my claim was invalid.　There are 3 cases here:
1. No leading "@"
2. One leading "@"
3. Two or more leading "@"
99% of the cases run the code for 1. Within the rest of 1%, 99% run the code for 2.
So adding machine cycles to case 1 in order to make case 3 faster doesn't make sense, but adding to case 2 isn't a big deal, especially if it can save from stack overflow on really huge number of leading "@".
I was mistaken between case 1　and 2, sorry about that, and now the new patch uses a loop within case 2 to remove the leading "@". Thank you for the review to find this.

[Ryosuke Niwa]

Comment on attachment 137841 [details]
Changed how it removes leading "@" from family name

View in context: https://bugs.webkit.org/attachment.cgi?id=137841&action=review

> Source/WebCore/platform/graphics/FontCache.cpp:196
> +        for (unsigned c = familyName.length(); i < c && familyName[i] == '@'; ++i) { }

This code is going to remove all @'s at the beginning.
Also, please don't use one-letter variable like "c". length will be better.

> Source/WebCore/platform/graphics/FontCache.cpp:197
> +        return getCachedFontPlatformData(fontDescription, AtomicString(familyName.impl()->substring(i)), checkingAlternateName);

I would have renamed familyName to something else like passedFamilyName, and did:
const AtomicString& familyName = stripLeadingVerticalFlowFlag(familyName);
where stripLeadingVerticalFlowFlag is an inline function that strips the leading '@' in Windows and no-op on other platforms.

> Source/WebCore/platform/graphics/FontCache.cpp:237
> +typedef HashMap<AtomicString, OpenTypeVerticalData*> FontVerticalDataCache;
> +
> +static FontVerticalDataCache* gFontVerticalDataCache = 0;

We typically use DEFINE_STATIC_LOCAL and declare these variables inside a function.

> Source/WebCore/platform/graphics/FontCache.cpp:251
> +    OpenTypeVerticalData* verticalData = new OpenTypeVerticalData(platformData);

We should probably use OwnPtr in the future. Although we might want to stick with new/delete for now since the rest of the file uses that pattern.

[Ryosuke Niwa]

Comment on attachment 137841 [details]
Changed how it removes leading "@" from family name

View in context: https://bugs.webkit.org/attachment.cgi?id=137841&action=review

> Source/WebCore/platform/graphics/FontCache.cpp:196
> +        for (unsigned c = familyName.length(); i < c && familyName[i] == '@'; ++i) { }

This code is going to remove all @'s at the beginning.
Also, please don't use one-letter variable like "c". length will be better.

> Source/WebCore/platform/graphics/FontCache.cpp:197
> +        return getCachedFontPlatformData(fontDescription, AtomicString(familyName.impl()->substring(i)), checkingAlternateName);

I would have renamed familyName to something else like passedFamilyName, and did:
const AtomicString& familyName = stripLeadingVerticalFlowFlag(familyName);
where stripLeadingVerticalFlowFlag is an inline function that strips the leading '@' in Windows and no-op on other platforms.

> Source/WebCore/platform/graphics/FontCache.cpp:237
> +typedef HashMap<AtomicString, OpenTypeVerticalData*> FontVerticalDataCache;
> +
> +static FontVerticalDataCache* gFontVerticalDataCache = 0;

We typically use DEFINE_STATIC_LOCAL and declare these variables inside a function.

> Source/WebCore/platform/graphics/FontCache.cpp:251
> +    OpenTypeVerticalData* verticalData = new OpenTypeVerticalData(platformData);

We should probably use OwnPtr in the future. Although we might want to stick with new/delete for now since the rest of the file uses that pattern.

[Koji Ishii]

Created attachment 152501 [details]
Reflected changes from rniwa's review

[Koji Ishii]

(In reply to comment #14)
> > Source/WebCore/platform/graphics/FontCache.cpp:197
> > +        return getCachedFontPlatformData(fontDescription, AtomicString(familyName.impl()->substring(i)), checkingAlternateName);
> 
> I would have renamed familyName to something else like passedFamilyName, and did:
> const AtomicString& familyName = stripLeadingVerticalFlowFlag(familyName);
> where stripLeadingVerticalFlowFlag is an inline function that strips the leading '@' in Windows and no-op on other platforms.

I tried inline function, but returning const AtomicString& of locally created object crashes, so I followed your advice but the code lives within the function.


> > Source/WebCore/platform/graphics/FontCache.cpp:237
> > +typedef HashMap<AtomicString, OpenTypeVerticalData*> FontVerticalDataCache;
> > +
> > +static FontVerticalDataCache* gFontVerticalDataCache = 0;
> 
> We typically use DEFINE_STATIC_LOCAL and declare these variables inside a function.

Thanks for the advise, changed to follow the pattern.


> > Source/WebCore/platform/graphics/FontCache.cpp:251
> > +    OpenTypeVerticalData* verticalData = new OpenTypeVerticalData(platformData);
> 
> We should probably use OwnPtr in the future. Although we might want to stick with
> new/delete for now since the rest of the file uses that pattern.

Yeah, that was what I thought. Glad to know you thought the same way.

The updated patch is uploaded and passed EWS, appreciate your support to go further.

[Tony Chang]

Comment on attachment 152501 [details]
Reflected changes from rniwa's review

View in context: https://bugs.webkit.org/attachment.cgi?id=152501&action=review

> Source/WebCore/platform/graphics/FontCache.cpp:40
> +#if ENABLE(OPENTYPE_VERTICAL)
> +#include "OpenTypeVerticalData.h"
> +#endif

In WebKit, it's more common to put the #if ENABLE check in the .h file and unconditionally include the .h file everywhere.

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

Can this be HashMap<AtomicString, OwnPtr<OpenTypeVerticalData> > ?

> Source/WebCore/platform/graphics/FontCache.cpp:446
> +        for (size_t i = 0, count = keysToRemove.size(); i < count; ++i)
> +            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?

> Source/WebCore/platform/graphics/FontCache.h:54
> +#if ENABLE(OPENTYPE_VERTICAL)
> +class OpenTypeVerticalData;
> +#endif

You don't need the #if here. It should be safe to forward declare something that is unused.

[Koji Ishii]

Comment on attachment 152501 [details]
Reflected changes from rniwa's review

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.

[Tony Chang]

Comment on attachment 152501 [details]
Reflected changes from rniwa's review

View in context: https://bugs.webkit.org/attachment.cgi?id=152501&action=review

>>> 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?

Ref counting sounds even slower (more work overall, although it would be spread out more).  It seems slow to have to walk gFontDataCache an extra time, but I can't think of a way to avoid this so using GC here is OK.  In the worse case, we also walk fontVerticalDataCache 3 times, but I guess this will often be only a few entries.

[Koji Ishii]

Comment on attachment 152501 [details]
Reflected changes from rniwa's review

View in context: https://bugs.webkit.org/attachment.cgi?id=152501&action=review

>>>> 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?
> 
> Ref counting sounds even slower (more work overall, although it would be spread out more).  It seems slow to have to walk gFontDataCache an extra time, but I can't think of a way to avoid this so using GC here is OK.  In the worse case, we also walk fontVerticalDataCache 3 times, but I guess this will often be only a few entries.

Yeah, agreed. Thanks for your opinion. I'll upload the modified patch soon.

[Koji Ishii]

Created attachment 153703 [details]
Reflected items from Tony's review

[Gyuyoung Kim]

Comment on attachment 153703 [details]
Reflected items from Tony's review

Attachment 153703 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13323031

[Early Warning System Bot]

Comment on attachment 153703 [details]
Reflected items from Tony's review

Attachment 153703 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13308896

[Early Warning System Bot]

Comment on attachment 153703 [details]
Reflected items from Tony's review

Attachment 153703 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13311904

[Koji Ishii]

Removing #if ENABLE(OPENTYPE_VERTICAL) for #include "OpenTypeVerticalData.h" breaks qt and efl build. I need to look into how not to break them without using #if ENABLE(OPENTYPE_VERTICAL).

[Tony Chang]

(In reply to comment #25)
> Removing #if ENABLE(OPENTYPE_VERTICAL) for #include "OpenTypeVerticalData.h" breaks qt and efl build. I need to look into how not to break them without using #if ENABLE(OPENTYPE_VERTICAL).

You need to make sure platform/graphics/opentype is in the include path for the Qt and Efl ports.

[Koji Ishii]

Created attachment 155133 [details]
Try to fix qt build from the last change

[Koji Ishii]

Created attachment 155134 [details]
Resolved merge conflict in the previous patch

[Early Warning System Bot]

Comment on attachment 155134 [details]
Resolved merge conflict in the previous patch

Attachment 155134 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13375923

[Early Warning System Bot]

Comment on attachment 155134 [details]
Resolved merge conflict in the previous patch

Attachment 155134 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13381265

[Koji Ishii]

Created attachment 155153 [details]
Still struggling to fix build after #ifdef for a #include was removed

[Early Warning System Bot]

Comment on attachment 155153 [details]
Still struggling to fix build after #ifdef for a #include was removed

Attachment 155153 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13380725

[Early Warning System Bot]

Comment on attachment 155153 [details]
Still struggling to fix build after #ifdef for a #include was removed

Attachment 155153 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13390002

[Koji Ishii]

Created attachment 155164 [details]
Yet another try to add a new file to Qt build

bashi suggested me that Target.pri is the file Qt uses to build, and as I read it, I found it includes WebCore.pri which defines INCLUDEPATH, so I fixed that too.

[Koji Ishii]

Comment on attachment 155164 [details]
Yet another try to add a new file to Qt build

Reflected items from Tony's review

[Kenichi Ishibashi]

Comment on attachment 155164 [details]
Yet another try to add a new file to Qt build

View in context: https://bugs.webkit.org/attachment.cgi?id=155164&action=review

> Source/WebCore/GNUmakefile.list.am:3409
> +	Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h \

nit: Case-sensitive dictionary order.

> Source/WebCore/Target.pri:2094
> +    platform/graphics/opentype/OpenTypeVerticalData.h \

Ditto.

[Koji Ishii]

(In reply to comment #36)
> (From update of attachment 155164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155164&action=review
> 
> > Source/WebCore/GNUmakefile.list.am:3409
> > +	Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h \
> 
> nit: Case-sensitive dictionary order.
> 
> > Source/WebCore/Target.pri:2094
> > +    platform/graphics/opentype/OpenTypeVerticalData.h \
> 
> Ditto.

Are you sure about this? I'm ok with either, but the rest of the files look case-insensitive dictionary order to me; "Float" comes after "filter".

[Kenichi Ishibashi]

Comment on attachment 155164 [details]
Yet another try to add a new file to Qt build

View in context: https://bugs.webkit.org/attachment.cgi?id=155164&action=review

>>> Source/WebCore/GNUmakefile.list.am:3409
>>> +	Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h \
>> 
>> nit: Case-sensitive dictionary order.
> 
> Are you sure about this? I'm ok with either, but the rest of the files look case-insensitive dictionary order to me; "Float" comes after "filter".

Looks like GNUmakefile.list.am is inconsistent about ordering. See platform/graphics/cairo for example. I'm ok if you won't add further files under platform/graphics/opentype/, but I want to see files under opentype/ at the same place.

[Kenichi Ishibashi]

(In reply to comment #38)
> (From update of attachment 155164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155164&action=review
> 
> >>> Source/WebCore/GNUmakefile.list.am:3409
> >>> +	Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h \
> >> 
> >> nit: Case-sensitive dictionary order.
> > 
> > Are you sure about this? I'm ok with either, but the rest of the files look case-insensitive dictionary order to me; "Float" comes after "filter".
> 
> Looks like GNUmakefile.list.am is inconsistent about ordering. See platform/graphics/cairo for example. I'm ok if you won't add further files under platform/graphics/opentype/, but I want to see files under opentype/ at the same place.

Don't mind. I looked the entire of the file and realized you are right in general.

[Tony Chang]

Comment on attachment 155164 [details]
Yet another try to add a new file to Qt build

View in context: https://bugs.webkit.org/attachment.cgi?id=155164&action=review

I think this is OK to land.

It's a bit unfortunate that it's a completely separate cache and makes FontCache.cpp more complex even though it's only going to be used on Windows, but I don't know of a better place for it.

It would be awesome if you did a follow up patch to switch the whole file to using OwnPtrs instead of new/delete.

> Source/WebCore/platform/graphics/FontCache.cpp:192
> +    const AtomicString& familyName = passedFamilyName.isEmpty() || passedFamilyName[0] != '@' ?

Nit: I would include parentheses around the the ternary condition.

[Koji Ishii]

Created attachment 155668 [details]
Included parentheses around the the ternary condition

The only diff from the last patch is to include parentheses around the the ternary condition.

[WebKit Review Bot]

Comment on attachment 155164 [details]
Yet another try to add a new file to Qt build

Cleared Tony Chang's review+ from obsolete attachment 155164 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Koji Ishii]

> (From update of attachment 155164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155164&action=review
> 
> I think this is OK to land.
> 
> It's a bit unfortunate that it's a completely separate cache and makes FontCache.cpp more complex even though it's only going to be used on Windows, but I don't know of a better place for it.

Yeah, agreed. I actually was surprised by finding there's no cache per font file in WebKit today. I guess there will be such needs from other than vertical flow on Windows and I hope the code can be re-used in such instances.

> It would be awesome if you did a follow up patch to switch the whole file to using OwnPtrs instead of new/delete.

I entered bug 92808. I'll be working on bug 51450 first, but I'll also try to submit a patch for 92808.

> > Source/WebCore/platform/graphics/FontCache.cpp:192
> > +    const AtomicString& familyName = passedFamilyName.isEmpty() || passedFamilyName[0] != '@' ?
> 
> Nit: I would include parentheses around the the ternary condition.

Submitted updated patch that fixes this.

[WebKit Review Bot]

Comment on attachment 155668 [details]
Included parentheses around the the ternary condition

Rejecting attachment 155668 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Merge conflict in LayoutTests/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [chromium] Use the event bounding box when constructing a PlatformEvent::GestureTap

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13401630

Comment on attachment 155668 [details]
Included parentheses around the the ternary condition

Clearing flags on attachment: 155668

Committed r124397: <http://trac.webkit.org/changeset/124397>

All reviewed patches have been landed.  Closing bug.