Bug 19069 – Renderers for wx

• Component: WebKit wx
• Status: RESOLVED FIXED

[Robin Dunn]

This patch adds some renderers for wxWebKit

[Robin Dunn]

Created attachment 21153 [details]
Renderers for wx

[David Kilzer (:ddkilzer)]

Comment on attachment 21153 [details]
Renderers for wx

>+#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!

[Kevin Ollivier]

Created attachment 22165 [details]
Updated version of Robin's patch, addressing David's comments

Regarding the proper include for WebView.h, I'm leaving it as-is as we haven't decided how we will handle that, but it should be noted that wxWebKit will probably not be made an official part of the wxWidgets project due to the size of WebKit and the fact that most wxWidgets users build from source, meaning that checkouts and builds will take significantly longer. Given that, <wx/WebView.h> may give people the wrong impression that WebView is part of wx proper.

[Eric Seidel (no email)]

Comment on attachment 22165 [details]
Updated version of Robin's patch, addressing David's comments

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.

[Kevin Ollivier]

Created attachment 22554 [details]
Patch with Eric's comments addressed

Patch with Eric's comments addressed. Regarding the wxDC issue, the issue is that the actual class used for the DC is different depending on whether you enable wxGraphicsContext support (i.e. vector-based drawing) or not. The goal is to make vector-based drawing "just work" on all platforms we support, but currently there are issues with that. For example, on Windows, the lack of GDI+ hardware acceleration really hurts performance there. (We're discussing using the Cairo implementation of wxGraphicsContext there, but it requires some work to get going.) Also, some embedded platforms may not support wxGraphicsContext, so we're leaving both implementations in the tree for the foreseeable future, as wxWebKit does work reasonably well with even the old drawing context. 

Since both implementations use classes derived from wxDC, but have different APIs, we decided to just cast to the base class when the base class APIs can be used for both implementations.

As for the hardcoded numbers, wx doesn't yet have a way to get those numbers internally. What we'll try to do is to "get it right" here in wxWebKit first over time, then move the implementations back into wx once we've got the bugs worked out. 

Anyway, hopefully this explains things. :-) Lastly, rather than add to manual_tests, I'll probably put time into getting DumpRenderTree going when I can. In the meantime, I've removed the link.

[Eric Seidel (no email)]

Comment on attachment 22554 [details]
Patch with Eric's comments addressed

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.

[Kevin Ollivier]

Did my previous comments not answer your questions? Anyway, the current constants are temporary - wx has no method right now to provide the right constants, and we'll have to do more testing with wxWebKit to determine how to best get those values. Once we do, we'll fix the problem properly in wx and move this code to use that. The wxDC* question is a bit complicated and I'm not sure how to give an easier answer than the one I gave last time. :(

Landed in r35518.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Did my previous comments not answer your questions? Anyway, the current
> constants are temporary - wx has no method right now to provide the right
> constants, and we'll have to do more testing with wxWebKit to determine how to
> best get those values. Once we do, we'll fix the problem properly in wx and
> move this code to use that.

What Eric and I meant was that it would be a good idea to define the constants locally in the .cpp file, even if means replacing them later.

[Kevin Ollivier]

(In reply to comment #8)
> (In reply to comment #7)
> > Did my previous comments not answer your questions? Anyway, the current
> > constants are temporary - wx has no method right now to provide the right
> > constants, and we'll have to do more testing with wxWebKit to determine how to
> > best get those values. Once we do, we'll fix the problem properly in wx and
> > move this code to use that.
> 
> What Eric and I meant was that it would be a good idea to define the constants
> locally in the .cpp file, even if means replacing them later.
> 

Yes, I know it's good policy in general to use defines for constants. In this case, though, RenderTheme has functions whose sole purpose is to return the constant, and any code that needs the constant will call that function, so the usual benefits of using a constant (replace the value in multiple contexts by changing the defined value) don't apply here. 

i.e. the following code seems rather redundant:

int RenderThemeWx::popupInternalPaddingTop(RenderStyle*) const 
{
    return POPUP_INTERNAL_PADDING_TOP;
}

Don't defining popupInternalPaddingTop and POPUP_INTERNAL_PADDING_TOP both serve to solve the same problem (a single, centralized place to get the constant value)?

Anyway, I have no problems changing the code to use constants for all the numbers for now, as it should only take 10 mins or so, and I'll go ahead and commit an update hopefully today.