574454 - Support for rendering themed window frames, titlebar, and caption buttons

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

Split off from bug 513162. This is basically complete, just needs some testing. Landing post beta1.

Comment 1

[Jim Mathies [:jimm]]

(In reply to comment #144)
> (From update of attachment 453776 [details] [diff] [review])
> >+static PRInt32 GetTopLevelWindowActiveState(nsIFrame *aFrame)
> >+{
> >+  // Get the widget. nsIFrame's GetWindow walks up the view chain
> >+  // until it finds a window.
> >+  nsIWidget* widget = aFrame->GetWindow();
> >+  nsWindow * window = static_cast<nsWindow*>(widget);
> >+  if (widget && static_cast<nsWindow*>(widget)->GetParentWindow(PR_FALSE))
> >+    window = static_cast<nsWindow*>(widget)->GetParentWindow(PR_FALSE);
> 
> I think the extra casts can be avoided and we could cache the result of the
> lookup.
> 
> >+
> >+    case NS_THEME_WINDOW_BUTTON_MAXIMIZE:
> >+    case NS_THEME_WINDOW_BUTTON_MINIMIZE:
> >+    case NS_THEME_WINDOW_BUTTON_CLOSE:
> >+    case NS_THEME_WINDOW_BUTTON_RESTORE:
> >+    {
> >+      // min size for min/max is really small, but close is alway big enough to
> >+      // work with. Since we want xul to default to the default size, use the
> >+      // close button for every button.
> >+      aWidgetType = NS_THEME_WINDOW_BUTTON_CLOSE;
> >+      break;
> 
> I'm not on my Windows machine at the moment but I seem to remember the close
> button is the largest so why do we pick it here as the minimum size for all
> window buttons?
> 
> >+namespace mozilla {
> >+namespace widget {
> >+namespace themeconst {
> >+
> > /* 
> >  * The following constants are used to determine how a widget is drawn using
> >  * Windows' Theme API. For more information on theme parts and states see
> >  * http://msdn.microsoft.com/en-us/library/bb773210(VS.85).aspx
> >  */
> > #define THEME_COLOR 204
> > #define THEME_FONT  210
> > 
> >@@ -222,8 +226,58 @@
> > 
> > 
> > // Our extra constants for passing a little bit more info to the renderer.
> > #define DFCS_RTL             0x00010000
> > 
> > // Toolbar separator dimension which can't be gotten from Windows
> > #define TB_SEPARATOR_HEIGHT  2
> > 
> >+// Pulled from sdk/include/vsstyle.h
> >+enum {
> >+	WP_CAPTION = 1,
> >+	WP_SMALLCAPTION = 2,
> >+	WP_MINCAPTION = 3,
> >+	WP_SMALLMINCAPTION = 4,
> >+	WP_MAXCAPTION = 5,
> >+	WP_SMALLMAXCAPTION = 6,
> >+	WP_FRAMELEFT = 7,
> >+	WP_FRAMERIGHT = 8,
> >+	WP_FRAMEBOTTOM = 9,
> >+	WP_SMALLFRAMELEFT = 10,
> >+	WP_SMALLFRAMERIGHT = 11,
> >+	WP_SMALLFRAMEBOTTOM = 12,
> >+	WP_SYSBUTTON = 13,
> >+	WP_MDISYSBUTTON = 14,
> >+	WP_MINBUTTON = 15,
> >+	WP_MDIMINBUTTON = 16,
> >+	WP_MAXBUTTON = 17,
> >+	WP_CLOSEBUTTON = 18,
> >+	WP_SMALLCLOSEBUTTON = 19,
> >+	WP_MDICLOSEBUTTON = 20,
> >+	WP_RESTOREBUTTON = 21,
> >+	WP_MDIRESTOREBUTTON = 22,
> >+	WP_HELPBUTTON = 23,
> >+	WP_MDIHELPBUTTON = 24,
> >+	WP_HORZSCROLL = 25,
> >+	WP_HORZTHUMB = 26,
> >+	WP_VERTSCROLL = 27,
> >+	WP_VERTTHUMB = 28,
> >+	WP_DIALOG = 29,
> >+	WP_CAPTIONSIZINGTEMPLATE = 30,
> >+	WP_SMALLCAPTIONSIZINGTEMPLATE = 31,
> >+	WP_FRAMELEFTSIZINGTEMPLATE = 32,
> >+	WP_SMALLFRAMELEFTSIZINGTEMPLATE = 33,
> >+	WP_FRAMERIGHTSIZINGTEMPLATE = 34,
> >+	WP_SMALLFRAMERIGHTSIZINGTEMPLATE = 35,
> >+	WP_FRAMEBOTTOMSIZINGTEMPLATE = 36,
> >+	WP_SMALLFRAMEBOTTOMSIZINGTEMPLATE = 37,
> >+	WP_FRAME = 38,
> >+};
> >+
> >+enum {
> >+	BS_NORMAL = 1,
> >+	BS_HOT = 2,
> >+	BS_PUSHED = 3,
> >+	BS_DISABLED = 4,
> >+};
> >+
> >+}}} // mozilla::widget::themeconst
> 
> Seems kinda misleading to have the #defines in a namespace since only the enums
> are namespaced. Can you throw just the enums into the namespace and if you
> want, file a bug to convert everything else into namespaced enums?

Comment 2

[Jim Mathies [:jimm]]

Attachment: bits v.1

Comment 4

[Dietrich Ayala (:dietrich)]

Will this fix bug 575271? If so, can you mark the dependency?

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #4)
> Will this fix bug 575271? If so, can you mark the dependency?

It's needed to fix it, but it won't fix it on it's own. ;) We need to file a follow up browser bug for implementing custom drawn window frames.

Comment 6

[Alfred Kayser]

So, this is for Windows XP?

Comment 7

[Peter Henkel [:Terepin]]

No, as the drawing in title bar for non Aero enviroments are not yet finsihed.

Comment 8

[Tan Wei Lin Jason [:Ryuji]]

This bug is for All Windows platforms.

Comment 9

[Jim Mathies [:jimm]]

(In reply to comment #6)
> So, this is for Windows XP?

(In reply to comment #7)
> No, as the drawing in title bar for non Aero enviroments are not yet finsihed.

(In reply to comment #8)
> This bug is for All Windows platforms.

This bug is specific to giving us the ability to render a properly themed window frame and titlebar via content on os desktops that do not use glass. Anything beyond that should be filed in other bugs.

Was hoping to get this posted and reviewed this week, but the mozilla summit will likely force that goal back to early next week.

Comment 10

[Peter Henkel [:Terepin]]

Jim, is there bug for making title bar fully customizable?

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #10)
> Jim, is there bug for making title bar fully customizable?

Once we implement dependent bug 575870, you'll be able to customize it via user css and addins will also be able to muck with it. The new window frame will be xul (browser chrome) content, so everything you can do today to the browser chrome will also be possible on the window frame & titlebar.

Comment 12

[Peter Henkel [:Terepin]]

So users will be able put there buttons?

Comment 13

[imradyurrad]

(In reply to comment #12)
> So users will be able put there buttons?

No, that's not what he meant.

And let's focus on actually getting the current title bar bugs fixed before we make it customizable. Don't wanna break even more major features.

Comment 14

[imradyurrad]

Disregard the first part. It seems it will be customizable =)

Comment 15

[Jim Mathies [:jimm]]

(In reply to comment #12)
> So users will be able put there buttons?

Generally, yes. The beginnings of this are now available on desktops that support glass. For example, the fx button is just browser chrome with a transparent background. (..and the "personas draws over caption buttons" bug is a good example of a few of the gritty details we still have to work out!)

Comment 16

[[not reading bugmail]]

So to summarize, maybe I'm not understanding the schematics here, as the menu button showed up on a toolbar before drawing part 1 landed, your saying that is chrome now still.  We couldn't place widgets there before, and I'm assuming this is comment 12 and in response to that in comment 15.

Are you saying we might we be able to place/move widgets up there like we can with the menu bar?

Comment 17

[Jim Mathies [:jimm]]

(In reply to comment #16)
> So to summarize, maybe I'm not understanding the schematics here, as the menu
> button showed up on a toolbar before drawing part 1 landed, your saying that is
> chrome now still.  We couldn't place widgets there before, and I'm assuming
> this is comment 12 and in response to that in comment 15.
> 
> Are you saying we might we be able to place/move widgets up there like we can
> with the menu bar?

Yes. It's just normal xul based chrome with a transparent background. Best way to visualize it is this - in 3.6, chrome content was contained in a rectangular region within the window. For 4.0 we're removing the limitation of that rectangle.

I would wait a bit though until we have a more complete implementation. We've only removed the upper window boundary at this point. (example: https://bug513162.bugzilla.mozilla.org/attachment.cgi?id=451595) To get personas working we'll be removing the other three sides as well.

Comment 18

[Jim Mathies [:jimm]]

Attachment: css additions

Comment 19

[Jim Mathies [:jimm]]

Attachment: theme code cleanup

Comment 20

[Jim Mathies [:jimm]]

Attachment: win theme detection

Comment 21

[Jim Mathies [:jimm]]

Attachment: remove compositor check

Comment 22

[Jim Mathies [:jimm]]

Attachment: frame rendering and metrics

Comment 23

[Jim Mathies [:jimm]]

Attachment: css additions

dbaron, these are the same css changes you gave feedback on in bug 513162, with updated naming.

Comment 24

[Jim Mathies [:jimm]]

Attachment: disable chrome margins

Dao, when these patches land, the titlebar on themed desktops will be pretty badly hosed since I'm removing the disabling code in widget. This is a temporary fix that disables the set in browser.js until we get all the window framing in place.

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 457180 [details] [diff] [review]
remove compositor check

Rob - this is a simple patch that removes the compositor checks in widget.

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 457177 [details] [diff] [review]
theme code cleanup

Roc, this doesn't involve any logic changes. It's just a bit of cleanup in nsNativeThemeWin.cpp's GetMinimumWidgetSize which is setup for code I'm going to add via the "frame rendering and metrics" patch.

Comment 27

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

Comment on attachment 457177 [details] [diff] [review]
theme code cleanup

+      // down, so use the min-size request value (of 0).
+      sizeReq = TS_MIN; 
+    break;
+
+    case NS_THEME_RESIZER:
+      *aIsOverridable = PR_FALSE;
+    break;
+

+      aResult->width++;
+      aResult->height = aResult->height / 2 + 1;
+    break;

Align the breaks with the other code

+      if (aFrame->GetContent()->IsHTML())
+        sizeReq = TS_MIN;

{} around if body

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 457197 [details] [diff] [review]
disable chrome margins

>-  if (displayAppButton)
>-    document.documentElement.setAttribute("chromemargin", "0,-1,-1,-1");
>-  else
>+  //if (displayAppButton)
>+  //  document.documentElement.setAttribute("chromemargin", "0,-1,-1,-1");
>+  //else
>     document.documentElement.removeAttribute("chromemargin");

Use #ifdef 0 instead?

Comment 29

[Jim Mathies [:jimm]]

Attachment: frame rendering and metrics

Metrics and rendering for various parts of the window on different desktop variations.

Rob if you're tied up let me know and I'll switch to a different reviewer.

Comment 30

[Jim Mathies [:jimm]]

Attachment: ncclient area handling

nsWindow code that handles the ncclient area properly for various nc events.

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 457352 [details] [diff] [review]
ncclient area handling

Neil, care to take a crack at reviewing this?

Comment 32

[Jim Mathies [:jimm]]

Attachment: ncclient area handling

Small update - removed some debug code and fixed a comment typo.

Comment 33

[Jim Mathies [:jimm]]

Attachment: test xulrunner app

This is a basic xul runner app that supports custom frame rendering.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 457195 [details] [diff] [review]
css additions

r=dbaron

Comment 35

[[e-mail]]

Comment on attachment 457372 [details] [diff] [review]
ncclient area handling

>+  if (mNonClientMargins.top != -1) {
>+    rect.left   = 0;
>+    rect.top    = 0;
Shouldn't this be mNonClientMargins.top?

>+nsWindow::ExcludeNonClientFromPaintRegion(HRGN aRegion)
Can we not just exclude the entire extended client area here?

>+    gfxRgn.Sub(gfxRgn, rect);
>+  }
>+
>+  // Assemble the HRGN windows will use to paint nc client areas we haven't
>+  // extended into.
>+  const nsIntRect* pRect;
>+  HRGN paintRgn = CreateRectRgn(0, 0, 0, 0);
Why not use CombineRgn to substrct the rectangle in the first place?

>+        ValidateNonClientMargins(PR_TRUE);
The parameter always seems to be PR_TRUE?

Comment 36

[Jim Mathies [:jimm]]

(In reply to comment #35)
> Comment on attachment 457372 [details] [diff] [review]
> ncclient area handling
> 
> >+  if (mNonClientMargins.top != -1) {
> >+    rect.left   = 0;
> >+    rect.top    = 0;
> Shouldn't this be mNonClientMargins.top?

left and top are client area coordinates. We're invalidating the non-client areas we render manually (in our client area), so that when window activation changes, the frame redraws using the right theme.

> 
> >+nsWindow::ExcludeNonClientFromPaintRegion(HRGN aRegion)
> Can we not just exclude the entire extended client area here?

That's what the code is doing. the region we want windows to paint is complex for each side, it includes the margins plus anything beyond the nc client area for a particular side. 

For example, on top:

____________
| |   a  | | we paint in 'a'
| |------| |
 ^        ^ windows paints these

If we don't exclude 'a' from the region handed to defwndproc, windows will also paint in 'a'. We have to do the right calculation for each side.

> 
> >+    gfxRgn.Sub(gfxRgn, rect);
> >+  }
> >+
> >+  // Assemble the HRGN windows will use to paint nc client areas we haven't
> >+  // extended into.
> >+  const nsIntRect* pRect;
> >+  HRGN paintRgn = CreateRectRgn(0, 0, 0, 0);
> Why not use CombineRgn to substrct the rectangle in the first place?

Not sure I understand the question, are you asking why I'm not manipulating the original windows region I'm handed?

> 
> >+        ValidateNonClientMargins(PR_TRUE);
> The parameter always seems to be PR_TRUE?

I was originally using both, but found I didn't need it in the end. Left the validation part baked into the method for future use.

Comment 37

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

We need this work done for a beta, not sure if it needs to be beta 3, I'll figure that out with shorlander and jimm offline.

Comment 38

[Jim Mathies [:jimm]]

(In reply to comment #37)
> We need this work done for a beta, not sure if it needs to be beta 3, I'll
> figure that out with shorlander and jimm offline.

This should be in in the next few days. The next key blocker is bug 575870, which leverages this work to implement the custom frame in the browser.

Comment 39

[[e-mail]]

Let's see if I have this straight. Normally the window consists of a non client area and a client area. The client area is smaller because of the non client margins in which the border and caption are usually drawn. However the widget code allows us to decrease the non client margins adding extra client area thus creating an extended client area.

(In reply to comment #36)
> (In reply to comment #35)
> left and top are client area coordinates. We're invalidating the non-client
> areas we render manually (in our client area), so that when window activation
> changes, the frame redraws using the right theme.
So the area we need to invalidate is the extra client area created by reducing the non client margins, but in the coordinates of the new area?

> If we don't exclude 'a' from the region handed to defwndproc, windows will also
> paint in 'a'. We have to do the right calculation for each side.
If you add the areas we don't want DefWndProc to paint to the normal client area, does that not result in the extended client area? Can we not exclude the extra client area indirectly by excluding the extended client area?

> Not sure I understand the question, are you asking why I'm not manipulating the
> original windows region I'm handed?
Well, why not create the region using the original region rather than flipping it through gfx and back, yes.

> I was originally using both, but found I didn't need it in the end. Left the
> validation part baked into the method for future use.
Fair enough, I was just confused by the naming The best suggestion I have is InvalidateNonClientMargins(PRBool aInvalidate = PR_TRUE); and pass PR_FALSE to validate. Or s/Inv/V/g and pass PR_FALSE to invalidate.

Comment 40

[Jim Mathies [:jimm]]

(In reply to comment #39)
> Let's see if I have this straight. Normally the window consists of a non client
> area and a client area. The client area is smaller because of the non client
> margins in which the border and caption are usually drawn. However the widget
> code allows us to decrease the non client margins adding extra client area thus
> creating an extended client area.
> 

Yes, that pretty much sums it up. We can extend any side into the non-client area using an attribute on the window. For example we're currently extending the top margin on glass desktops for the fx button.

> (In reply to comment #36)
> > (In reply to comment #35)
> > left and top are client area coordinates. We're invalidating the non-client
> > areas we render manually (in our client area), so that when window activation
> > changes, the frame redraws using the right theme.
> So the area we need to invalidate is the extra client area created by reducing
> the non client margins, but in the coordinates of the new area?

Yes.

> 
> > If we don't exclude 'a' from the region handed to defwndproc, windows will also
> > paint in 'a'. We have to do the right calculation for each side.
> If you add the areas we don't want DefWndProc to paint to the normal client
> area, does that not result in the extended client area? Can we not exclude the
> extra client area indirectly by excluding the extended client area?

Yes. Although it's a complex region, so it's not as simple as GetClientRect(rect) -> ExcludeFromRegion(rect).
 
Note DefWndProc doesn't know about our new client areas. I know that sounds a little crazy, but DefWndProc will just go right on painting over the new client area where it thinks non-client is supposed to be. Windows expects we'll handle WM_NCPAINT, so there's a disconnect between what DefWndProc does for WM_NCPAINT events and the new non-client settings we set. You can still get DefWndProc to do a lot of the painting though by setting up your own region, which is what we're doing here.

> 
> > Not sure I understand the question, are you asking why I'm not manipulating the
> > original windows region I'm handed?
> Well, why not create the region using the original region rather than flipping
> it through gfx and back, yes.

I can test modifying the region windows hands in, might work, should work! :) Will try it.

> 
> > I was originally using both, but found I didn't need it in the end. Left the
> > validation part baked into the method for future use.
> Fair enough, I was just confused by the naming The best suggestion I have is
> InvalidateNonClientMargins(PRBool aInvalidate = PR_TRUE); and pass PR_FALSE to
> validate. Or s/Inv/V/g and pass PR_FALSE to invalidate.

Yeah, gotcha. Will reverse the meaning in the name.

Comment 41

[Jim Mathies [:jimm]]

(In reply to comment #40)
> (In reply to comment #39)
> > If you add the areas we don't want DefWndProc to paint to the normal client
> > area, does that not result in the extended client area? Can we not exclude the
> > extra client area indirectly by excluding the extended client area?
> 
> Yes. Although it's a complex region, so it's not as simple as
> GetClientRect(rect) -> ExcludeFromRegion(rect).

Actually that wasn't accurate, the client area is a simple region. It's the non-client area that's complex.

Comment 42

[Jim Mathies [:jimm]]

(In reply to comment #40)
> (In reply to comment #39)
> > > Not sure I understand the question, are you asking why I'm not manipulating the
> > > original windows region I'm handed?
> > Well, why not create the region using the original region rather than flipping
> > it through gfx and back, yes.
> 
> I can test modifying the region windows hands in, might work, should work! :)
> Will try it.

Hmm, this really doesn't work, because nine times out of ten we get '1' for the region, which there's a comment about in the patch. It basically means repaint everything. So we end up having to create our own region most of the time.

Comment 43

[Jim Mathies [:jimm]]

Attachment: ncclient area handling v.2

Comment 44

[Jim Mathies [:jimm]]

Comment on attachment 458684 [details] [diff] [review]
ncclient area handling v.2

Here's an updated patch which reverses the meaning on that validate call, and I took your suggestion and used the client rect in place of building up the region manually. (Which shortened that method down quite nicely.)

Comment 45

[Jim Mathies [:jimm]]

I should add a caveat, I need to test on winXP first before I proclaim success.

Comment 46

[[e-mail]]

Comment on attachment 458684 [details] [diff] [review]
ncclient area handling v.2

Almost there... just two more questions!

>+  if (mNonClientMargins.top != -1) {
>+    rect.left   = 0;
>+    rect.top    = 0;
>+    rect.right  = mBounds.width;
>+    rect.bottom = mCaptionHeight;
Shouldn't this be mCaptionHeight - mNonClientMargins.top?
(Currently .top is always -1 or 0 so you don't notice)
Similarly for the other sides, I guess.

>+  if ((int)aRegion == 1) { // undocumented value indicating a full refresh
I don't think an int is the same as a WPARAM. Perhaps use (HRGN)1 instead?

>+  RECT rect = {0};
[I don't see the point of this, given that GetClientRect writes the rect.]

>+  POINT origin = {rect.left, rect.top};
>+  POINT dim = {rect.right, rect.bottom};
>+  ClientToScreen(mWnd, &origin);
>+  ClientToScreen(mWnd, &dim);
[Possibly use MapWindowPoints here.]

>+  void                    InvalidateNonClientMargins(PRBool aValidate = PR_FALSE);
[Not what I was thinking of, but still better than last time.]

Comment 47

[Jim Mathies [:jimm]]

(In reply to comment #46)
> Comment on attachment 458684 [details] [diff] [review]
> ncclient area handling v.2
> 
> Almost there... just two more questions!
> 
> >+  if (mNonClientMargins.top != -1) {
> >+    rect.left   = 0;
> >+    rect.top    = 0;
> >+    rect.right  = mBounds.width;
> >+    rect.bottom = mCaptionHeight;
> Shouldn't this be mCaptionHeight - mNonClientMargins.top?
> (Currently .top is always -1 or 0 so you don't notice)
> Similarly for the other sides, I guess.

I was just being lazy by invalidating the whole area. I've updated this to only invalidate the areas we draw to.
 
> 
> >+  if ((int)aRegion == 1) { // undocumented value indicating a full refresh
> I don't think an int is the same as a WPARAM. Perhaps use (HRGN)1 instead?

updated.


> >+  RECT rect = {0};
> [I don't see the point of this, given that GetClientRect writes the rect.]

updated.

> >+  POINT origin = {rect.left, rect.top};
> >+  POINT dim = {rect.right, rect.bottom};
> >+  ClientToScreen(mWnd, &origin);
> >+  ClientToScreen(mWnd, &dim);
> [Possibly use MapWindowPoints here.]

updated.

> 
> >+  void                    InvalidateNonClientMargins(PRBool aValidate = PR_FALSE);
> [Not what I was thinking of, but still better than last time.]

updated to: InvalidateNonClientMargins(PRBool aInvalidate)

Comment 48

[Jim Mathies [:jimm]]

Attachment: ncclient area handling v.3

Comment 49

[Jim Mathies [:jimm]]

Comment on attachment 458736 [details] [diff] [review]
ncclient area handling v.3

argh, missed one rect = {0}.

Comment 50

[Jim Mathies [:jimm]]

Attachment: ncclient area handling v.3

Comment 51

[Jim Mathies [:jimm]]

Attachment: frame rendering and metrics

Nixed the nsIntSize for the command buttons, and pre-populated the data with system metrics. Should be a little safer.

Comment 52

[[e-mail]]

Comment on attachment 458737 [details] [diff] [review]
ncclient area handling v.3

>+  MapWindowPoints(mWnd, NULL, pt, 2);
MSDN suggests that you can pass the RECT itself instead.

>+  HRGN nonClientRgn = CreateRectRgn(pt[0].x, pt[0].y, pt[1].x, pt[1].y);
Strictly speaking this is the client region that we're excluding ;-)

Comment 53

[Jim Mathies [:jimm]]

(In reply to comment #52)
> Comment on attachment 458737 [details] [diff] [review]
> ncclient area handling v.3
> 
> >+  MapWindowPoints(mWnd, NULL, pt, 2);
> MSDN suggests that you can pass the RECT itself instead.
> 
> >+  HRGN nonClientRgn = CreateRectRgn(pt[0].x, pt[0].y, pt[1].x, pt[1].y);
> Strictly speaking this is the client region that we're excluding ;-)

Doh!, your right. I can just pass the rect. :) I'll update the final patch.

Comment 54

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

Comment on attachment 458748 [details] [diff] [review]
frame rendering and metrics

Looks fine to me, one comment:

>+      // inset the caption area so it doesn't overflow.
>+      RECT rect = widgetRect;
>+      OffsetRect(&rect, 4, 4);
>+      rect.bottom -= 4;
>+      rect.right -= 8;

Where do the magic 4 numbers come from?  Maybe add a comment explaining that.  (Also may just want to use rect.top += 4; rect.left += 4; instead of OffsetRect, since you're already directly modifying bottom/right.)

>+
>+      // if enabled, draw a gradient titlebar background, otherwise
>+      // fill with a solid color.
>+      BOOL bFlag = TRUE;
>+      SystemParametersInfo(SPI_GETGRADIENTCAPTIONS, 0, &bFlag, NULL);
>+      if (!bFlag) {
>+        if (state == mozilla::widget::themeconst::FS_ACTIVE)
>+          FillRect(hdc, &rect, (HBRUSH)(COLOR_ACTIVECAPTION+1));
>+        else
>+          FillRect(hdc, &rect, (HBRUSH)(COLOR_INACTIVECAPTION+1));
>+      } else {
>+        DWORD startColor, endColor;
>+        if (state == mozilla::widget::themeconst::FS_ACTIVE) {
>+          startColor = GetSysColor(COLOR_ACTIVECAPTION);
>+          endColor = GetSysColor(COLOR_GRADIENTACTIVECAPTION);
>+        } else {
>+          startColor = GetSysColor(COLOR_INACTIVECAPTION);
>+          endColor = GetSysColor(COLOR_GRADIENTINACTIVECAPTION);
>+        }
>+
>+        TRIVERTEX vertex[2];
>+        vertex[0].x     = rect.left;
>+        vertex[0].y     = rect.top;
>+        vertex[0].Red   = GetRValue(startColor) << 8;
>+        vertex[0].Green = GetGValue(startColor) << 8;
>+        vertex[0].Blue  = GetBValue(startColor) << 8;
>+        vertex[0].Alpha = 0;
>+
>+        vertex[1].x     = rect.right;
>+        vertex[1].y     = rect.bottom; 
>+        vertex[1].Red   = GetRValue(endColor) << 8;
>+        vertex[1].Green = GetGValue(endColor) << 8;
>+        vertex[1].Blue  = GetBValue(endColor) << 8;
>+        vertex[1].Alpha = 0;
>+
>+        GRADIENT_RECT gRect;
>+        gRect.UpperLeft  = 0;
>+        gRect.LowerRight = 1;
>+        // available on win2k & up
>+        GradientFill(hdc, vertex, 2, &gRect, 1, GRADIENT_FILL_RECT_H);
>+      }
>+
>+      // frame things up with the left, top, and right raised borders.
>+      DrawEdge(hdc, &widgetRect, EDGE_RAISED, BF_TOP|BF_LEFT|BF_RIGHT);
>+      break;
>+    }
>+
>+    case NS_THEME_WINDOW_FRAME_LEFT:
>+      DrawEdge(hdc, &widgetRect, EDGE_RAISED, BF_LEFT);
>+      break;
>+
>+    case NS_THEME_WINDOW_FRAME_RIGHT:
>+      DrawEdge(hdc, &widgetRect, EDGE_RAISED, BF_RIGHT);
>+      break;
>+
>+    case NS_THEME_WINDOW_FRAME_BOTTOM:
>+      DrawEdge(hdc, &widgetRect, EDGE_RAISED, BF_BOTTOM);
>+      break;
>+
>+    case NS_THEME_WINDOW_BUTTON_CLOSE:
>+    case NS_THEME_WINDOW_BUTTON_MINIMIZE:
>+    case NS_THEME_WINDOW_BUTTON_MAXIMIZE:
>+    case NS_THEME_WINDOW_BUTTON_RESTORE:
>+    {
>+      PRInt32 oldTA = SetTextAlign(hdc, TA_TOP | TA_LEFT | TA_NOUPDATECP);
>+      DrawFrameControl(hdc, &widgetRect, part, state);
>+      SetTextAlign(hdc, oldTA);
>+      break;
>+    }
>+
>     default:
>       rv = NS_ERROR_FAILURE;
>       break;
>   }
> 
>   nativeDrawing.EndNativeDrawing();
> 
>   if (NS_FAILED(rv))
>diff --git a/widget/src/windows/nsUXThemeConstants.h b/widget/src/windows/nsUXThemeConstants.h
>--- a/widget/src/windows/nsUXThemeConstants.h
>+++ b/widget/src/windows/nsUXThemeConstants.h
>@@ -222,8 +222,68 @@
> 
> 
> // Our extra constants for passing a little bit more info to the renderer.
> #define DFCS_RTL             0x00010000
> 
> // Toolbar separator dimension which can't be gotten from Windows
> #define TB_SEPARATOR_HEIGHT  2
> 
>+namespace mozilla {
>+namespace widget {
>+namespace themeconst {
>+
>+// Pulled from sdk/include/vsstyle.h
>+enum {
>+	WP_CAPTION = 1,
>+	WP_SMALLCAPTION = 2,
>+	WP_MINCAPTION = 3,
>+	WP_SMALLMINCAPTION = 4,
>+	WP_MAXCAPTION = 5,
>+	WP_SMALLMAXCAPTION = 6,
>+	WP_FRAMELEFT = 7,
>+	WP_FRAMERIGHT = 8,
>+	WP_FRAMEBOTTOM = 9,
>+	WP_SMALLFRAMELEFT = 10,
>+	WP_SMALLFRAMERIGHT = 11,
>+	WP_SMALLFRAMEBOTTOM = 12,
>+	WP_SYSBUTTON = 13,
>+	WP_MDISYSBUTTON = 14,
>+	WP_MINBUTTON = 15,
>+	WP_MDIMINBUTTON = 16,
>+	WP_MAXBUTTON = 17,
>+	WP_CLOSEBUTTON = 18,
>+	WP_SMALLCLOSEBUTTON = 19,
>+	WP_MDICLOSEBUTTON = 20,
>+	WP_RESTOREBUTTON = 21,
>+	WP_MDIRESTOREBUTTON = 22,
>+	WP_HELPBUTTON = 23,
>+	WP_MDIHELPBUTTON = 24,
>+	WP_HORZSCROLL = 25,
>+	WP_HORZTHUMB = 26,
>+	WP_VERTSCROLL = 27,
>+	WP_VERTTHUMB = 28,
>+	WP_DIALOG = 29,
>+	WP_CAPTIONSIZINGTEMPLATE = 30,
>+	WP_SMALLCAPTIONSIZINGTEMPLATE = 31,
>+	WP_FRAMELEFTSIZINGTEMPLATE = 32,
>+	WP_SMALLFRAMELEFTSIZINGTEMPLATE = 33,
>+	WP_FRAMERIGHTSIZINGTEMPLATE = 34,
>+	WP_SMALLFRAMERIGHTSIZINGTEMPLATE = 35,
>+	WP_FRAMEBOTTOMSIZINGTEMPLATE = 36,
>+	WP_SMALLFRAMEBOTTOMSIZINGTEMPLATE = 37,
>+	WP_FRAME = 38,
>+};
>+
>+enum FRAMESTATES {
>+	FS_ACTIVE = 1,
>+	FS_INACTIVE = 2,
>+};
>+
>+enum {
>+	BS_NORMAL = 1,
>+	BS_HOT = 2,
>+	BS_PUSHED = 3,
>+	BS_DISABLED = 4,
>+	BS_INACTIVE = 5, /* undocumented, inactive caption button */
>+};
>+
>+}}} // mozilla::widget::themeconst
>diff --git a/widget/src/windows/nsUXThemeData.cpp b/widget/src/windows/nsUXThemeData.cpp
>--- a/widget/src/windows/nsUXThemeData.cpp
>+++ b/widget/src/windows/nsUXThemeData.cpp
>@@ -65,16 +65,19 @@ BOOL
> nsUXThemeData::sFlatMenus = FALSE;
> PRPackedBool
> nsUXThemeData::sIsXPOrLater = PR_FALSE;
> PRPackedBool
> nsUXThemeData::sIsVistaOrLater = PR_FALSE;
> PRPackedBool
> nsUXThemeData::sHaveCompositor = PR_FALSE;
> 
>+PRBool nsUXThemeData::sTitlebarInfoPopulated = PR_FALSE;
>+SIZE nsUXThemeData::sCommandButtons[3];
>+
> nsUXThemeData::OpenThemeDataPtr nsUXThemeData::openTheme = NULL;
> nsUXThemeData::CloseThemeDataPtr nsUXThemeData::closeTheme = NULL;
> nsUXThemeData::DrawThemeBackgroundPtr nsUXThemeData::drawThemeBG = NULL;
> nsUXThemeData::DrawThemeEdgePtr nsUXThemeData::drawThemeEdge = NULL;
> nsUXThemeData::GetThemeContentRectPtr nsUXThemeData::getThemeContentRect = NULL;
> nsUXThemeData::GetThemeBackgroundRegionPtr nsUXThemeData::getThemeBackgroundRegion = NULL;
> nsUXThemeData::GetThemePartSizePtr nsUXThemeData::getThemePartSize = NULL;
> nsUXThemeData::GetThemeSysFontPtr nsUXThemeData::getThemeSysFont = NULL;
>@@ -218,13 +221,53 @@ const wchar_t *nsUXThemeData::GetClassNa
>     case eUXCombobox:
>       return L"Combobox";
>     case eUXHeader:
>       return L"Header";
>     case eUXListview:
>       return L"Listview";
>     case eUXMenu:
>       return L"Menu";
>+    case eUXWindowFrame:
>+      return L"Window";
>     default:
>       NS_NOTREACHED("unknown uxtheme class");
>       return L"";
>   }
> }
>+
>+// static
>+void
>+nsUXThemeData::InitTitlebarInfo()
>+{
>+  // Pre-populate with generic metrics. These likley will not match
>+  // the current theme, but they insure the buttons at least show up.
>+  sCommandButtons[0].cx = GetSystemMetrics(SM_CXSIZE);
>+  sCommandButtons[0].cy = GetSystemMetrics(SM_CYSIZE);
>+  sCommandButtons[1].cx = sCommandButtons[2].cx = sCommandButtons[0].cx;
>+  sCommandButtons[1].cy = sCommandButtons[2].cy = sCommandButtons[0].cy;
>+}
>+
>+// static
>+void
>+nsUXThemeData::UpdateTitlebarInfo(TITLEBARINFOEX& info)
>+{
>+  if (sTitlebarInfoPopulated)
>+    return;
>+
>+  // Only set if we have valid data for all three buttons we use.
>+  if ((info.rgrect[2].right - info.rgrect[2].left) == 0 ||
>+      (info.rgrect[3].right - info.rgrect[3].left) == 0 ||
>+      (info.rgrect[5].right - info.rgrect[5].left) == 0)
>+    return;
>+
>+  // minimize
>+  sCommandButtons[0].cx = info.rgrect[2].right - info.rgrect[2].left;
>+  sCommandButtons[0].cy = info.rgrect[2].bottom - info.rgrect[2].top;
>+  // maximize/restore
>+  sCommandButtons[1].cx = info.rgrect[3].right - info.rgrect[3].left;
>+  sCommandButtons[1].cy = info.rgrect[3].bottom - info.rgrect[3].top;
>+  // close
>+  sCommandButtons[2].cx = info.rgrect[5].right - info.rgrect[5].left;
>+  sCommandButtons[2].cy = info.rgrect[5].bottom - info.rgrect[5].top;
>+
>+  sTitlebarInfoPopulated = PR_TRUE;
>+}
>diff --git a/widget/src/windows/nsUXThemeData.h b/widget/src/windows/nsUXThemeData.h
>--- a/widget/src/windows/nsUXThemeData.h
>+++ b/widget/src/windows/nsUXThemeData.h
>@@ -43,16 +43,18 @@
> #include <uxtheme.h>
> 
> #include "nscore.h"
> 
> #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
> #include <dwmapi.h>
> #endif
> 
>+#include "nsWindowDefs.h"
>+
> // These window messages are not defined in dwmapi.h
> #ifndef WM_DWMCOMPOSITIONCHANGED
> #define WM_DWMCOMPOSITIONCHANGED        0x031E
> #endif
> 
> // Windows 7 additions
> #ifndef WM_DWMSENDICONICTHUMBNAIL
> #define WM_DWMSENDICONICTHUMBNAIL 0x0323
>@@ -80,19 +82,23 @@ enum nsUXThemeClass {
>   eUXScrollbar,
>   eUXTrackbar,
>   eUXSpin,
>   eUXStatus,
>   eUXCombobox,
>   eUXHeader,
>   eUXListview,
>   eUXMenu,
>+  eUXWindowFrame,
>   eUXNumClasses
> };
> 
>+#define CMDBUTTONIDX_MINIMIZE 0
>+#define CMDBUTTONIDX_RESTORE  1
>+#define CMDBUTTONIDX_CLOSE    2
> 
> class nsUXThemeData {
>   static HMODULE sThemeDLL;
> #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>    static HMODULE sDwmDLL;
> #endif
>   static HANDLE sThemes[eUXNumClasses];
>   
>@@ -102,21 +108,28 @@ public:
>   static const PRUnichar kThemeLibraryName[];
> #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>    static const PRUnichar kDwmLibraryName[];
> #endif
>   static BOOL sFlatMenus;
>   static PRPackedBool sIsXPOrLater;
>   static PRPackedBool sIsVistaOrLater;
>   static PRPackedBool sHaveCompositor;
>+  static PRBool sTitlebarInfoPopulated;
>+  static SIZE sCommandButtons[3];
>+
>   static void Initialize();
>   static void Teardown();
>   static void Invalidate();
>   static HANDLE GetTheme(nsUXThemeClass cls);
> 
>+  // nsWindow calls this to update desktop settings info
>+  static void InitTitlebarInfo();
>+  static void UpdateTitlebarInfo(TITLEBARINFOEX& info);
>+
>   static inline BOOL IsAppThemed() {
>     return isAppThemed && isAppThemed();
>   }
> 
>   static inline HRESULT GetThemeColor(nsUXThemeClass cls, int iPartId, int iStateId,
>                                                    int iPropId, OUT COLORREF* pFont) {
>     if(!getThemeColor)
>       return E_FAIL;
>diff --git a/widget/src/windows/nsWindow.cpp b/widget/src/windows/nsWindow.cpp
>--- a/widget/src/windows/nsWindow.cpp
>+++ b/widget/src/windows/nsWindow.cpp
>@@ -152,16 +152,17 @@
> #include "nsCRT.h"
> #include "nsAppDirectoryServiceDefs.h"
> #include "nsXPIDLString.h"
> #include "nsWidgetsCID.h"
> #include "nsTHashtable.h"
> #include "nsHashKeys.h"
> #include "nsString.h"
> #include "mozilla/Services.h"
>+#include "nsNativeThemeWin.h"
> 
> #if defined(WINCE)
> #include "nsWindowCE.h"
> #endif
> 
> #if defined(WINCE_WINDOWS_MOBILE)
> #define KILL_PRIORITY_ID 2444
> #endif
>@@ -404,39 +405,44 @@ nsWindow::nsWindow() : nsBaseWidget()
> #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7
>   mTaskbarPreview = nsnull;
>   mHasTaskbarIconBeenCreated = PR_FALSE;
> #endif
> 
>   // Global initialization
>   if (!sInstanceCount) {
> #if !defined(WINCE)
>-  gKbdLayout.LoadLayout(::GetKeyboardLayout(0));
>-#endif
>-
>-  // Init IME handler
>-  nsIMM32Handler::Initialize();
>-
>-#ifdef NS_ENABLE_TSF
>-  nsTextStore::Initialize();
>-#endif
>-
>-#if !defined(WINCE)
>-  if (SUCCEEDED(::OleInitialize(NULL)))
>-    sIsOleInitialized = TRUE;
>-  NS_ASSERTION(sIsOleInitialized, "***** OLE is not initialized!\n");
>+    gKbdLayout.LoadLayout(::GetKeyboardLayout(0));
>+#endif
>+
>+    // Init IME handler
>+    nsIMM32Handler::Initialize();
>+
>+#ifdef NS_ENABLE_TSF
>+    nsTextStore::Initialize();
>+#endif
>+
>+#if !defined(WINCE)
>+    if (SUCCEEDED(::OleInitialize(NULL)))
>+      sIsOleInitialized = TRUE;
>+    NS_ASSERTION(sIsOleInitialized, "***** OLE is not initialized!\n");
> #endif
> 
> #if defined(HEAP_DUMP_EVENT)
>-  InitHeapDump();
>-#endif
>-
>-#if !defined(WINCE)
>-  InitTrackPointHack();
>-#endif
>+    InitHeapDump();
>+#endif
>+
>+#if !defined(WINCE)
>+    InitTrackPointHack();
>+#endif
>+
>+    // Init titlebar button info for custom frames.
>+    if (GetWindowsVersion() >= VISTA_VERSION) {
>+      nsUXThemeData::InitTitlebarInfo();
>+    }
>   } // !sInstanceCount
> 
>   mIdleService = nsnull;
> 
>   sInstanceCount++;
> }
> 
> nsWindow::~nsWindow()
>@@ -1207,16 +1213,26 @@ NS_METHOD nsWindow::Show(PRBool bState)
>     }
>   }
>   
> #ifdef MOZ_XUL
>   if (!wasVisible && bState)
>     Invalidate(PR_FALSE);
> #endif
> 
>+  // Update titlebar metric info when the window is shown.
>+  if (!nsUXThemeData::sTitlebarInfoPopulated && bState &&
>+      GetWindowsVersion() >= VISTA_VERSION &&
>+      (mWindowType == eWindowType_toplevel || mWindowType == eWindowType_dialog) &&
>+      (mBorderStyle == eBorderStyle_default || mBorderStyle == eBorderStyle_all)) {
>+    TITLEBARINFOEX info = {0};
>+    info.cbSize = sizeof(TITLEBARINFOEX);
>+    SendMessage(mWnd, WM_GETTITLEBARINFOEX, 0, (LPARAM)&info); 
>+    nsUXThemeData::UpdateTitlebarInfo(info);
>+  }
>   return NS_OK;
> }
> 
> /**************************************************************
>  *
>  * SECTION: nsIWidget::IsVisible
>  *
>  * Returns the visibility state.
>diff --git a/widget/src/windows/nsWindow.h b/widget/src/windows/nsWindow.h
>--- a/widget/src/windows/nsWindow.h
>+++ b/widget/src/windows/nsWindow.h
>@@ -234,16 +234,17 @@ public:
>    */
>   static void             GlobalMsgWindowProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam);
>   nsWindow*               GetTopLevelWindow(PRBool aStopOnDialogOrPopup);
>   static HWND             GetTopLevelHWND(HWND aWnd, PRBool aStopOnDialogOrPopup = PR_FALSE);
>   HWND                    GetWindowHandle() { return mWnd; }
>   WNDPROC                 GetPrevWindowProc() { return mPrevWndProc; }
>   static nsWindow*        GetNSWindowPtr(HWND aWnd);
>   WindowHook&             GetWindowHook() { return mWindowHook; }
>+  nsWindow*               GetParentWindow(PRBool aIncludeOwner);
> 
>   /**
>    * Misc.
>    */
>   virtual PRBool          AutoErase(HDC dc);
>   nsIntPoint*             GetLastPoint() { return &mLastPoint; }
>   PRBool                  GetIMEEnabled() { return mIMEEnabled; }
>   // needed in nsIMM32Handler.cpp
>@@ -279,17 +280,16 @@ protected:
>   static VOID    CALLBACK HookTimerForPopups( HWND hwnd, UINT uMsg, UINT idEvent, DWORD dwTime );
> 
>   /**
>    * Window utilities
>    */
>   static BOOL             SetNSWindowPtr(HWND aWnd, nsWindow * ptr);
>   LPARAM                  lParamToScreen(LPARAM lParam);
>   LPARAM                  lParamToClient(LPARAM lParam);
>-  nsWindow*               GetParentWindow(PRBool aIncludeOwner);
>   virtual void            SubclassWindow(BOOL bState);
>   PRBool                  CanTakeFocus();
>   PRBool                  UpdateNonClientMargins(PRInt32 aSizeMode = -1, PRBool aReflowWindow = PR_TRUE);
>   void                    ResetLayout();
> #if !defined(WINCE)
>   static void             InitTrackPointHack();
> #endif
> 
>diff --git a/widget/src/windows/nsWindowDbg.cpp b/widget/src/windows/nsWindowDbg.cpp
>--- a/widget/src/windows/nsWindowDbg.cpp
>+++ b/widget/src/windows/nsWindowDbg.cpp
>@@ -389,16 +389,17 @@ EventMsgInfo gAllEvents[] = {
>   {"WM_DWMNCRENDERINGCHANGED",            0x031F},
>   {"WM_DWMCOLORIZATIONCOLORCHANGED",      0x0320},
>   {"WM_DWMWINDOWMAXIMIZEDCHANGE",         0x0321},
>   {"WM_DWMSENDICONICTHUMBNAIL",           0x0323},
>   {"WM_DWMSENDICONICLIVEPREVIEWBITMAP",   0x0326},
>   {"WM_TABLET_QUERYSYSTEMGESTURESTATUS",  0x02CC},
>   {"WM_GESTURE",                          0x0119},
>   {"WM_GESTURENOTIFY",                    0x011A},
>+  {"WM_GETTITLEBARINFOEX",                0x033F},
>   {NULL, 0x0}
> };
> 
> static long gEventCounter = 0;
> static long gLastEventMsg = 0;
> 
> void PrintEvent(UINT msg, PRBool aShowAllEvents, PRBool aShowMouseMoves)
> {
>diff --git a/widget/src/windows/nsWindowDefs.h b/widget/src/windows/nsWindowDefs.h
>--- a/widget/src/windows/nsWindowDefs.h
>+++ b/widget/src/windows/nsWindowDefs.h
>@@ -105,16 +105,24 @@
> // App Command messages for IntelliMouse and Natural Keyboard Pro
> // These messages are not included in Visual C++ 6.0, but are in 7.0+
> #ifndef WM_APPCOMMAND
> #define WM_APPCOMMAND                     0x0319
> #endif
> 
> #define FAPPCOMMAND_MASK                  0xF000
> 
>+#ifndef WM_GETTITLEBARINFOEX
>+#define WM_GETTITLEBARINFOEX              0x033F
>+#endif
>+
>+#ifndef CCHILDREN_TITLEBAR
>+#define CCHILDREN_TITLEBAR                5
>+#endif
>+
> #ifndef APPCOMMAND_BROWSER_BACKWARD
>   #define APPCOMMAND_BROWSER_BACKWARD       1
>   #define APPCOMMAND_BROWSER_FORWARD        2
>   #define APPCOMMAND_BROWSER_REFRESH        3
>   #define APPCOMMAND_BROWSER_STOP           4
>   #define APPCOMMAND_BROWSER_SEARCH         5
>   #define APPCOMMAND_BROWSER_FAVORITES      6
>   #define APPCOMMAND_BROWSER_HOME           7
>@@ -167,18 +175,16 @@
> #define TABLET_INK_SIGNATURE 0xFFFFFF00
> #define TABLET_INK_CHECK     0xFF515700
> #define TABLET_INK_TOUCH     0x00000080
> #define MOUSE_INPUT_SOURCE() GetMouseInputSource()
> #else
> #define MOUSE_INPUT_SOURCE() nsIDOMNSMouseEvent::MOZ_SOURCE_MOUSE
> #endif
> 
>-
>-
> /**************************************************************
>  *
>  * SECTION: enums
>  * 
>  **************************************************************/
> 
> // nsWindow::sCanQuit
> typedef enum
>@@ -254,16 +260,26 @@ struct nsModifierKeyState {
> // Used for synthesizing events
> struct KeyPair {
>   PRUint8 mGeneral;
>   PRUint8 mSpecific;
>   KeyPair(PRUint32 aGeneral, PRUint32 aSpecific)
>     : mGeneral(PRUint8(aGeneral)), mSpecific(PRUint8(aSpecific)) {}
> };
> 
>+#ifndef TITLEBARINFOEX
>+struct TITLEBARINFOEX
>+{
>+    DWORD cbSize;
>+    RECT rcTitleBar;
>+    DWORD rgstate[CCHILDREN_TITLEBAR + 1];
>+    RECT rgrect[CCHILDREN_TITLEBAR + 1];
>+};
>+#endif
>+
> /**************************************************************
>  *
>  * SECTION: macros
>  * 
>  **************************************************************/
> 
> #define NSRGB_2_COLOREF(color) \
>       RGB(NS_GET_R(color),NS_GET_G(color),NS_GET_B(color))

Comment 55

[Jim Mathies [:jimm]]

(In reply to comment #54)
> Comment on attachment 458748 [details] [diff] [review]
> frame rendering and metrics
> 
> Looks fine to me, one comment:
> 
> >+      // inset the caption area so it doesn't overflow.
> >+      RECT rect = widgetRect;
> >+      OffsetRect(&rect, 4, 4);
> >+      rect.bottom -= 4;
> >+      rect.right -= 8;
> 
> Where do the magic 4 numbers come from?  Maybe add a comment explaining that. 
> (Also may just want to use rect.top += 4; rect.left += 4; instead of
> OffsetRect, since you're already directly modifying bottom/right.)
> 

Those are based on the window border that gets drawn with DrawEdge. I should be able to pull those from metrics actually, I'll bet they match up to some cx/cy padding values. I'll do some testing to see.

Comment 56

[Jim Mathies [:jimm]]

I think what I'll do here w/the beta coming up is land without the "remove compositor check" and the "disable chrome margins" patches. So the fx button will remain enabled on nightlies. The compositor patch can land with bug 575870 and will be required to do the work there.

Comment 58

[Jim Mathies [:jimm]]

Attachment: button box css

To simplify positioning of the titlebar elements in bug 575870, I've added a -moz-window-button-box style. This doesn't render anything, it's just for positioning of the caption buttons. The box has top and left/right padding set on it.

Comment 59

[Jim Mathies [:jimm]]

Attachment: button box theme code

Additions for the button box styles, and I fixed a caption button size bug in the previous patch for xp. (This is a diff on top of the previous patch.)

Comment 60

[Jim Mathies [:jimm]]

Attachment: button box theme code

Updater per some discussion with dao, there's no need to do any rtl checking here, everything should "just work".

Comment 61

[Markus Stange [:mstange]]

Comment on attachment 458748 [details] [diff] [review]
frame rendering and metrics

>diff --git a/widget/src/windows/nsNativeThemeWin.cpp b/widget/src/windows/nsNativeThemeWin.cpp

>+static PRInt32 GetWindowFrameButtonState(nsIFrame *aFrame, PRInt32 eventState)
>+{
>+  if (GetTopLevelWindowActiveState(aFrame) ==
>+      mozilla::widget::themeconst::FS_INACTIVE)
>+    return mozilla::widget::themeconst::BS_INACTIVE;
>+
>+  if (eventState & NS_EVENT_STATE_ACTIVE)

(eventState & NS_EVENT_STATE_HOVER) && (eventState & NS_EVENT_STATE_ACTIVE)
would be a little more accurate.

>+static PRInt32 GetClassicWindowFrameButtonState(PRInt32 eventState)
>+{
>+  if (eventState & NS_EVENT_STATE_ACTIVE)

Here too.

Comment 62

[Jim Mathies [:jimm]]

(In reply to comment #61)
> Comment on attachment 458748 [details] [diff] [review]
> frame rendering and metrics
> 
> >diff --git a/widget/src/windows/nsNativeThemeWin.cpp b/widget/src/windows/nsNativeThemeWin.cpp
> 
> >+static PRInt32 GetWindowFrameButtonState(nsIFrame *aFrame, PRInt32 eventState)
> >+{
> >+  if (GetTopLevelWindowActiveState(aFrame) ==
> >+      mozilla::widget::themeconst::FS_INACTIVE)
> >+    return mozilla::widget::themeconst::BS_INACTIVE;
> >+
> >+  if (eventState & NS_EVENT_STATE_ACTIVE)
> 
> (eventState & NS_EVENT_STATE_HOVER) && (eventState & NS_EVENT_STATE_ACTIVE)
> would be a little more accurate.
> 
> >+static PRInt32 GetClassicWindowFrameButtonState(PRInt32 eventState)
> >+{
> >+  if (eventState & NS_EVENT_STATE_ACTIVE)
> 
> Here too.

Thanks, added this. This was needed since buttons highlight even though the window is inactive.

Comment 63

[Jim Mathies [:jimm]]

Attachment: theme code updates

- updated the hover state for themed desktops. (classic desktops don't have these so there's no need to check for it.)
- button box code for bug 575870
- fix xp bug for custom sized caption buttons
- moved button size query code into the theme code so that it's called when layout needs the values. this removed the same from nsWindow and made querying for the data more reliable.

I'll fire off some try builds as well.

Comment 64

[Jim Mathies [:jimm]]

Attachment: ncclient area handling v.4

Neil, had to make a couple changes to this after testing:

- nixed the invalidate/validate option for InvalidateNonClientRegion.
- added code that invalidates the non-client areas we want windows to draw. The need for this showed up after testing bug 575870. Since we're only drawing the titlebar, the non-client areas we expected windows to draw weren't getting painted. The new code takes care of invalidating these areas and calls RedrawWindow to force an wm_ncpaint event when we receive wm_ncactivate. This replaces the default behavior of wm_ncactivate, which normally paints these areas.
- added a wm_settext handler, which normally repaints the titlebar.

Comment 65

[Jim Mathies [:jimm]]

Attachment: ncclient area handling v.4

This is a slight improvement on the last patch, I'm combining the entire invalidation area in InvalidateNonClientRegion into a single region and use RedrawWindow to invalidate.

Comment 66

[Jim Mathies [:jimm]]

(In reply to comment #63)
> I'll fire off some try builds as well.

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-719ed62bd8ac/tryserver-win32/

Comment 67

[[e-mail]]

Comment on attachment 462986 [details] [diff] [review]
ncclient area handling v.4

>+nsWindow::InvalidateNonClientRegion()
So, if I've understood this correctly, you want to invalidate the original non-client region, but using the coordinates of the new client region? Then assuming I have my variables straight, you want to take the client rect and deflate it by the non client offsets, which gives you the original client rect, which you then exclude from the window rect (which you obtain by adding the caption/resize margins to the original client rect).

>+    case WM_SETTEXT:
>+      /*
>+       * WM_SETTEXT paints the titlebar area. Avoid this if we have a
>+       * custom titlebar we paint ourselves.
Do we need to invalidate anything?

Comment 68

[Jim Mathies [:jimm]]

(In reply to comment #67)
> Comment on attachment 462986 [details] [diff] [review]
> ncclient area handling v.4
> 
> >+nsWindow::InvalidateNonClientRegion()
> So, if I've understood this correctly, you want to invalidate the original
> non-client region, but using the coordinates of the new client region?

RedrawWindow takes client coordinates. AFAIK it's the only invalidation call you can make that can invalidate specific non-client areas. So I start with the client rect, and expand that out.

> Then assuming I have my variables straight, you want to take the client rect and
> deflate it by the non client offsets, which gives you the original client rect,

Well, technically I *expand* it to include non-client areas we want windows to paint in WM_NCPAINT. 

clientRgn = the client area

winRgn = the client area + the non-client area we want windows to paint.

winRgn - clientRgn = a region equal to the window frame we don't paint. At this point we could call RedrawWindow on this region, which would trigger a WM_NCPAINT event for the frame we want painted.

What follows after that is our normal client area invalidation that covers the areas of the client area we paint non-client chrome on. I've just combined that with winRgn, so I only have to make one invalidation call. 

> >+    case WM_SETTEXT:
> >+      /*
> >+       * WM_SETTEXT paints the titlebar area. Avoid this if we have a
> >+       * custom titlebar we paint ourselves.
>
> Do we need to invalidate anything?

Not at this point, we don't paint any text. Also, I believe the bug requesting we draw text titles someplace in the titlebar was marked won't fix.

Comment 69

[Jim Mathies [:jimm]]

It all falls together like this

windows -> WM_NCACTIVATE -> InvalidateNonClientRegion() ->

(invalidate logic) -> RedrawWindow

[invalid region = non-client frame + client area we paint non-client chrome on]

windows -> WM_NCPAINT -> ExcludeNonClientFromPaintRegion() ->

region = (invalid region - client area) -> DefWndProc(region)

[frame painted]

windows -> WM_PAINT -> OnPaint ->

invalid client area that contains chrome painted.

Comment 70

[[e-mail]]

(In reply to comment #68)
> winRgn - clientRgn = a region equal to the window frame we don't paint. At this
> point we could call RedrawWindow on this region, which would trigger a
> WM_NCPAINT event for the frame we want painted.
> 
> What follows after that is our normal client area invalidation that covers the
> areas of the client area we paint non-client chrome on. I've just combined that
> with winRgn, so I only have to make one invalidation call. 
OK, so I was trying to use the same terms that I used before, but I will try again using your names, assuming I've understood them correctly:

winRgn is the entire window frame. This often includes the borders and caption.
clientRgn is the part of the window we don't want Windows to paint. If we have set nonclient margins then this may include borders and caption that Windows normally paints, but instead those areas become the non-client chrome. Finally inside the non-client chrome is an area presumably called the client chrome.

So my question is, why not subtract the client chrome from the winRgn directly?

> > >+    case WM_SETTEXT:
> > >+      /*
> > >+       * WM_SETTEXT paints the titlebar area. Avoid this if we have a
> > >+       * custom titlebar we paint ourselves.
> > Do we need to invalidate anything?
> Not at this point, we don't paint any text. Also, I believe the bug requesting
> we draw text titles someplace in the titlebar was marked won't fix.
And even if we did, I guess it would be done using existing layout objects.

Comment 71

[[e-mail]]

++-------------------++
|| non-client chrome ||
|+-------------------+| }
||   client chrome   || }
|+-------------------+| }
||      content      || } area we don't want to invalidate
|+-------------------+| }
||   client chrome   || }
|+-------------------+| }
+---------------------+ <- Windows non-client area (winRgn - clientRgn)

Comment 72

[Jim Mathies [:jimm]]

(In reply to comment #70)
> (In reply to comment #68)
> 
> winRgn is the entire window frame.

Yes, although to be compatible with RedrawWindow it's in client coordinates. We could use GetWindowRect and them convert those to client coords I think, but it's easier to just go from client -> window with some simple +/- math.

> This often includes the borders and caption.
> clientRgn is the part of the window we don't want Windows to paint. If we have
> set nonclient margins then this may include borders and caption that Windows
> normally paints, but instead those areas become the non-client chrome. Finally
> inside the non-client chrome is an area presumably called the client chrome.
> 
> So my question is, why not subtract the client chrome from the winRgn directly?

Well that's basically what's happening here. Maybe there's a simpler way to do this that I'm missing?

+-+-----------------------+-+
| | app non-client chrome | |
| +-----------------------+ |
| |   app client chrome   | | }
| +-----------------------+ | }
| |      app content      | | } area we don't want to invalidate
| +-----------------------+ | }
| |   app client chrome   | | }
| +-----------------------+ | 
+---------------------------+ <
 ^                         ^    windows non-client chrome

client area = app *

((client rect + windows non-client chrome) - client rect) + app non-client chrome

clientRgn = client rect
winRgn = (client rect + windows non-client chrome)
winRgn = winRgn - clientRgn
winRgn += windows non-client chrome

winRgn = the inverse of "area we don't want to invalidate"

Comment 73

[Jim Mathies [:jimm]]

correction:

clientRgn = client rect
winRgn = (client rect + windows non-client chrome)
winRgn = winRgn - clientRgn
winRgn += *app* non-client chrome

Comment 74

[Jim Mathies [:jimm]]

We could also start with screen coordinates, but I'm not sure how we get all the regions into the client coordinate space in the end, probably could be done though.

Comment 75

[[e-mail]]

(In reply to comment #72)
> (In reply to comment #70)
> > So my question is, why not subtract the client chrome from the winRgn directly?
> Well that's basically what's happening here. Maybe there's a simpler way to do
> this that I'm missing?
Hopefully the simpler way is to subtract the app client chrome region from the window non-client chrome region. This then automatically includes the app non-client chrome region too.

Oh, and I wasn't trying to suggest screen coordinates. Sorry for any confusion.

Comment 76

[Jim Mathies [:jimm]]

(In reply to comment #75)
> Hopefully the simpler way is to subtract the app client chrome region from the
> window non-client chrome region. This then automatically includes the app
> non-client chrome region too.

Hmm, "app client chrome" or "app non-client chrome"? (I think you meant the latter.)

+-+-----------------------+-+
| | app non-client chrome | |
| +-----------------------+ |
| |   app client chrome   | | }
| +-----------------------+ | }
| |      app content      | | } area we don't want to invalidate
| +-----------------------+ | }
| |   app client chrome   | | }
| +-----------------------+ | 
+---------------------------+ <
 ^                         ^    windows non-client chrome
client area = app *

Comment 77

[Bill Gianopoulos [:WG9s]]

(In reply to comment #66)
> (In reply to comment #63)
> > I'll fire off some try builds as well.
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-719ed62bd8ac/tryserver-win32/

I tested the Tryserver builds and everything seems to work just fine.

I also did builds of mhy own including Linux builds and verified that nothing regressed under Linux as a result of this either.

For anyone building along at home, you also need to include the patch for bug 575870 in additions to the patches on this bug.

Comment 78

[[e-mail]]

(In reply to comment #76)
> (In reply to comment #75)
> > Hopefully the simpler way is to subtract the app client chrome region from the
> > window non-client chrome region. This then automatically includes the app
> > non-client chrome region too.
> Hmm, "app client chrome" or "app non-client chrome"? (I think you meant the
> latter.)
No, you want to invalidate the app non-client chrome. So there's no point subtracting it from the window region, only to add it back again.

Comment 79

[Jim Mathies [:jimm]]

Attachment: ncclient area handling v.4

Alright, deleted everything and started from scratch. Came up with a smaller chunk of code. (Saved our drawing in the comments for posterity's sake!)

Comment 80

[[e-mail]]

Comment on attachment 464074 [details] [diff] [review]
ncclient area handling v.4

>+  GetWindowRect(mWnd, &rect);
>+  MapWindowPoints(NULL, mWnd, (LPPOINT)&rect, 2);
>+  HRGN winRgn = CreateRectRgnIndirect(&rect);
>+  
>+  // Subtract app client chrome and app content leaving
>+  // app non-client chrome in winRgn.
>+  GetClientRect(mWnd, &rect);
>+  rect.top = mNonClientOffset.top;
>+  rect.right = rect.right - mNonClientOffset.right;
>+  rect.bottom = rect.bottom - mNonClientOffset.bottom;
>+  rect.left = mNonClientOffset.left;
[I still think that this rect computes to the same as the mapped window rect calculated above minus the caption/border widths.]

r=me assuming the rest of the patch is the same as last time.

Comment 81

[Jim Mathies [:jimm]]

(In reply to comment #80)
> Comment on attachment 464074 [details] [diff] [review]
> ncclient area handling v.4
> 
> >+  GetWindowRect(mWnd, &rect);
> >+  MapWindowPoints(NULL, mWnd, (LPPOINT)&rect, 2);
> >+  HRGN winRgn = CreateRectRgnIndirect(&rect);
> >+  
> >+  // Subtract app client chrome and app content leaving
> >+  // app non-client chrome in winRgn.
> >+  GetClientRect(mWnd, &rect);
> >+  rect.top = mNonClientOffset.top;
> >+  rect.right = rect.right - mNonClientOffset.right;
> >+  rect.bottom = rect.bottom - mNonClientOffset.bottom;
> >+  rect.left = mNonClientOffset.left;
> [I still think that this rect computes to the same as the mapped window rect
> calculated above minus the caption/border widths.]

This should actually be GetWindowRect, I'm invalidating a bit of the client area I don't need to.

> [I still think that this rect computes to the same as the mapped window rect
> calculated above *minus the caption/border widths.]

*minus area within the client area that contain manually drawn windows chrome.

Comment 82

[Jim Mathies [:jimm]]

Attachment: theme code updates

refreshing post the approval of the patch in bug 575870.

Comment 83

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

Comment on attachment 464093 [details] [diff] [review]
theme code updates


>-static PRBool IsTopLevelMenu(nsIFrame *aFrame)
>+static PRBool IsTopLevelMenu( nsIFrame *aFrame)

bogus space added; otherwise, looks fine

Comment 84

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/05983881f947
http://hg.mozilla.org/mozilla-central/rev/4a345ea809f6
http://hg.mozilla.org/mozilla-central/rev/c051f7df730e
http://hg.mozilla.org/mozilla-central/rev/ff19c2e86695
http://hg.mozilla.org/mozilla-central/rev/5715673e78c2
http://hg.mozilla.org/mozilla-central/rev/34b248dfbf59
http://hg.mozilla.org/mozilla-central/rev/05dde680ade2
http://hg.mozilla.org/mozilla-central/rev/a0d6e4d37273

Comment 85

[alex_mayorga]

Attachment: minimize, maximize, close blue highlight

minimize, maximize, close buttons on Mozilla/5.0 (Windows NT 6.0; rv:2.0b6pre) Gecko/20100910 Firefox/4.0b6pre ID:20100910041829 have a blue highlight that no other windows have when using "Windows Classic" theme.
Is this bug it or fallout from it?