[webkit-reviews] review denied: [Bug 10467] WebKit should have Qt platform support (Part II) : [Attachment 10122] Corrected patch

[bugzilla-request-daemon at opendarwin.org]

Eric Seidel <macdome at opendarwin.org> has denied Eric Seidel
<macdome at opendarwin.org>'s request for review:
Bug 10467: WebKit should have Qt platform support (Part II)
http://bugzilla.opendarwin.org/show_bug.cgi?id=10467

Attachment 10122: Corrected patch
http://bugzilla.opendarwin.org/attachment.cgi?id=10122&action=edit

------- Additional Comments from Eric Seidel <macdome at opendarwin.org>
FontCache probably needs your copyright as well:

--- qt/FontCacheQt.cpp	(Revision 0)
+++ qt/FontCacheQt.cpp	(Revision 0)
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2006 Dirk Mueller <mueller at kde.org>
+ *

likewise for PlatformTextEdit
and ImageSourceQt

normally webkit doesn't have {} around one line ifs
+    if (length < 4) {
+	 return 0;
+    }

Um, these really should be part of the individual decoders:

+    if (strncmp(contents, "GIF8", 4) == 0) {
+	 return new GIFImageDecoder();
+    }
+

GiffImageDecoder::isGif(char*)

Seems silly for all the platforms to have to write the same length if checks.

Even better would be an ImageDecoder class method which just new'd the right
one. (even though that would be a layering violation)

This seems to be indented "wrong":

+    : m_x(p.x())
+      , m_y(p.y())

the , and : should line up.

No breaks are need here:

+	 return QPainter::CompositionMode_Clear;
+	 break;

likewise:

+    case ButtCap:
+	 return Qt::FlatCap;
+	 break;

They just double the length of the source files for no good reason.

This should probably use the normal constructor initializers:

+    TransparencyLayer(const QPainter& p, int width, int height)
+    {
+	 pixmap = QPixmap(width, height);
+
+	 painter = new QPainter(&pixmap);

: pixmap(width, height), etc

Short one-liners can be written on one line if you choose:

+    bool isNull()
+    {
+	 return !x && !y && !blur;
+    }

it's your call there.

I'm not sure you always want antialiasing on:

+    // Good looking SVGs please...
+    painter->setRenderHint(QPainter::Antialiasing);

I feel like I remember webkit turning it on/off at times, but I'm not sure.

return should have it's own line:

+    if (m_data->shadow.isNull()) return;

I've seen this type of naming avoided in WebKit, probably because it can be
confused with obj-c private member variables:

+	 Pen _pen = pen();

Hum... I can't remember seeing alloca ever used anywhere else in WebKit:

+	 float *bmap = (float*)alloca(sizeof(float)*(md+1));

That also seems like a bad idea, as thickness is defined by the user and could
cause that to crash.

Generally we use spaces between operators:

n=-thickness
int d = n*n+m*m;
etc.

Personally I find multi-line blocks less confusing if they have an extra {}
here:

+	 for (int n=-thickness; n<=thickness; n++)
+	     for (int m=-thickness; m<=thickness; m++) {
+		 int d = n*n+m*m;
+		 if (d<=md)
+		     factor += bmap[d];
+	     }

even though for () for() doesn't really need the extra outside block for the
compiler's sake.

Looks like another crash possibility:
+	 float* amap = (float*)alloca(sizeof(float)*(h*w));
+	 memset(amap, 0, h*w*(sizeof(float)));

Hum, where is the corresponding save() to this end()?

+void GraphicsContext::endTransparencyLayer()

+    TransparencyLayer layer = m_data->layers.pop();
+    layer.painter->end();

There *has* to be a cleaner way (like using well named local variables?) to
simplify this:

+    path.addRect(QRectF(rect.x() + qMax(topLeft.width(), bottomLeft.width()),
+			 rect.y() + qMax(topLeft.height(), topRight.height()),
+			 rect.width() - qMax(topLeft.width(),
bottomLeft.width()) - qMax(topRight.width(), bottomRight.width()),
+			 rect.height() - qMax(topLeft.height(),
topRight.height()) - qMax(bottomLeft.height(),
+									       
	  bottomRight.height())));

More missing copyright:
qt/FontPlatformDataQt.cpp

With a const QString&() operator, I wouldn't think this would be necessary:

font.setFamily(toQString(familyName));

These should use the constructor form:

+FontPlatformData::FontPlatformData(const FontPlatformData& other)
+{
+    m_font = other.m_font;
+}

m_font(other.m_font)

more copyright:

qt/FontPlatformData.h

* in the "wrong" place:

+    static Cursors *self();
+    static Cursors *s_self;

spacing:
+    for(unsigned i = 0; i < bufferLength; i++) {

{ takes a separate line in function declarations:
+void setSharedTimerFiredFunction(void (*f)()) {

Hum, generally this either shouldn't be commited, or should be in WebKitTools:

+++ qt/test/testunity.cpp	(Revision 0)

Eventually this will all need to be wired into the KPart/khtml API:

+    Settings* settings = new Settings;
+    settings->setAutoLoadImages(true);
+    settings->setMinFontSize(5);


More copyright:
 qt/PathQt.cpp


Should use constructor initializers:

+void FontData::platformInit()
+{
+    QFontMetrics metrics(m_font.font());
+    m_ascent = metrics.ascent();
+    m_descent = metrics.descent();


Ok, I was only able to stomach about 75% if the patch ;)

My #1 suggestion: Break this up in to pieces.  Even if that means landing
"non-functional" pieces of Qt.	An example would be to get all of the
fundamental classes like FloatPoint, Size, etc. landed, then come back to more
complicated ones like FrameQt.

There are lots of style nits, not all of which need to be resolved to land, but
the more like the rest of webkit this code is, the easier it will be for
someone to read.

There are also some alloca uses which could result in crashes, which should be
fixed.