[webkit-reviews] review denied: [Bug 20542] Adding EOT Font Rendering capability along the 'GDI olny' path : [Attachment 23022] A patch for supporting EOT rendering along GDI path

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Prunthaban <prunthaban at google.com>'s
request for review:
Bug 20542: Adding EOT Font Rendering capability along the 'GDI olny' path
https://bugs.webkit.org/show_bug.cgi?id=20542

Attachment 23022: A patch for supporting EOT rendering along GDI path
https://bugs.webkit.org/attachment.cgi?id=23022&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
It seems there are missing files?  Can this little amount of code really
support EOT fonts?

This will not compile in release mode:
+    if(!m_isEOT || renderingMode != AlternateRenderingMode)
+	 ASSERT(m_cgFont);

ASSERT disappears in release mode.

Also, be sure to read:
http://webkit.org/coding/coding-style.html

There are lots of little style violations in this code.  A few which come to
mind:

// comment
( generally we have a space after // before the comment)

if ( instead of if(

We don't use else after a return statement:
+    if(!m_isEOT)
+	 return FontPlatformData(hfont, m_cgFont, size, bold, italic,
renderingMode == AlternateRenderingMode);
+    else
+	 return FontPlatformData(hfont, size, bold, italic, renderingMode ==
AlternateRenderingMode);   

{ goes on the next line after a function definition:

+void EOTStream::setInHeader(bool inHeader) {
+    m_inHeader = inHeader;
+}

Again, won't compile in Release mode (afaik):

+	     if(!m_useGDI)
+		 ASSERT(m_cgFont);


+bool isEOTFont(SharedBuffer* fontData) 

should have some minimum length check.	Otherwise it will crash if passed data
shorter than sizeof(EOTPrefix)

According to our style guidelines, we don't use c-style casts in C++, this
should be a reinterpret_cast<EOTPrefix*>:

+    EOTPrefix* prefix = (EOTPrefix*)(data);


No inner { } here:
+	     if (localGlyphBuffer[i] == invalid_glyph) {
+		 localGlyphBuffer[i] = 0;
+	     }

r- for all the style violations.  I've CC'd hyatt and hixie, who are the two
people who you would need to convince that EOT support is a good thing for
WebKit before we could accept such a patch.