555133 - Attempt to infer whether or not theme parts are transparent

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Rob Arnold [:robarnold]]

Attachment: Add nsITheme::Transparency and modify nsITheme::GetWidgetTransparency

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 1

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

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...

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(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?

Comment 3

[Rob Arnold [:robarnold]]

(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 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 435093 [details] [diff] [review]
Add nsITheme::Transparency and modify nsITheme::GetWidgetTransparency

That seems rather strange to me, but ok!

Comment 5

[Rob Arnold [:robarnold]]

Attachment: Add nsITheme::Transparency and modify nsITheme::GetWidgetTransparency

(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.

Comment 6

[Rob Arnold [:robarnold]]

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

Comment 7

[Daniel Holbert [:dholbert]]

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?

Comment 8

[Daniel Holbert [:dholbert]]

(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

Comment 9

[Rob Arnold [:robarnold]]

(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.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If you'd done |typedef enum { ... } Transparency;| it would have been OK.

BTW, I just fixed bug 550401 as an olive branch ;).

Comment 11

[Daniel Holbert [:dholbert]]

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

Comment 12

[Rob Arnold [:robarnold]]

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

Comment 13

[Jim Mathies [:jimm]]

* 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.

Comment 14

[[not reading bugmail]]

(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?

Comment 15

[Daniel Holbert [:dholbert]]

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. :))

Comment 16

[Jim Mathies [:jimm]]

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

Comment 17

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

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

Comment 18

[Karl Tomlinson (:karlt)]

(In reply to comment #17)

AFAIK, the issue that caused bug 555950 hasn't been resolved yet.

Comment 19

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

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

Comment 20

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

Attachment: fix regressions

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 21

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

Comment on attachment 450342 [details] [diff] [review]
fix regressions

Jim could review this quicker, maybe.

Comment 22

[Rob Arnold [:robarnold]]

Comment on attachment 450342 [details] [diff] [review]
fix regressions

Well, I'm glad that turned out to be so simple!

Comment 23

[Karl Tomlinson (:karlt)]

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

Comment 24

[[not reading bugmail]]

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

Comment 25

[[not reading bugmail]]

(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.