[webkit-reviews] review denied: [Bug 42999] Add a Chromium WebKit API for drawing text : [Attachment 62727] Patch v2 w/ ChangeLog

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied  review:
Bug 42999: Add a Chromium WebKit API for drawing text
https://bugs.webkit.org/show_bug.cgi?id=42999

Attachment 62727: Patch v2 w/ ChangeLog
https://bugs.webkit.org/attachment.cgi?id=62727&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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