[webkit-reviews] review granted: [Bug 23340] Add remaining bits of platform/graphics/chromium : [Attachment 26744] patch v1

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 23340: Add remaining bits of platform/graphics/chromium
https://bugs.webkit.org/show_bug.cgi?id=23340

Attachment 26744: patch v1
https://bugs.webkit.org/attachment.cgi?id=26744&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Indent (and static_cast)
 static inline float scaleEmToUnits(float x, int unitsPerEm)
 50 {
 51   return unitsPerEm ? x / (float)unitsPerEm : x;
 52 }

No need for the c-style cast:
126	// TEXTMETRICS have this.  Set m_treatAsFixedPitch based off that.
 127	 HDC dc = GetDC((HWND)0);
0 works fine w/o cast

This code is copied at least twice:
59     TEXTMETRIC textMetric = {0};
 60	if (!GetTextMetrics(dc, &textMetric)) {
 61	    if (ChromiumBridge::ensureFontLoaded(m_font.hfont())) {
 62		// Retry GetTextMetrics.
 63		// FIXME: Handle gracefully the error if this call also fails.
 64		// See http://crbug.com/6401.
 65		if (!GetTextMetrics(dc, &textMetric))
 66		    ASSERT_NOT_REACHED();
 67	    }
 68	}
maybe it should be a static function?

Darin fisher learned I was wrong here:
   else
 71	    // hack taken from the Windows port
 72	    m_xHeight = static_cast<float>(m_ascent) * 0.56;
comments in else/if blocks supposedly cause them to be treated as multi-line by
the style guides.  At least that was darin adlers opinion which darin fisher
learned earlier this week.

spacing:
 static bool treatAsSpace(UChar c)
 46 {
 47    return c == ' ' || c == '\t' || c == '\n' || c == 0x00A0;
 48 }
 49 

numItems:
 478	     int num_items = 0;
I think the code really means numberOfGlyphs? but I'm not sure.

{ on next line:
5     virtual bool nextWinFontData(HFONT*, SCRIPT_CACHE**,
SCRIPT_FONTPROPERTIES**, int* ascent) {
 346	     return false;
 347	 }

Looks fine.