174585 - Need to work around gdk_flush() call in nsNativeThemeGTK.cpp

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This gdk_flush() call really hurts on remote connections. We need to find a way
to avoid it "most of the time", perhaps by caching the "known good" status of a
theme on a per-(widget+state) basis.

Comment 1

[Brian Ryner (not reading)]

Attachment: cache known-good states

This patch caches widget states that are known to draw without error.  I
thought about this quite a bit and decided that this is not redundant at all
with the current widget disabled bitfield.  The reason is that there are 3
possible states you might have -- the widget state has never been drawn, the
widget state is known to be safe, or the entire widget is disabled.  The widget
being disabled is not the same as not knowing the state to be safe.  So, I
think this is actually the minimum amount of space we need to store this
information -- one bit per widget+state, and an additional bit per widget.

Also, I decided not to include canDefault as a factor in the state cache since
it's never given to gtk directly.  I also decided not to use the scrollbar
curPos and maxPos simply because I can't imagine those states individually
triggering a problem.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 136087 [details] [diff] [review]
cache known-good states

looks OK. This is a bit of a hack; just because painting a widget in one state
doesn't cause an error the first time, doesn't mean it won't the next time. On
the other hand this whole thing is a hack so if this heuristic works on real
themes, and it should, then let's do it.

Comment 3

[Brian Ryner (not reading)]

checked in.

Comment 4

[Aleksey Nogin]

This have added a warning on brad TBox:

+gfx/src/gtk/nsNativeThemeGTK.cpp:430
+ `int (*oldHandler)(Display*, XErrorEvent*)' might be used uninitialized in
this function

Comment 5

[Brian Ryner (not reading)]

The warning is bogus, I'd say. |oldHandler| is only set or used if |safeState|
is false.  It's not possible for |safeState| to become true between the first
and second checks.

Comment 6

[Brian Ryner (not reading)]

or rather, to become false.

Comment 7

[Serge Gautherie (:sgautherie)]

Re comment 5:

The code is right, but the compiler does not track the |if| conditions :-(
Would you care for a patch like this anyway ?
{
-  XErrorHandler oldHandler;
+  /* Useless initialization, except to avoid a compiler warning. */
+  XErrorHandler oldHandler = nsnull;
}

Comment 8

[Serge Gautherie (:sgautherie)]

Re comment 7:

Warning fixed:
Check in: { 01/07/2004 12:08	bryner%brianryner.com 	mozilla/ gfx/ src/ gtk/
nsNativeThemeGTK.cpp 	1.45 }