[webkit-reviews] review denied: [Bug 19069] Renderers for wx : [Attachment 22165] Updated version of Robin's patch, addressing David's comments

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 24 11:56:39 PDT 2008

Eric Seidel <eric at webkit.org> has denied Kevin Ollivier
<kevino at theolliviers.com>'s request for review:
Bug 19069: Renderers for wx

Attachment 22165: Updated version of Robin's patch, addressing David's comments

------- Additional Comments from Eric Seidel <eric at webkit.org>
This doesn't really belong in the file:
+// A good URL for testing form element rendering:

instead you should land a test case as part of WebCore/manual_tests or use one
of the existing test cases.

WebCore style omits argument names when they don't add to the understanding of
the method signature:
+    virtual void adjustRepaintRect(const RenderObject* o, IntRect& r);
Both of those should be omited.

This should be in the .cpp file (along with the rest of your changes) and
probably with some sort of explanation as to why it's 21... or maybe you should
look up that constant somewhere from a wx call.  21 seems rather arbitrary, and
it's sad to have constants typed manually, since then if wx ever were to
change, this code would be wrong.
+    virtual int minimumMenuListSize(RenderStyle*) const { return 21; }

Why is the local variable needed here?
+    IntRect rect = r;
+    wxRenderer_DrawChoice(window, *dc, rect, flags);

Shouldn't wx's NativeContentPtr (platformContext() return value) just be a

Otherwise this looks fine.

More information about the webkit-reviews mailing list