[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]

Eric Seidel <eric at webkit.org> has denied Kevin Ollivier
<kevino at theolliviers.com>'s request for review:
Bug 19069: Renderers for wx
https://bugs.webkit.org/show_bug.cgi?id=19069

Attachment 22165: Updated version of Robin's patch, addressing David's comments
https://bugs.webkit.org/attachment.cgi?id=22165&action=edit

------- 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:
http://www.tizag.com/htmlT/forms.php
+

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
wxDC*?

Otherwise this looks fine.