Summary: | [Chromium] Pipe getFontDataForCharacters via ChromiumBridge | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Langley <agl> | ||||||||
Component: | Layout and Rendering | Assignee: | Adam Langley <agl> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | ||||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Adam Langley
2009-06-18 17:19:41 PDT
Created attachment 31520 [details]
patch
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 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.
Created attachment 31791 [details]
patch
Hopefully have addressed all comments
Did you attach the right patch? Created attachment 31798 [details] patch > Did you attach the right patch? Opps, sorry. |