Bug 73452 – [Chromium] Add the FontCache implementation for Android

• Component: Platform
• Status: RESOLVED FIXED

[Peter Beverloo]

Add the FontCache implementation specific for the Chromium WebKit port on Android, and include various font-related files intended for Linux which can be re-used by us.

[Peter Beverloo]

Created attachment 117200 [details]
Patch

[Adam Barth]

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 (

[Peter Beverloo]

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.

[WebKit Review Bot]

Comment on attachment 117389 [details]
Patch for landing

Clearing flags on attachment: 117389

Committed r101704: <http://trac.webkit.org/changeset/101704>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.