[webkit-reviews] review granted: [Bug 19069] Renderers for wx : [Attachment 22554] Patch with Eric's comments addressed

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted 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 22554: Patch with Eric's comments addressed
https://bugs.webkit.org/attachment.cgi?id=22554&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
Looks OK.

Two comments:

1.  Whenever you use a numeric constant, it is always better to use a constant
variable.  Because the variable can have a nice long name to describe what the
numeric constant means.  In  the case of these 20, 2, 100, etc. values, it
would be nice to have either a comment or the use of a constant.  example:

int RenderThemeWx::minimumMenuListSize(RenderStyle*) const 
{ 
   // The minimum menu list size is document at:
  // http://wx.com/my_cool_docs/
    return 21; 
}

or:
	case MenulistAppearance: {
	    // The menu pop-down width is documented at:
	    // www.wx.com/my_even_cooler_docs
	     static const wxMenuPopDownWidth = 100;
	    r.setWidth(r.width() + wxMenuPopDownWidth);
	    break;
	}

My second comment is:
static_cast<wxDC*>(i.context->platformContext());

Why can't platformContext() just be a wxDC*?

In general looks fine.	I don't need to see the patch again.  You're a commiter
(IIRC), so feel free to make modifications and land.