Created attachment 117200 [details] Patch

Comment on attachment 117200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117200&action=review > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:103 > +const SimpleFontData* FontCache::getFontDataForCharacters(const Font& font, > + const UChar* characters, > + int length) One line, please. > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:124 > + DEFINE_STATIC_LOCAL(const AtomicString, sansStr, ("Sans")); > + DEFINE_STATIC_LOCAL(const AtomicString, serifStr, ("Serif")); > + DEFINE_STATIC_LOCAL(const AtomicString, monospaceStr, ("Monospace")); Please use complete words in variable names. > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:145 > +void FontCache::getTraitsInFamily(const AtomicString& familyName, > + Vector<unsigned>& traitsMasks) One line. > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:151 > +FontPlatformData* FontCache::createFontPlatformData(const FontDescription& fontDescription, > + const AtomicString& family) One line. > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:183 > + typeface->unref(); Can typeface be a RefPtr? > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:191 > + (style & SkTypeface::kBold) && !typeface->isBold(), > + (style & SkTypeface::kItalic) && !typeface->isItalic(), > + fontDescription.orientation(), > + fontDescription.textOrientation()); I would indent this to match the (

Created attachment 117389 [details] Patch for landing Thank you for the review! (In reply to comment #2) > (From update of attachment 117200 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117200&action=review > > > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:103 > > +const SimpleFontData* FontCache::getFontDataForCharacters(const Font& font, > > + const UChar* characters, > > + int length) > > One line, please. Done > > > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:124 > > + DEFINE_STATIC_LOCAL(const AtomicString, sansStr, ("Sans")); > > + DEFINE_STATIC_LOCAL(const AtomicString, serifStr, ("Serif")); > > + DEFINE_STATIC_LOCAL(const AtomicString, monospaceStr, ("Monospace")); > > Please use complete words in variable names. Done, also sorted them by their time of usage. > > > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:145 > > +void FontCache::getTraitsInFamily(const AtomicString& familyName, > > + Vector<unsigned>& traitsMasks) > > One line. Done > > > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:151 > > +FontPlatformData* FontCache::createFontPlatformData(const FontDescription& fontDescription, > > + const AtomicString& family) > > One line. Done > > > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:183 > > + typeface->unref(); > > Can typeface be a RefPtr? I don't think so, as the reference counting is already being managed by Skia in SkRefCnt (which SkTypeface extends from). > > > Source/WebCore/platform/graphics/chromium/FontCacheAndroid.cpp:191 > > + (style & SkTypeface::kBold) && !typeface->isBold(), > > + (style & SkTypeface::kItalic) && !typeface->isItalic(), > > + fontDescription.orientation(), > > + fontDescription.textOrientation()); > > I would indent this to match the ( Done.

Comment on attachment 117389 [details] Patch for landing Clearing flags on attachment: 117389 Committed r101704: <http://trac.webkit.org/changeset/101704>

All reviewed patches have been landed. Closing bug.