Bug 26529 – [Chromium] Pipe getFontDataForCharacters via ChromiumBridge

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Langley]

This is hopefully the last step before our renderers can run cleanly in a chroot.

WebKit needs to be able to ask for the correct font to use in the case that the current font doesn't include glyphs for certain code points. Currently we make a fontconfig call in our WebKit port to handle this.

This patch changes this so that the call is sent our via ChromiumBridge.

http://codereview.chromium.org/132007

[Adam Langley]

Created attachment 31520 [details]
patch

[David Levin]

Comment on attachment 31520 [details]
patch

I'm leaving the "real" review for Darin Fisher, but I'll give you some style things to fix up to make it go faster.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> [phone]  Adam Langley  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Typically the link to the bug is near the top of the change log entry.  (I did see you put it in bug it way at the bottom.)



> diff --git a/WebCore/platform/chromium/ChromiumBridge.h b/WebCore/platform/chromium/ChromiumBridge.h
> +        static String getFontFamilyForCharacters(const UChar* characters, int numCharacters);

"characters" doesn't seem to add any information here due to the api name and the const UChar* type.
So it would be good to remove it.  (WebKit style is to not put in param names when they don't add information.)



> diff --git a/WebCore/platform/graphics/chromium/FontCacheLinux.cpp b/WebCore/platform/graphics/chromium/FontCacheLinux.cpp
> +    String family = ChromiumBridge::getFontFamilyForCharacters(characters, length);
> +    if (family.isEmpty())
> +        return NULL;

Use 0 instead of NULL.

[Darin Fisher (:fishd, Google)]

Comment on attachment 31520 [details]
patch

My only comment, in addition to what Dave wrote, is that numCharacters should probably be size_t instead of int.  Post a revised patch and LGTM.

[Adam Langley]

Created attachment 31791 [details]
patch

Hopefully have addressed all comments

[Darin Fisher (:fishd, Google)]

Did you attach the right patch?

[Adam Langley]

Created attachment 31798 [details]
patch

> Did you attach the right patch?

Opps, sorry.