574621 - Need to use prescontext's DefaultBackgroundColor in toplevel content documents

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Attachment: fix

With bug 130078 fixed, we need to ensure that toplevel content documents still get the prescontext's DefaultBackgroundColor in their background if the document is transparent/translucent. This patch does that.

Comment 1

[Timothy Nikkel (:tnikkel)]

Comment on attachment 453998 [details] [diff] [review]
fix

rootFrame here isn't the root frame, it is the root element style frame, so I think it will always have a parent. Feel free to rename the variable so this problem is removed.

I recall a bug that I can't locate now that you fixed that was a regression from removing child widgets (?) with opening of pages in a sidebar via opening a bookmark that has "load this bookmark in the sidebar" checked where there was an unusual setup with chrome/content in the docshell hierarchy. Does IsChildOfSameType do the right thing in that case?

Looking at this code I wonder if it's ever possible that we wouldn't update mCanvasBackgroundColor because neither 'if' is entered. Do you think we could assert that one of the 'if' statement executes?

Comment 2

[Timothy Nikkel (:tnikkel)]

Also, thanks for doing this, it removes an item from my bug 130078 todo list.

Comment 3

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

Comment on attachment 453998 [details] [diff] [review]
fix

OK, so the patch is wrong ... rootFrame needs to be renamed to rootStyleFrame, and we should probably have proper API for checking whether a prescontext is for a toplevel content document.

Comment 4

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

IIRC that bug I fixed was a chrome subdocument of a chrome document, which should not get a background here IMHO.

I'll update this later.

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Updated patch (seems to work)

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 457777 [details] [diff] [review]
Updated patch (seems to work)

Stealing review since roc should probably not review this since he basically wrote the patch.

r=me if the local variable rootFrame is renamed to rootStyleFrame.

Comment 7

[Timothy Nikkel (:tnikkel)]

Landed with review comments addressed
http://hg.mozilla.org/mozilla-central/rev/9e290d196600

but backed out due to failure in layout/base/test/chrome/test_chrome_content_integration.xul
http://hg.mozilla.org/mozilla-central/rev/a24232050cb1
http://hg.mozilla.org/mozilla-central/rev/c402a67ba0db

Comment 8

[Timothy Nikkel (:tnikkel)]

It looks like this also caused a crash in talos tp4 on linux64 loading www.jugem.jp/jugem.jp/index.html
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280434220.1280434694.24886.gz

Comment 9

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

We didn't get a stack there. I wonder if that's the same crash that you and karl have been chasing?

Comment 10

[Timothy Nikkel (:tnikkel)]

Yes it is the same crash, so it's just the one test fail left.

Comment 11

[Timothy Nikkel (:tnikkel)]

Or a very similar crash fixed by the same patch, at least.

Comment 12

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

OK, so the reason you don't see much of a problem without this patch is that tabbrowser.css styles <tabpanels> with background-color:white. That's what you're seeing as the background for Web content. If you try changing the Web page background color preference to something other than white, you'll see that it doesn't take effect.

I'll write a chrometest for this. I've also fixed up the mochitest failure, so I'll resubmit the patch in a jiffy.

BTW, drawing all Web content to a transparent surface can be bad for performance as well as subpixel AA. So we should definitely get new Talos runs once this is fixed!

Comment 13

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

Attachment: fix v2

Fixes the failure in test_chrome_content_integration.xul; we need to honor the "transparent" attribute on our container element.

Added a test to ensure that the background color pref is honoured. We fail the test with bug 130078 without this patch.

This fixes bug 588407.

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 469003 [details] [diff] [review]
fix v2

Can you incorporate the rename of rootFrame -> rootStyleFrame in this patch so there is less potential confusion?

Adding a call to IsTransparentContainerElement in UpdateCanvasBackground adds a fair bit of "stuff" to this little function that gets called every time we paint. Assuming we want to support dynamic changes of the transparent attribute, is it worth it to cache this value and set up some sort of notification when the transparent attribute gets added/removed?

>+      dump(comparison[1] + "\n");
>+      dump(comparison[2] + "\n");

Did you intend to leave these dump's in?

Thanks.

Comment 15

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

(In reply to comment #14)
> Can you incorporate the rename of rootFrame -> rootStyleFrame in this patch so
> there is less potential confusion?

Sure!

> Adding a call to IsTransparentContainerElement in UpdateCanvasBackground adds a
> fair bit of "stuff" to this little function that gets called every time we
> paint. Assuming we want to support dynamic changes of the transparent
> attribute, is it worth it to cache this value and set up some sort of
> notification when the transparent attribute gets added/removed?

Painting is a heavyweight operation and should be happening at much less than 1000Hz. IsTransparentContainerElement is noise at that rate.

> >+      dump(comparison[1] + "\n");
> >+      dump(comparison[2] + "\n");
> 
> Did you intend to leave these dump's in?

No, I'll take them out. Thanks.

Comment 16

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

(In reply to comment #15)
> Painting is a heavyweight operation and should be happening at much less than
> 1000Hz. IsTransparentContainerElement is noise at that rate.

(Long term, it should be happening at no more than ~60Hz per browser window, but we won't quite get there for FF4.)

Comment 17

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/be60f89d06ba