Description
Koji Ishii
2012-03-16 04:41:54 PDT
Created attachment 132330 [details] Cache support for OpenTypeVerticalData (in progress) This patch requires bug 81326 applied. 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.
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
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)
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? 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 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. 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? 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? (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. (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. 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.
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. 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. Created attachment 152501 [details]
Reflected changes from rniwa's review
(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. 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. 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. 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. 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. Created attachment 153703 [details]
Reflected items from Tony's review
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 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 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 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). (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. Created attachment 155133 [details]
Try to fix qt build from the last change
Created attachment 155134 [details]
Resolved merge conflict in the previous patch
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 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 Created attachment 155153 [details]
Still struggling to fix build after #ifdef for a #include was removed
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 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 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.
Comment on attachment 155164 [details]
Yet another try to add a new file to Qt build
Reflected items from Tony's review
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. (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". 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. (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. 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. 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.
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. > (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. 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. |