Bug 42999 – Add a Chromium WebKit API for drawing text

• Component: WebKit API
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

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.

[Darin Fisher (:fishd, Google)]

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.

[Brett Wilson (Google)]

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

[Darin Fisher (:fishd, Google)]

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

[Brett Wilson (Google)]

Created attachment 62740 [details]
Patch v3

Comments addressed.

[WebKit Review Bot]

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.

[Brett Wilson (Google)]

Created attachment 62741 [details]
Patch v4

Pacify the style bot

[Darin Fisher (:fishd, Google)]

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

[Brett Wilson (Google)]

Created attachment 62744 [details]
Final patch for reference

[Brett Wilson (Google)]

Fixed in r64160