[webkit-reviews] review denied: [Bug 11617] Compile and work on Qt/KDE platforms : [Attachment 11539] fixes the style issues

bugzilla-request-daemon at macosforge.org bugzilla-request-daemon at macosforge.org
Thu Nov 16 14:14:48 PST 2006


mitz at webkit.org has denied mitz at webkit.org's request for review:
Bug 11617: Compile and work on Qt/KDE platforms
http://bugs.webkit.org/show_bug.cgi?id=11617

Attachment 11539: fixes the style issues
http://bugs.webkit.org/attachment.cgi?id=11539&action=edit

------- Additional Comments from mitz at webkit.org
r- since there are still a few coding style and comment issues, but other than
that the patch looks good.

Please avoid trailing spaces:

-   ${CMAKE_CURRENT_SOURCE_DIR}/page
+   ${CMAKE_CURRENT_SOURCE_DIR}/page   

Don't use tabs in ChangeLog:

+	http://bugs.webkit.org/show_bug.cgi?id=11617

+	Adjusting to the new api.

I don't think this comment makes sense for Qt:

+    // Double-click events don't exist in Cocoa. Since
passWidgetMouseDownEventToWidget will
+    // just pass currentEvent down to the widget, we don't want to call it for
events that
+    // don't correspond to Cocoa events.  The mousedown/ups will have already
been passed on as
+    // part of the pressed/released handling.

What's the point of this comment?

+    // FIXME: this method always returns true
+    notImplemented();
+    return false;
+}

This should be '#include "Shared.h"'

+#include <Shared.h>

Should use braces for one-liners:

+    if (!m_menu) {
+	 m_menu = new QMenu();
+    }

FIXME style is "// FIXME: Sentence-capitalized comment". I think these comments
could be phrased better (the second one might not make sense once the first one
is changed or removed):

+    //FIXME this method is silly
+    //FIXME another silly method

Include <WTF/Forward.h> and drop the WTF:: from these:

+    virtual void
registerCommandForUndo(WTF::PassRefPtr<WebCore::EditCommand>);
+    virtual void
registerCommandForRedo(WTF::PassRefPtr<WebCore::EditCommand>);



More information about the webkit-reviews mailing list