Bug 73452 - [Chromium] Add the FontCache implementation for Android
Summary: [Chromium] Add the FontCache implementation for Android
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peter Beverloo
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2011-11-30 08:36 PST by Peter Beverloo
Modified: 2011-12-01 14:08 PST (History)
6 users (show)

See Also:


Attachments
Patch (11.84 KB, patch)
2011-11-30 08:37 PST, Peter Beverloo
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (11.64 KB, patch)
2011-12-01 03:10 PST, Peter Beverloo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Beverloo 2011-11-30 08:36:26 PST
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.
Comment 1 Peter Beverloo 2011-11-30 08:37:37 PST
Created attachment 117200 [details]
Patch
Comment 2 Adam Barth 2011-11-30 10:24:06 PST
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 (
Comment 3 Peter Beverloo 2011-12-01 03:10:27 PST
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 4 WebKit Review Bot 2011-12-01 14:08:30 PST
Comment on attachment 117389 [details]
Patch for landing

Clearing flags on attachment: 117389

Committed r101704: <http://trac.webkit.org/changeset/101704>
Comment 5 WebKit Review Bot 2011-12-01 14:08:35 PST
All reviewed patches have been landed.  Closing bug.