1303310 - Use WidgetCache to get colors at nsLookAndFeel

Status: NEW

(Core :: Widget: Gtk, defect, P3)

Description

[Martin Stránský [:stransky] (ni? me)]

nsLookAndFeel should use styles from WidgetCache instead of creating its own widgets. It will help to implement multiple themes needed for Bug 1158076.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

We also need Bug 1282753 fixed to use comboboxes color here.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

That's a patch which depends on the Bug 1282753 (ComboBox code update). I checked the style classes that they are still present when get by WidgetCache. WidgetCache also provides some new widget types to support text colors.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8792843 [details] [diff] [review]
patch

Reusing WidgetStyleCache at least where there is an existing entry is good, thanks.

For the foreground colors on labels and the link button, the color is fetched
only once and so there may be no need to cache the widget, or even the style
context.  If and where the labels are never going to be drawn or measured, I'd
prefer the CreateStyleForWidget() approach, as used for the tooltip label.
However, tooltip labels are now measured for bug 1308936 (and so would be better off using WidgetStyleCache), so maybe other labels should be measured also, in which case using WidgetStyleCache make sense.

There are some unrelated whitespace changes, so please remove these.

MOZ_GTK_TEXTMENUITEM would be better named MOZ_GTK_MENUITEM_LABEL, for
consistency with the other names.

Is MOZ_GTK_COMBOBOX_LABEL the same as MOZ_GTK_COMBOBOX_ENTRY_TEXTAREA?

Please call GetWidget() only once on MOZ_GTK_ENTRY.

>     case eColorID__moz_buttondefault:

>+        {

Please place these opening parentheses after the case label, as in case 1 of
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
I expect that will help some editors with the indentation of the subsequent
lines.  Given the indentation of the case statements already differs from the
style guide, positioning of the closing brace is less clear.  I don't mind
whether you leave them as they are aligned with the previous statement or
align them with the case statement, as in the style guide.  (Don't realign the whole switch block to change the case statement indentation.)

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #0)
> It will help to implement multiple themes needed for Bug 1158076.

I don't think we need to support multiple themes in one process.
e10s means that each process is either chrome or content.

An adjustment to make attachment 8753289 [details] [diff] [review] work should be possible and would be much simpler.