Bug 26529 - [Chromium] Pipe getFontDataForCharacters via ChromiumBridge
Summary: [Chromium] Pipe getFontDataForCharacters via ChromiumBridge
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Adam Langley
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-18 17:19 PDT by Adam Langley
Modified: 2009-06-25 16:19 PDT (History)
0 users

See Also:


Attachments
patch (3.81 KB, patch)
2009-06-18 17:23 PDT, Adam Langley
fishd: review-
Details | Formatted Diff | Diff
patch (2.12 KB, patch)
2009-06-24 10:43 PDT, Adam Langley
no flags Details | Formatted Diff | Diff
patch (3.74 KB, patch)
2009-06-24 11:41 PDT, Adam Langley
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Langley 2009-06-18 17:19:41 PDT
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
Comment 1 Adam Langley 2009-06-18 17:23:09 PDT
Created attachment 31520 [details]
patch
Comment 2 David Levin 2009-06-23 15:58:13 PDT
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
> +2009-06-18  Adam Langley  <agl@google.com>
> +
> +        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.
Comment 3 Darin Fisher (:fishd, Google) 2009-06-23 22:40:48 PDT
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.
Comment 4 Adam Langley 2009-06-24 10:43:18 PDT
Created attachment 31791 [details]
patch

Hopefully have addressed all comments
Comment 5 Darin Fisher (:fishd, Google) 2009-06-24 11:36:25 PDT
Did you attach the right patch?
Comment 6 Adam Langley 2009-06-24 11:41:52 PDT
Created attachment 31798 [details]
patch

> Did you attach the right patch?

Opps, sorry.