[webkit-reviews] review denied: [Bug 10467] WebKit should have Qt platform support (Part II) : [Attachment 10154] Patch chunk: theme/cursor related stuff

[bugzilla-request-daemon at opendarwin.org]

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

Attachment 10154: Patch chunk: theme/cursor related stuff
http://bugzilla.opendarwin.org/attachment.cgi?id=10154&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
Comments:

+class Cursors {

Classes should be declared in .h files, not .cpp files

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

Stars should be next to the type, not the identifier (Cursors* self())

+    if (!s_self) {
+	 s_self = new Cursors;
+    }

Shouldn't use braces here

+
+// vim: ts=4 sw=4 et

We don't have modelines in our files

+#include <QStyle>
+#include <QWidget>
+#include <QPainter>
+#include <QStyleOptionButton>
+
+#include "config.h"
+
+#include "RenderTheme.h"
+#include "GraphicsContext.h"

Is there a reason config.h is not the first #include? The convention for
#includes is to first have config.h, then the .h file specific to this
implementation, then any other includes in alphabetical order.

+class RenderThemeQt : public RenderTheme

Again, classes should be declared in .h files (in this case, RenderThemeQt.h)

+    bool stylePainterAndWidgetForPaintInfo(const RenderObject::PaintInfo&,
QStyle*&, QPainter*&, QWidget*&) const;

The name of this method is a bit unclear. Perhaps
"getStylePainterAndWidgetFromPaintInfo"?

+	 option.state |= QStyle::State_ReadOnly; // Readonly is supported on
textfields.

Put this comment above the line instead of next to it.

+    // TODO: this is not enough for sure! (use 'option'...)

Might want to use a FIXME instead of TODO, since that's the convention used
through most of WebKit

+// vim: ts=4 sw=4 et

Again, no modelines, please.


   Overall, this looks pretty solid. As you can see, the comments above are
mostly style-related, so it should be pretty easy to clean it up. The biggest
thing is to move those class declarations into .h files.