We would like to hook up the WebKit port's text drawing to random canvases, so we should expose something in the Chromium WebKit API to do this.
Created attachment 62603 [details] First patch This is missing a ChangeLog (currently fixing my checkout and can't run prepare-Changelog) but I'm hoping to get some initial feedback to get this started.
unlike webcore, the style for enums at the webkit layer is of the form: enum Foo { FooBar, FooBaz }; so, ... enum Family { FamilyNone, FamilyStandard, ... }; enum Smoothing { SmoothingAuto, SmoothingNone, ... }; enum Weight { Weight100, ... }; i would probably just create WebTextRun instead of making it be an inner class of WebFont. it seems like it need not be specific to WebFont. i have some more nits, but the review tool is not letting me annotate the patch right now.
Created attachment 62727 [details] Patch v2 w/ ChangeLog This renames the enums, adds letter and word spacing, and adds assertions that make sure the WebKit API matches WebCore's enums
Comment on attachment 62727 [details] Patch v2 w/ ChangeLog WebKit/chromium/public/WebFont.h:49 + WEBKIT_API static WebFont* Create(const WebFontDescription&); nit: Create -> create how do I destroy a WebFont? i don't see a virtual destructor. comments below on the interface are made with ignorance about what the .cpp file looks like. i wanted to see if the API made sense to me without knowledge of how it is implemented. WebKit/chromium/public/WebFont.h:59 + virtual void drawText(WebCanvas*, const WebTextRun&, const WebFloatPoint&, WebColor, const WebRect& clip, bool canvasIsOpaque, it might be helpful to document some of these parameters. it is not clear to me what from and to mean. are they specifying subranges of WebTextRun? in which case fromChar, toChar might be better. maybe you should give a name to the WebFloatPoint parameter, or at least describe what it is. i'm guessing it is the origin of the text run, but does that mean top-left? WebKit/chromium/public/WebFont.h:61 + virtual int width(const WebTextRun&) const = 0; nit: calculateWidth might be a nicer name WebKit/chromium/public/WebFont.h:62 + virtual int offsetForPosition(const WebTextRun&, float position, bool includePartialGlyphs) const = 0; i think this method could use some documentation. is this identifying the character offset? WebKit/chromium/public/WebFontDescription.h:85 + #if WEBKIT_IMPLEMENTATION nit: we have a convention of making the WEBKIT_IMPLEMENTATION section be the last part of the public section (or private section if there is one). i would just group this one with the conversion operator. WebKit/chromium/public/WebFontDescription.h:97 + // Not members of WebCore::FontDescription, but used in the places where we nit: i tend to avoid comments in the WebKit API headers that refer to WebCore since they can get out-of-date easily. no one making changes to WebCore will ever think to update WebKit API headers. it is better to just make the comments standalone and make sense without knowledge of WebCore. WebKit/chromium/public/WebTextRun.h:43 + WebTextRun(const WebString& t, bool isRtl, bool hasDirectionalOverride) style-nit: isRTL <- webkit style is to capitalize acronyms unless they are at the start of a term. WebKit/chromium/public/WebTextRun.h:54 + #ifdef WEBKIT_IMPLEMENTATION same nit about moving this to the end of the "public" section. WebKit/chromium/src/AssertMatchingEnums.cpp:333 + COMPILE_ASSERT_MATCHING_ENUM(WebFontDescription::GenericFamilyNone, FontDescription::NoFamily); nit: insert in sorted order. it makes it easier to keep this file organized. WebKit/chromium/src/WebFontDescription.cpp:52 + WebFontDescription::operator WebCore::FontDescription() const { nit: opening bracket on the next line WebKit/chromium/src/WebFontDescription.cpp:35 + we usually add a 'using namespace WebCore' so that we can avoid the WebCore:: prefix down below. WebKit/chromium/src/WebFontImpl.cpp:93 + // FIXME(brettw) hook canvasIsOpaque up to the platform-specific indicators nit: FIXME(foo) -> FIXME WebKit/chromium/src/WebTextRun.cpp:42 + return WebCore::TextRun(text, false, 0, 0, rtl, directionalOverride); nit: you can drop the WebCore:: prefix
Created attachment 62740 [details] Patch v3 Comments addressed.
Attachment 62740 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit/chromium/public/WebFontDescription.h:104: One space before end of line comments [whitespace/comments] [5] WebKit/chromium/src/WebFontDescription.cpp:56: font_family is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 62741 [details] Patch v4 Pacify the style bot
Comment on attachment 62741 [details] Patch v4 WebKit/chromium/public/WebFont.h:51 + WEBKIT_API static WebFont* Create(const WebFontDescription&); nit: Create -> create WebKit/chromium/public/WebFont.h:70 + // run to draw. If |to| is -1, the enture run will be drawn. nit: enture -> entire WebKit/chromium/public/WebFont.h:87 + // |leftBaseline| position. |h| indicates the height of the selection rect nit: h -> height WebKit/chromium/public/WebTextRun.h:43 + WebTextRun(const WebString& t, bool isRtl, bool hasDirectionalOverride) nit: isRtl -> isRTL so this can compile :-) WebKit/chromium/src/WebTextRun.cpp:40 + WebTextRun::operator WebCore::TextRun() const nit: no need for WebCore:: prefix R=me with those nits fixed
Created attachment 62744 [details] Final patch for reference
Fixed in r64160