[webkit-reviews] review denied: [Bug 19069] Renderers for wx : [Attachment 21153] Renderers for wx

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Robin Dunn
<robin at alldunn.com>'s request for review:
Bug 19069: Renderers for wx
http://bugs.webkit.org/show_bug.cgi?id=19069

Attachment 21153: Renderers for wx
http://bugs.webkit.org/attachment.cgi?id=21153&action=edit

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
>+#include "WebKit/wx/WebView.h"
>+
> #include <wx/defs.h>
> #include <wx/renderer.h>
> #include <wx/dcclient.h>

Should this be #include <wx/WebView.h>?

>+void RenderThemeWx::adjustRepaintRect(const RenderObject* o, IntRect& r)
>+{
>+    switch (o->style()->appearance()) {
>+	  case MenulistAppearance: {
>+	      r.setWidth(r.width() + 100); // = inflateRect(r,
popupButtonSizes()[[popupButton() controlSize]], popupButtonMargins());
>+	      //fprintf(stderr, "caling adjustRepaintRect, width is %d...\n",
r.width());
>+	      break;
>+	  }
>+	  default:
>+	      break;
>+    }
>+}

Please remove commented-out code (especially debugging statements :).

>     if (!isEnabled(o))
>	  flags |= wxCONTROL_DISABLED;
>-
>+	  
>     EAppearance appearance = o->style()->appearance();
>     if (supportsFocus(o->style()->appearance()) && isFocused(o))
>	  flags |= wxCONTROL_FOCUSED;
>@@ -204,16 +233,18 @@
>     if (isPressed(o))
>	  flags |= wxCONTROL_PRESSED;
>     
>-    if (appearance == PushButtonAppearance || appearance == ButtonAppearance)

>+    if (appearance == PushButtonAppearance || appearance == ButtonAppearance)

>	  wxRendererNative::Get().DrawPushButton(window, *dc, r, flags);

Gratuitous whitespace changes.

>+int RenderThemeWx::popupInternalPaddingLeft(RenderStyle*) const 
>+{ 
>+    return 6; 
> }
> 
>+int RenderThemeWx::popupInternalPaddingRight(RenderStyle*) const 
>+{
>+#ifdef __WXMAC__
>+    return 22;
>+#else
>+    return 20;
>+#endif
>+}
>+
>+int RenderThemeWx::popupInternalPaddingTop(RenderStyle*) const 
>+{
>+    return 2;
>+}
>+
>+int RenderThemeWx::popupInternalPaddingBottom(RenderStyle*) const
>+{ 
>+    return 2; 
>+}

Might be nice to use const int variables (to match RenderThemeMac.mm and
RenderThemeSafari.cpp), but not necessary.

Needs a ChangeLog entry (as do your other patches :).  See
<http://webkit.org/coding/contributing.html> for details.

r- for commented-out code, #include formatting and no ChangeLog entry, but
otherwise the patch looks good!  Thanks, Robin!