nsRenderingContextGTK::PopState calls nsRegionGTK::IsEmpty too much

RESOLVED FIXED

Status

RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: bzbarsky, Assigned: tor)

Tracking

({perf})

Trunk
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
PopState() takes a boolean arg that almost all callers ignore.  Setting this arg
requires a call to nsRegionGTK::IsEmpty, typically.  This is about 10% of the
time spent in PopState.

I'd think that this arg should become an (optional) PRBool* arg; then callers
can pass null if they don't care about the clip.
(Reporter)

Updated

15 years ago
Blocks: 240275
(Assignee)

Comment 1

15 years ago
SetClipRect() also has an oft-ignored boolean arg that causes IsEmpty()
to be called.
(Assignee)

Comment 2

15 years ago
I went through the tree and couldn't find any callers that used the
arg returned from PopState() - I'll attach the patch tomorrow.
(Assignee)

Comment 3

15 years ago
Created attachment 146194 [details] [diff] [review]
remove PopState() argument
(Assignee)

Updated

15 years ago
Attachment #146194 - Flags: review?(blizzard)
Attachment #146194 - Flags: review?(blizzard) → review+
(Assignee)

Updated

15 years ago
Attachment #146194 - Flags: superreview?(bryner)
(Assignee)

Comment 4

15 years ago
Created attachment 146350 [details] [diff] [review]
resync PopState() patch to tree
Attachment #146194 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 5

15 years ago
PopState() patch checked in.
Assignee: blizzard → tor
(Assignee)

Comment 6

15 years ago
Created attachment 146711 [details] [diff] [review]
remove argument of SetClip{Rect,Region}
(Assignee)

Updated

15 years ago
Attachment #146711 - Flags: superreview?(bryner)
Attachment #146711 - Flags: review?(blizzard)
(Assignee)

Comment 7

15 years ago
Comment on attachment 146711 [details] [diff] [review]
remove argument of SetClip{Rect,Region}
Attachment #146711 - Attachment is obsolete: true
Attachment #146711 - Flags: superreview?(bryner)
Attachment #146711 - Flags: review?(blizzard)
(Assignee)

Comment 8

15 years ago
Created attachment 146712 [details] [diff] [review]
remove unrelated nsGCCache changes from previous patch
(Assignee)

Updated

15 years ago
Attachment #146712 - Flags: superreview?(bryner)
Attachment #146712 - Flags: review?(blizzard)
Attachment #146712 - Flags: superreview?(bryner) → superreview+
Attachment #146712 - Flags: review?(blizzard) → review+
(Assignee)

Comment 9

15 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 10

15 years ago
Created attachment 146875 [details]
png showing regression

After this checkin: When i mouse over items on personal toobar, the black line
under them vanishes. When i scroll windows, black lines draw over images, and
white lines over the fonts. Not so good.

Testing with a current trunk cvs build, Linux.
I backed out the 40 or so files this checkin touched, and the bug vanished.
I rolled the changes back in again, and the bug re-surfaced.

Building with gcc version 3.3.3 and 
--enable-default-toolkit=gtk2 --disable-accessibility --disable-postscript
--disable-ldap --disable-debug --disable-tests --disable-logging
--enable-crypto --disable-bidi --enable-optimize=-O2 --disable-jsd
--disable-mathml
(Assignee)

Comment 11

15 years ago
Oops, accidentally checked in a one-line perf test change that causes
these regressions.  I've backed that out of the tree.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.