Closed Bug 444870 Opened 16 years ago Closed 14 years ago

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

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b2

People

(Reporter: karlt, Assigned: karlt)

References

Details

(Keywords: perf)

Attachments

(4 files, 1 obsolete file)

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.
(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.
Blocks: 537860, 402940
Component: Graphics → Widget: Gtk
Keywords: perf
QA Contact: thebes → gtk
The reftest relies on the loss of accuracy in alpha from the slow path.
Attachment #455374 - Flags: review?(roc)
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.
Blocks: 563887
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.
It probably makes sense not to ask themes to paint widgets of zero size, so we can add a check for these.
Attachment #455638 - Flags: review?(roc)
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
Attachment #455639 - Flags: review?(roc)
Attached patch simplify/correct clip rects for tab widgets (obsolete) (deleted) — Splinter Review
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().
Attachment #455640 - Flags: review?(roc)
(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?
Blocks: 546178
(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).
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 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).
Attachment #455640 - Attachment is obsolete: true
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.
Attachment #457407 - Flags: review?(roc)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: