[webkit-reviews] review granted: [Bug 73452] [Chromium] Add the FontCache implementation for Android : [Attachment 117200] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Peter Beverloo
<peter at chromium.org>'s request for review:
Bug 73452: [Chromium] Add the FontCache implementation for Android
https://bugs.webkit.org/show_bug.cgi?id=73452

Attachment 117200: Patch
https://bugs.webkit.org/attachment.cgi?id=117200&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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 (