[webkit-reviews] review granted: [Bug 15737] [GTK] A MenuList should look like a GtkComboBox : [Attachment 16971] New RenderThemeGtk implementation (code shared with Mozilla)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Nov 2 21:46:53 PDT 2007


Adam Roben <aroben at apple.com> has granted Alp Toker <alp at atoker.com>'s request
for review:
Bug 15737: [GTK] A MenuList should look like a GtkComboBox
http://bugs.webkit.org/show_bug.cgi?id=15737

Attachment 16971: New RenderThemeGtk implementation (code shared with Mozilla)
http://bugs.webkit.org/attachment.cgi?id=16971&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
It looks like in many of the adjust*Style methods you are no longer allowing
web developers to style form controls at all, where the old code allowed for at
least limited styling. Are you sure this is the direction the GTK port should
go in? In the Mac/Windows ports there are heuristics to determine whether the
native look should be used or a more generic, styled look should be used for
each control, which gives a nice balance between native look-and-feel and
stylability. Maybe that's something you want to emulate?

+    // TODO: This strategy is possibly incorrect for the GTK+ port.

Would you mind changing all your TODOs to FIXMEs?

+static void setMozState(RenderTheme* theme, GtkWidgetState* state,
RenderObject* o)

It would be slightly nicer for the second paramater to be a GtkWidgetState&,
but it's not a big deal.
+    //state->active = theme->isChecked(o);

I think this commented-out code should be removed.

+    // It could be made a configuration option if 13 actually breaks site
compatibility.

I think you mean "...if sizes other than 13 actually break..."

+void RenderThemeGtk::adjustSearchFieldResultsButtonStyle(CSSStyleSelector*
selector, RenderStyle* style, Element* e) const
+{
+    adjustSearchFieldCancelButtonStyle(selector, style, e);
+}

This seems a little strange to me, but I suppose you're just saying that the
cancel and results buttons should have the same style? It's a little confusing,
though, since adjustSearchFieldResultsDecorationStyle doesn't call through to
adjustSearchFieldCancelButtonStyle.

+bool RenderThemeGtk::paintSearchFieldResultsDecoration(RenderObject* o, const
RenderObject::PaintInfo& i, const IntRect& rect)
+{
+    return paintSearchFieldCancelButton(o, i, rect);
+}

I'm very confused by this. On Mac/Windows, the results button and the results
decoration are both magnifying glasses. The difference is that the results
button has a little downward-pointing arrow that indicates it will show a menu
when you click on it. I don't think you want to show a cancel button where the
results decoration goes, because then you'd have cancel buttons on both sides
of the control, but one wouldn't do anything.

+static void gtkStyleSetCb(GtkWidget* widget, GtkStyle* previous, RenderTheme*
renderTheme)

Can you replace "Cb" with "Callback"?

r=me, though I do think you should consider the issue of stylability. You could
land the code as-is and file a bug about it.


More information about the webkit-reviews mailing list