399532 - nsDisplayBackground::GetBounds() should take nsITheme overflow into account

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Julien "_FrnchFrgg_" RIVAUD]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.8.1.6) Gecko/20070723 Iceweasel/2.0.0.6 (Debian-2.0.0.6-1)
Build Identifier: 

nsDisplayBackground doesn't override ::GetBounds(), and as such it returns the underlying frame rect. This is right, since CSS backgrounds cannot overflow the frame rect; *except* for themed frames (-moz-appearance) which advertize this via nsITheme::GetWidgetOverflow().

Reproducible: Always

Actual Results:  
nsDisplayBackground never takes the advertized overflow into account, even for themed frames.

Expected Results:  
nsDisplayBackground::GetBounds() should take the overflow rect into account if the underlying frame is themed.

Comment 1

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Make nsDisplayBackground::GetBounds() take nsITheme overflow into account

Comment 2

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

Hmm. Would you mind adding a PRPackedBool mIsThemed to nsDisplayBackground to record whether or not the frame is themed? IsThemed() isn't the cheapest function. Just set mIsThemed in the constructor. Thanks!

Comment 3

[Julien "_FrnchFrgg_" RIVAUD]

So in the constructor, I call mFrame->IsThemed() and cache that in mIsThemed ? I thought display lists were short-lived one-shot things ? Is GetBounds() called several times on each display item when optimizing/painting ?

Comment 4

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

(In reply to comment #3)
> So in the constructor, I call mFrame->IsThemed() and cache that in mIsThemed ?

Yep

> I thought display lists were short-lived one-shot things ?

Yes but we may still call GetBounds more than once. We also call IsOpaque and other methods that can use mIsThemed.

> Is GetBounds()
> called several times on each display item when optimizing/painting ?

Can be, yes.

Comment 5

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: [399532 Followup] Cache value of mFrame->IsThemed() in nsDisplayBackground.mIsThemed

To apply on top of first patch.

Comment 6

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Two first patches merged together

Comment 7

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Cache value of mFrame->IsThemed(), correct version

Comment 8

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Both patches merged, correct version

Sorry, a "configure" was wrongly pasted in before diffing. This is the correct version.

Comment 9

[Julien "_FrnchFrgg_" RIVAUD]

Comment on attachment 292943 [details] [diff] [review]
Both patches merged, correct version

This patch corrects wrong behaviour, and the possible slowdown due to an added test should be offseted by the new way of caching IsThemed(), so this patch could even accelerate things.
No real risk, the change is completely non-controversial.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 292943 [details] [diff] [review]
Both patches merged, correct version

a=beltzner for 1.9

Comment 11

[Reed Loden [:reed]]

Checking in layout/base/nsDisplayList.cpp;
/cvsroot/mozilla/layout/base/nsDisplayList.cpp,v  <--  nsDisplayList.cpp
new revision: 3.36; previous revision: 3.35
done
Checking in layout/base/nsDisplayList.h;
/cvsroot/mozilla/layout/base/nsDisplayList.h,v  <--  nsDisplayList.h
new revision: 3.25; previous revision: 3.24
done