On OS X, certain native widgets must be transparent and this is indicated through the theme code. On win32, the theme can sometimes tell us when theme parts are partially transparent or opaque. If it can't (and for the other theme backends), we indicate that we don't know. The two callers (nsDisplayList::IsOpaque and nsLayoutUtils::GetFrameTransparency) will error on their appropriate conservative side in the event we don't know the transparency.

Comment on attachment 435093 [details] [diff] [review] Add nsITheme::Transparency and modify nsITheme::GetWidgetTransparency + switch (mThemeTransparency) { + case nsITheme::eOpaque: + return PR_TRUE; + case nsITheme::eTransparent: + case nsITheme::eUnknownTransparency: + default: + return PR_FALSE; + } return mThemeTransparency == nsITheme::eOpaque; + switch (transparency) { + case nsITheme::eOpaque: + case nsITheme::eUnknownTransparency: + default: + return eTransparencyOpaque; + case nsITheme::eTransparent: + return eTransparencyTransparent; + } return mThemeTransparency == eTransparent ? eTransparencyTransparent : eTransparencyOpaque; + // The classic theme probably does transparency by omission but we don't + // know for sure This is a bit confusing. I'd just say "for the classic theme, we have no way of knowing". (Although I suppose we could hardcode knowledge if we really wanted to.) I'd like you to confirm that when we call nsUXThemeData::isThemeBackgroundPartiallyTransparent for a XUL window's default background, it says false. Otherwise we'll be turning on transparent windows for everything...

(In reply to comment #0) > On OS X, certain native widgets must be transparent and this is indicated > through the theme code. But the others used to be Opaque, and now you're returning Unknown -- is that intentional? What effect does that have on OSX windows?

(In reply to comment #2) > (In reply to comment #0) > > On OS X, certain native widgets must be transparent and this is indicated > > through the theme code. > > But the others used to be Opaque, and now you're returning Unknown -- is that > intentional? What effect does that have on OSX windows? It should have no change in behavior. The previously existing caller, nsLayoutUtils::GetFrameTransparency interprets Unknown as Opaque. The new caller, nsDisplayBackground::IsOpaque, interprets Unknown as not opaque. Previously it just assumed all theme parts were not opaque.

Comment on attachment 435093 [details] [diff] [review] Add nsITheme::Transparency and modify nsITheme::GetWidgetTransparency That seems rather strange to me, but ok!

(In reply to comment #1) > I'd like you to confirm that when we call > nsUXThemeData::isThemeBackgroundPartiallyTransparent for a XUL window's default > background, it says false. Otherwise we'll be turning on transparent windows > for everything... As far as I could tell, it is never called. The windows theme doesn't support the NS_THEME_WINDOW or NS_THEME_DIALOG widget types (which would explain the lack of calls) and we would return eUnknownTransparency anyways.

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/f3569af989e8

This checkin added a bunch of GCC compile-warning-spew -- multiple lines for every cpp file that directly or indirectly #includes nsITheme.h. E.g.: { nsDOMClassInfo.cpp In file included from ../../dist/include/nsIWidget.h:52, from ../../dist/include/nsPluginNativeWindow.h:49, from ../../dist/include/nsIPluginHost.h:42, from ../../../mozilla/dom/base/nsPluginArray.h:44, from ../../../mozilla/dom/base/nsGlobalWindow.h:89, from ../../../mozilla/dom/base/nsLocation.cpp:42: ../../dist/include/nsITheme.h:146: warning: ‘typedef’ was ignored in this declaration } The issue is here: 142 typedef enum Transparency { 143 eOpaque = 0, 144 eTransparent, 145 eUnknownTransparency 146 }; The 'typedef' there is incorrect. robarnold, mind removing that?

(In reply to comment #7) > This checkin added a bunch of GCC compile-warning-spew To be precise, there are 585 distinct instances instances of this warning in the last linux build log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1269905502.1269906778.28420.gz

(In reply to comment #7) > This checkin added a bunch of GCC compile-warning-spew -- multiple lines for > every cpp file that directly or indirectly #includes nsITheme.h. > > E.g.: > { > nsDOMClassInfo.cpp > In file included from ../../dist/include/nsIWidget.h:52, > from ../../dist/include/nsPluginNativeWindow.h:49, > from ../../dist/include/nsIPluginHost.h:42, > from ../../../mozilla/dom/base/nsPluginArray.h:44, > from ../../../mozilla/dom/base/nsGlobalWindow.h:89, > from ../../../mozilla/dom/base/nsLocation.cpp:42: > ../../dist/include/nsITheme.h:146: warning: ‘typedef’ was ignored in this > declaration > } > > The issue is here: > 142 typedef enum Transparency { > 143 eOpaque = 0, > 144 eTransparent, > 145 eUnknownTransparency > 146 }; > > The 'typedef' there is incorrect. robarnold, mind removing that? I could have sworn it was necessary at some point but msvc seems happy without it. I've also been doing a lot of C recently.

If you'd done |typedef enum { ... } Transparency;| it would have been OK. BTW, I just fixed bug 550401 as an olive branch ;).

There's a pretty good description of |typedef enum { ... } Foo;| vs |enum Foo {...};| here: http://stackoverflow.com/questions/707512/typedef-enum-in-objective-c

The typedef is fixed in mozilla-central: http://hg.mozilla.org/mozilla-central/rev/7f2aff4309a5

* 3776e9061c1d Jim Mathies – merge backout. * 08217a723505 Jim Mathies – Backout bug 555133 to fix bug 555950. * 9cae695de5b0 Jim Mathies – merge backout. * 29e175069989 Jim Mathies – Backout bug 555133 to fix bug 555950.

(In reply to comment #12) > The typedef is fixed in mozilla-central: > http://hg.mozilla.org/mozilla-central/rev/7f2aff4309a5 Does this need to backed out also?

Dale: Jim did back that out, as http://hg.mozilla.org/mozilla-central/rev/29e175069989 (the last changeset he mentioned in comment 13) (That typedef-fix was a tweak to code that was added in the main patch, so it'd be impossible to back out the main patch without also backing out that followup. :))

Yep, all backed out. A clobber build confirms the regression is fixed. We can re-land this after some bug fixing and testing.

I pushed this patch to try: http://hg.mozilla.org/try/rev/87f96ef93a37 Tests passed. I think we should reland it.

(In reply to comment #17) AFAIK, the issue that caused bug 555950 hasn't been resolved yet.

Good point. I was seeing that in my build and wondering whether I'd regressed it :-).

I found the regression problem and fixed it. Under some circumstances GetThemePartAndState would return a part code of 0 (meaning "do some default thing" --- I'm not sure exactly) or -1 (meaning "we'll render this ourselves"). In those cases, isThemeBackgroundPartiallyTransparent will return false, but the widget might not actually be opaque. So we should only call isThemeBackgroundPartiallyTransparent when we have a real part code, otherwise we should return eUnknownTransparency. This patch adds a comment to GetThemePartAndState and modifies GetWidgetTransparency.

Comment on attachment 450342 [details] [diff] [review] fix regressions Jim could review this quicker, maybe.

Comment on attachment 450342 [details] [diff] [review] fix regressions Well, I'm glad that turned out to be so simple!

Roc, looks like this also caused bug 556193 one more time.

(In reply to comment #24) > Roc, looks like this also caused bug 556193 one more time. and unearthed bug 556180 and bug 548962 for similar reasons.