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)
Tracking
()
RESOLVED
FIXED
mozilla2.0b2
People
(Reporter: karlt, Assigned: karlt)
References
Details
(Keywords: perf)
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 2•14 years ago
|
||
The reftest relies on the loss of accuracy in alpha from the slow path.
Attachment #455374 -
Flags: review?(roc)
Assignee | ||
Comment 3•14 years ago
|
||
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.
Attachment #455374 -
Flags: review?(roc) → review+
Assignee | ||
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
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)
Assignee | ||
Comment 6•14 years ago
|
||
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)
Assignee | ||
Comment 7•14 years ago
|
||
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)
Comment 8•14 years ago
|
||
(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?
Assignee | ||
Comment 9•14 years ago
|
||
(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•14 years ago
|
||
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...
Attachment #455638 -
Flags: review?(roc) → review+
Attachment #455640 -
Flags: review?(roc) → review+
Attachment #455639 -
Flags: review?(roc) → review+
Assignee | ||
Comment 11•14 years ago
|
||
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
Assignee | ||
Comment 12•14 years ago
|
||
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)
Attachment #457407 -
Flags: review?(roc) → review+
Assignee | ||
Comment 13•14 years ago
|
||
(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
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.
Description
•