Closed Bug 574454 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Widget: Win32, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta4+

People

(Reporter: jimm, Assigned: jimm)

References

(Depends on 3 open bugs, Blocks 2 open bugs)

Details

Attachments

(10 files, 15 obsolete files)

6.27 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.18 KB, patch
robarnold
: review+
Details | Diff | Splinter Review
4.38 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
25.99 KB, application/zip
Details
5.25 KB, patch
neil
: review+
Details | Diff | Splinter Review
32.98 KB, patch
vlad
: review+
Details | Diff | Splinter Review
3.11 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.04 KB, patch
neil
: review+
Details | Diff | Splinter Review
12.98 KB, patch
vlad
: review+
Details | Diff | Splinter Review
2.69 KB, image/jpeg
Details
Split off from bug 513162. This is basically complete, just needs some testing. Landing post beta1.
(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?
Attached patch bits v.1 (obsolete) — Splinter Review
Duplicate of this bug: 572312
Will this fix bug 575271? If so, can you mark the dependency?
(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.
Blocks: 575870
Depends on: 576960
Blocks: 576960
No longer depends on: 576960
So, this is for Windows XP?
OS: Windows 7 → Windows XP
No, as the drawing in title bar for non Aero enviroments are not yet finsihed.
This bug is for All Windows platforms.
(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.
Jim, is there bug for making title bar fully customizable?
(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.
So users will be able put there buttons?
(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.
Disregard the first part. It seems it will be customizable =)
Blocks: 574833
(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!)
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?
(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.
Attached patch css additions (obsolete) — Splinter Review
Attachment #453843 - Attachment is obsolete: true
Attached patch win theme detection (obsolete) — Splinter Review
Attached patch frame rendering and metrics (obsolete) — Splinter Review
Attached patch css additionsSplinter Review
dbaron, these are the same css changes you gave feedback on in bug 513162, with updated naming.
Attachment #457176 - Attachment is obsolete: true
Attachment #457195 - Flags: review?(dbaron)
Attached patch disable chrome margins (obsolete) — Splinter Review
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.
Attachment #457197 - Flags: review?
Attachment #457197 - Flags: review? → review?(dao)
Comment on attachment 457180 [details] [diff] [review]
remove compositor check

Rob - this is a simple patch that removes the compositor checks in widget.
Attachment #457180 - Flags: review?(tellrob)
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.
Attachment #457177 - Flags: review?(roc)
Attachment #457180 - Flags: review?(tellrob) → review+
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
Attachment #457177 - Flags: review?(roc) → review+
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?
Attachment #457197 - Flags: review?(dao) → review+
Attached patch frame rendering and metrics (obsolete) — Splinter Review
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.
Attachment #457178 - Attachment is obsolete: true
Attachment #457181 - Attachment is obsolete: true
Attachment #457350 - Flags: review?(tellrob)
Attached patch ncclient area handling (obsolete) — Splinter Review
nsWindow code that handles the ncclient area properly for various nc events.
Comment on attachment 457352 [details] [diff] [review]
ncclient area handling

Neil, care to take a crack at reviewing this?
Attachment #457352 - Flags: review?(neil)
Attached patch ncclient area handling (obsolete) — Splinter Review
Small update - removed some debug code and fixed a comment typo.
Attachment #457352 - Attachment is obsolete: true
Attachment #457372 - Flags: review?(neil)
Attachment #457352 - Flags: review?(neil)
Attached file test xulrunner app
This is a basic xul runner app that supports custom frame rendering.
No longer blocks: 576960
Comment on attachment 457195 [details] [diff] [review]
css additions

r=dbaron
Attachment #457195 - Flags: review?(dbaron) → review+
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?
(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.
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.
blocking2.0: --- → betaN+
(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.
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.
(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.
(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.
(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.
Attached patch ncclient area handling v.2 (obsolete) — Splinter Review
Attachment #457372 - Attachment is obsolete: true
Attachment #457372 - Flags: review?(neil)
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.)
Attachment #458684 - Flags: review?(neil)
I should add a caveat, I need to test on winXP first before I proclaim success.
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.]
(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)
Attached patch ncclient area handling v.3 (obsolete) — Splinter Review
Attachment #458684 - Attachment is obsolete: true
Attachment #458736 - Flags: review?(neil)
Attachment #458684 - Flags: review?(neil)
Comment on attachment 458736 [details] [diff] [review]
ncclient area handling v.3

argh, missed one rect = {0}.
Attachment #458736 - Flags: review?(neil) → review-
Attachment #458736 - Attachment is obsolete: true
Attachment #458737 - Flags: review?(neil)
Nixed the nsIntSize for the command buttons, and pre-populated the data with system metrics. Should be a little safer.
Attachment #457350 - Attachment is obsolete: true
Attachment #458748 - Flags: review?(tellrob)
Attachment #457350 - Flags: review?(tellrob)
Attachment #458748 - Flags: review?(vladimir)
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 ;-)
Attachment #458737 - Flags: review?(neil) → review+
(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 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))
Attachment #458748 - Flags: review?(vladimir) → review+
(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.
Attachment #458748 - Flags: review?(tellrob)
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.
blocking2.0: betaN+ → beta4+
Attachment #457197 - Attachment is obsolete: true
Duplicate of this bug: 575271
No longer blocks: 574833
Attached patch button box cssSplinter Review
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.
Attachment #461913 - Flags: review?(dbaron)
Attached patch button box theme code (obsolete) — Splinter Review
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.)
Attachment #461915 - Flags: review?(vladimir)
Attachment #461913 - Flags: review?(dbaron) → review+
Attached patch button box theme code (obsolete) — Splinter Review
Updater per some discussion with dao, there's no need to do any rtl checking here, everything should "just work".
Attachment #461915 - Attachment is obsolete: true
Attachment #462070 - Flags: review?(vladimir)
Attachment #461915 - Flags: review?(vladimir)
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.
Blocks: 583959
(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.
Attached patch theme code updates (obsolete) — Splinter Review
- 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.
Attachment #462070 - Attachment is obsolete: true
Attachment #462866 - Flags: review?(vladimir)
Attachment #462070 - Flags: review?(vladimir)
Attached patch ncclient area handling v.4 (obsolete) — Splinter Review
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.
Attachment #462872 - Flags: review?(neil)
Attached patch ncclient area handling v.4 (obsolete) — Splinter Review
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.
Attachment #462872 - Attachment is obsolete: true
Attachment #462986 - Flags: review?(neil)
Attachment #462872 - Flags: review?(neil)
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?
(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.
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.
(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.
++-------------------++
|| non-client chrome ||
|+-------------------+| }
||   client chrome   || }
|+-------------------+| }
||      content      || } area we don't want to invalidate
|+-------------------+| }
||   client chrome   || }
|+-------------------+| }
+---------------------+ <- Windows non-client area (winRgn - clientRgn)
(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"
correction:

clientRgn = client rect
winRgn = (client rect + windows non-client chrome)
winRgn = winRgn - clientRgn
winRgn += *app* non-client chrome
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.
(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.
(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 *
(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/jmathies@mozilla.com-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.
(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.
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!)
Attachment #462986 - Attachment is obsolete: true
Attachment #464074 - Flags: review?(neil)
Attachment #462986 - Flags: review?(neil)
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.
Attachment #464074 - Flags: review?(neil) → review+
(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.
refreshing post the approval of the patch in bug 575870.
Attachment #462866 - Attachment is obsolete: true
Attachment #464093 - Flags: review?(vladimir)
Attachment #462866 - Flags: review?(vladimir)
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
Attachment #464093 - Flags: review?(vladimir) → review+
Depends on: 585863
Depends on: 585886
Blocks: 557468
Blocks: 585964
Depends on: 586219
Depends on: 586884
Depends on: 586228
Depends on: 586233
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?
Depends on: 608984
Blocks: 610643
No longer blocks: 610643
Depends on: 610643
Depends on: 610335
Depends on: 610916
Depends on: 606160
No longer depends on: 610916
Depends on: 611803
Blocks: 617893
Depends on: 641223
Depends on: 651905
You need to log in before you can comment on or make changes to this bug.