Bug 40024 – [GTK] The implementation of RenderThemeGtk::systemColor is incomplete

• Status: UNCONFIRMED
• Component: WebKitGTK

[Olivier Tilloy]

A minimal implementation of RenderThemeGtk::systemColor was merged in order to fix bug #37779 (see http://trac.webkit.org/changeset/60440).

This implementation should be completed to match all the relevant CSSValue* constants with their Gtk style counterpart.

Note that not all those constants map to an actual colour as defined by Gtk styles, it looks to me like the specification (http://www.w3.org/TR/CSS2/ui.html#system-colors) is largely inspired by the windows API (GetSysColor, see http://msdn.microsoft.com/en-us/library/ms724371%28VS.85%29.aspx). Therefore constants such as CSSValueAppworkspace or CSSValueBackground won't be mapped.

[Olivier Tilloy]

Created attachment 57771 [details]
Complete implementation of RenderThemeGtk::systemColor - first draft

I'm attaching a patch with a complete implementation of RenderThemeGtk::systemColor, largely inspired by Mozilla's implementation in nsLookAndFeel::NativeGetColor. This is a first draft that may need refining.

[Martin Robinson]

(In reply to comment #1)
> Created an attachment (id=57771) [details]
> Complete implementation of RenderThemeGtk::systemColor - first draft
> 
> I'm attaching a patch with a complete implementation of RenderThemeGtk::systemColor, largely inspired by Mozilla's implementation in nsLookAndFeel::NativeGetColor. This is a first draft that may need refining.

Any updates on this?

[Martin Robinson]

*** Bug 15597 has been marked as a duplicate of this bug. ***

[Olivier Tilloy]

@Martin: I haven’t looked at this issue in a while. From what I remember last time I worked on it, the patch I submitted was pretty much complete and functional, and it was ready for a review (sorry if I didn’t make it clear). Not sure it still merges

My priorities have shifted away from webkit lately, but I’m willing to help get this patch reviewed and tweaked as needed. Does the patch need to be flagged to request a review?

[Carlos Garcia Campos]

Created attachment 205794 [details]
Patch for GTK3

[Build Bot]

Comment on attachment 205794 [details]
Patch for GTK3

Attachment 205794 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1012465

New failing tests:
fast/forms/select/popup-closes-on-blur.html

[Build Bot]

Created attachment 205856 [details]
Archive of layout-test-results from APPLE-EWS-4 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-4  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Sergio Villar Senin]

Comment on attachment 205794 [details]
Patch for GTK3

View in context: https://bugs.webkit.org/attachment.cgi?id=205794&action=review

Looks good. The original file does not exist anymore but I guess migrating the changes is straightforward.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:1026
> +        return 0xFF888888;

I prefer to have this magic number statically defined with some meaningful name.

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:1065
> +        gtk_style_context_restore(context);

Let's move this to a helper as is repeated bellow. Question: why do we need to temporarily add that class just to get the background color?

> Source/WebCore/platform/gtk/RenderThemeGtk3.cpp:1091
> +        gtk_style_context_restore(context);

Perhaps we could use the helper above here as well, we could pass the GTK_TYPE_ and GTK_STYLE_ info as arguments.