444870 - slow path in cairo_draw_with_xlib taken when partially drawing scrollbar thumbs and checkboxes

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

The slow path in cairo_draw_with_xlib is taken when partially drawing
scrollbars and checkboxes (and probably radio buttons too).  This happens, for
example, when dragging another window across the top of a scrollbar thumb or
when scrolling so that part of a checkbox becomes visible.

It happens because the cairo_draw_with_xlib drawing rect is outside the
surface extents.  cairo_copy_clip_rectangle_list returns a clip rect bounded
by the size of the surface and so _get_rectangular_clip in cairo-xlib-utils.c
thinks that it needs to clip, but nsNativeThemeGTK::DrawWidgetBackground does
not use DRAW_SUPPORTS_CLIP_RECT.

If _get_rectangular_clip compared clip rectangles against (the intersection of
the drawing rect and) the surface extents it would realize that it wouldn't
need to clip (because it is not possible to draw outside the surface).

However, this situation is happening because of some strange logic in
DrawWidgetBackground.  Instead of applying the overflow margin to the frame
border rect and intersecting with the dirty rect to provide the drawing rect,
DrawWidgetBackground applies the overflow margin to the dirty rect, and thus
often tries to draw more than what needs drawing and sometimes even outside
the current surface.

If nsCSSRendering::PaintBackgroundWithSC passed
nsNativeThemeGTK::DrawWidgetBackground the real dirty rect instead of the
intersection of the dirty rect with the frame border rect then
DrawWidgetBackground could apply the overflow margin to the frame rect instead
of the dirty rect.

Comment 1

[Karl Tomlinson (:karlt)]

(In reply to comment #0)
> If _get_rectangular_clip compared clip rectangles against (the intersection of
> the drawing rect and) the surface extents it would realize that it wouldn't
> need to clip (because it is not possible to draw outside the surface).

Well, that clip rect at the drawable extents may actually serve a useful purpose for bug 445707 comment 1.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: include overflow in the dirty rect passed to DrawWidgetBackground()

The reftest relies on the loss of accuracy in alpha from the slow path.

Comment 3

[Karl Tomlinson (:karlt)]

nsNativeThemeWin::GetWidgetOverflow currently does nothing.  I didn't find
clear STR in bug 420381 to know whether that is now be fixed (with the
GetWidgetOverflow code reenabled).

nsNativeThemeCocoa::DrawWidgetBackground doesn't currently inflate the dirty
rect so I don't know how it draws the overflow.

Comment 4

[Karl Tomlinson (:karlt)]

This patch is causing a couple of these assertions on the tinderbox

###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /builds/slave/mozilla-central-linux-debug/build/widget/src/gtk2/gtk2drawing.c, line 1057

in widget/src/cocoa/crashtests/419737-1.html and widget/src/cocoa/crashtests/435223-1.html

I can reproduce the former but not the later.  The later doesn't even include any checkboxes or radio buttons so I don't know what's up there.

In the former case at least, assertion is right: GetMinimumWidgetSize is being
ignored, with and without the patch.  However, without the patch the size of
the widget is checked before adding overflow.  It is empty and the widget does
not get painted, hence no assertion.  With the patch the overflow is included
before the check, it is not empty and it paints.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: keep the old behavior of not painting widgets of empty size

It probably makes sense not to ask themes to paint widgets of zero size, 
so we can add a check for these.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: no need to increase the cliprect for scrollbar thumbs because the cliprect already includes the extra size

There are a couple of related things we can clean up while here:

The cliprect adjustment in moz_gtk_scrollbar_thumb_paint has already been applied to the drawing rect from GetExtraSizeForWidget.  I think it has probably been unnecessary since GetExtraSizeForWidget was added for Bug 327878:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/widget/src/gtk2&command=DIFF_FRAMESET&file=nsNativeThemeGTK.cpp&rev2=1.73&rev1=1.72

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: simplify/correct clip rects for tab widgets

With attachment 455374 [details] [diff] [review], if we just trust the dirtyRect provided to
DrawWidgetBackground() to be a reasonable drawing rect, we no longer need to
adjust the cliprect in moz_gtk_tab_paint, as this is already included in
GetWidgetOverflow().

Comment 8

[Markus Stange [:mstange]]

(In reply to comment #3)
> nsNativeThemeCocoa::DrawWidgetBackground doesn't currently inflate the dirty
> rect so I don't know how it draws the overflow.

Huh? If the overflow isn't dirty, why should it be drawn?

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
> Huh? If the overflow isn't dirty, why should it be drawn?

Without attachment 455374 [details] [diff] [review], nsCSSRendering::PaintBackgroundWithSC is not including the overflow in the dirty rect, even if it is dirty.  It is intersecting the dirty rect with the frame border rect (only).

Comment 10

[Markus Stange [:mstange]]

Oh, I see, that's bad. nsNativeThemeCocoa::DrawWidgetBackground seems to ignore the dirty rect completely (other than passing it to gfxQuartzNativeDrawing, which stores but never uses it), so that's probably why we weren't hit by that bug...

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 455640 [details] [diff] [review]
simplify/correct clip rects for tab widgets

(In reply to comment #7)
> With attachment 455374 [details] [diff] [review], if we just trust the dirtyRect provided to
> DrawWidgetBackground() to be a reasonable drawing rect, we no longer need to
> adjust the cliprect in moz_gtk_tab_paint, as this is already included in
> GetWidgetOverflow().

Actually, sorry, we can't use this appUnit dirtyRect and round out to pixels because opaque widget backgrounds are snapped to nearest pixels, not rounded out.
We can't tell the native renderer that a larger rectangle is opaque (because the extra pixel is not).

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: correct clip rects for tab widgets

This is a replacement for attachment 455640 [details] [diff] [review].
It ensures that the native renderer gets the correct drawing/clipping region,
and makes the calculations consistent in the various places that use this tab margin.

Comment 13

[Karl Tomlinson (:karlt)]

(The first changeset includes both attachment 455374 [details] [diff] [review] and attachment 455638 [details] [diff] [review].)
http://hg.mozilla.org/mozilla-central/rev/df20b88dfd24
http://hg.mozilla.org/mozilla-central/rev/5af02cada59b
http://hg.mozilla.org/mozilla-central/rev/13c6e0ecf8a7