[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 bugzilla-request-daemon at opendarwin.org
Wed Aug 23 13:24:43 PDT 2006

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)

Attachment 10154: Patch chunk: theme/cursor related stuff

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

+class Cursors {

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

+    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

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

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.

More information about the webkit-reviews mailing list