Bug 15254 – [gtk] implement theme-aware drawing of text field or text area

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jan Alonzo]

Hi! Attached is a patch that makes the text field or text area conform to the current gtk theme.

[Jan Alonzo]

Created attachment 16344 [details]
implement theme-aware drawing of text fields and text area

[Adam Roben (:aroben)]

Comment on attachment 16344 [details]
implement theme-aware drawing of text fields and text area

+    else if (isChecked(o))
+        result = GTK_STATE_SELECTED;

Is this really needed for drawing text fields? Or is this just something that was missing and you added it while you were touching the code?

+    gtk_paint_shadow(entry->style, i.context->gdkDrawable(),
+                     determineState(o), determineShadow(o),
+                     NULL, entry, "entry",
+                     pos.x(), pos.y(), rect.width(), rect.height());

Since this is C++ code, please use 0 instead of NULL.

+    if (GTK_WIDGET_HAS_FOCUS(entry))
+    {

As per <http://webkit.org/coding/coding-style.html>, please pur the brace on the same line as the if.

r- so the above can be fixed. Once that's done I'll r+ it.

[Jan Alonzo]

Created attachment 16351 [details]
theme drawing of text entries 

This patch incorporates aroben's coding style suggestions. I left the isChecked in determineState but added a separate line in the ChangeLog for the change, not really required to fix this bug but nevertheless missing from the checks.

Thanks.

[Adam Roben (:aroben)]

Comment on attachment 16351 [details]
theme drawing of text entries 

r=me. Thanks for fixing it up!

[Holger Freyther]

(In reply to comment #4)
> (From update of attachment 16351 [details] [edit])
> r=me. Thanks for fixing it up!
> 

I'm not going to say r=- but I think
if (GTK_WIDGET_HAS_FOCUS(entry)) {} is a dead code path and something like if (isFocused(renderObject)) should be used.

[Jan Alonzo]

Created attachment 16435 [details]
gtk theme-based drawing of text entries

Update the patch based on zecke and bdash's comments

[Mark Rowe (bdash)]

Comment on attachment 16435 [details]
gtk theme-based drawing of text entries

Two comments:  You have tabs in your ChangeLog entry, and you don't need the "else"s when you have a series of if's with early returns.

[Mark Rowe (bdash)]

Comment on attachment 16435 [details]
gtk theme-based drawing of text entries

Those two issues can be fixed by whomever lands the change.  No need to jump through the hoops of uploading a new patch to get those minor points addressed.

[Holger Freyther]

Landed in r25807. The comments by Mark were taken into account.