513162 - [Windows] Drawing in the Title Bar for the New Firefox Theme

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

This is the Windows XP version of bug 513157, details on the rational can be found
in bug 513157 comment #0, or below.

For Firefox 4, there are a variety of reasons why we will need to either draw
in the title bar of the application window, or actually draw the title bar
entirely from scratch:

1) Having a control at the edge of the window frame to set the active Weave
account.

2) Drawing a persona throughout all of chrome (including the title bar).

3) In private browsing mode changing the appearance of the theme, including the
title bar.  Interactively this is to reduce the number of mode errors (a
darkened theme is easily viewable using peripheral vision).  On a more
emotional and design level, it just looks really cool.

4) Placing tabs slightly into the area of the title bar will help us free up
space for the content area.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Requesting blocking on 1.9.3 if we want the new Firefox theme in Firefox.next

Comment 3

[Wesley Johnston (:wesj)]

Is this bug for "Drawing in the Title Bar" or drawing our own titlebars with native theme code? I looked into this a bit yesterday. From what I read, Windows XP and Vista/7 without Aero only give you the option of 1.) drawing in the titlebar, or 2.) throwing out the entire window frame and drawing everything (titlebar, min/max/close, and window borders) all by yourself. If its necessary that content overlap the titlebar and content area, they you have to use option 2. Vista/7 with Aero gives you a third option where, if you have called DWMExtendGlassIntoFrame and if the application icon and title aren't shown, content is automatically drawn into the titlebar, and you just have to be careful not to draw where the min/max/close buttons are.

Just curious. If we just draw in the titlebar (all we want is a Firefox button up there), then only a new attribute or xul node is needed to specify that some content should be drawn there, and probably a bunch of code to do hit detection on it. If we draw the entire window frame ourselves, then a whole slew of nodes have to be inserted to support window resizing and whatnot.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

Thing we want to do, in order of potential complexity are:

-Add the Firefox button to replace the window icon
-Add contextual edit controls to the right of the Firefox button based on if a form element has the focus or there is a selection (cut, copy, paste)
----
-Darken all of the glass/title bar when the user is in private browsing mode
-Draw a persona on the title bar and window frame
-(potentially) add a full screen button to the window controls

My impression (although I could definitely be wrong) is that items after the bar are probably only achievable by drawing the title bar and window frame entirely ourselves, along with supporting all of the correct window management functionality.

Comment 5

[Damon Sicore (:damons)]

Blocking 1.9.3 Beta1+, and looking for an owner here.

Comment 6

[Benjamin Smedberg]

Jim is already planning on this, after OOPP finishes up this week or next.

Comment 7

[Peter Henkel [:Terepin]]

From the position of FX fan and future software developer, If I may ask two qeustions:
1. This will allow to draw in Windows's titlebar or it will be recreated custom one?
2. How hard it is?
Thanks for asnwers.

Comment 8

[Jim Mathies [:jimm]]

(In reply to comment #4)
> Thing we want to do, in order of potential complexity are:
> 
> -Add the Firefox button to replace the window icon
> -Add contextual edit controls to the right of the Firefox button based on if a
> form element has the focus or there is a selection (cut, copy, paste)
> ----
> -Darken all of the glass/title bar when the user is in private browsing mode
> -Draw a persona on the title bar and window frame
> -(potentially) add a full screen button to the window controls
> 
> My impression (although I could definitely be wrong) is that items after the
> bar are probably only achievable by drawing the title bar and window frame
> entirely ourselves, along with supporting all of the correct window management
> functionality.

To nix the default controls and titlebar, we'll need to do the drawing ourselves. We already support this for top level windows through eBorderStyle_none. (There might be a sizing border around the window we'd want to remove.) Generally though I think we already have the pieces in place to start experimenting with this on the main window without any changes to widget.

For drawing the background of the window frame and window button controls, we'll need to add a few new theme constants and theme rendering support - 

http://mxr.mozilla.org/mozilla-central/source/gfx/public/nsThemeConstants.h

http://msdn.microsoft.com/en-us/library/bb773210(VS.85).aspx
(see "WINDOW")

Comment 9

[Jim Mathies [:jimm]]

> http://msdn.microsoft.com/en-us/library/bb773210(VS.85).aspx
> (see "WINDOW")

Did a little more reading on this. MSDN has a nice primer on the subject here - 

http://msdn.microsoft.com/en-us/library/bb688195(VS.85).aspx

Using these theme constants on Vista and up with DWM enabled, we'll get the aero basic caption buttons. The DWM apparently handles drawing caption buttons automatically if glass is extended down into the client area, and the non-client area is removed. I think we can still let content render into this portion of the window on DWM enabled systems, we'll just have to be careful about window width and positioning. On XP, we can render themed areas using the theme library.

I think XP will actually be the tougher job. Some experimentation with rendering our own nc client area should give some idea of what's possible.

Back to the original list:

1) Add the Firefox button to replace the window icon
2) Add contextual edit controls to the right of the Firefox
button based on if a form element has the focus or there is
a selection (cut, copy, paste)

On Vista and up, we can remove the nc client area, extend glass down, and render content here. According to MSDN, caption button will be rendered by the DWM in glass.

On XP, we'll need to render the content and the themed background of the non-client area, plus caption buttons using theme rendering apis.

Non-DWM enabled Vista+ desktops will need to degrade down to normal 'XP' style theme rendering.
 
3) Darken all of the glass/title bar when the user is in private browsing mode

I haven't had a chance to look at this but my initial guess is that this will not be possible.

4) Draw a persona on the title bar and window frame

Probably simpler than 'XP' style theme rendering. With DWM desktops, content will need to adjust glass up, and we may have to do our own caption buttons (possibly, this is optimal in that personas will want to customize these buttons?). With non-DWM desktops, we'll already be rendering everything anyway.

5) add a full screen button to the window controls

Positioning when the DWM is rendering the caption buttons will be tricky. With 'XP' style theme rendering mode, this shouldn't be a problem.

Comment 10

[Johnathan Nightingale [:johnath]]

This summary (and the promise of work in progress) brings me much joy, just one point:

(In reply to comment #9)
> 4) Draw a persona on the title bar and window frame
> 
> Probably simpler than 'XP' style theme rendering. With DWM desktops, content
> will need to adjust glass up, and we may have to do our own caption buttons
> (possibly, this is optimal in that personas will want to customize these
> buttons?). With non-DWM desktops, we'll already be rendering everything anyway.

Down the road, the personas concept may well be extended to greater UI customization, and classical themers may also love the idea of modifying the OS elements, but for the work in this bug, I think you should assume that the persona is purely a "background image" for the chrome, and that we should be painting any interactive elements ourselves, sans-customization. Growing scope down the road based on themer demand feels like a much safer approach than assuming it up front.

Comment 11

[Peter Henkel [:Terepin]]

Thanks for all the info. I always enjoy reading such posts/articles. I have questions:
(In reply to comment #9)
> 1) Add the Firefox button to replace the window icon
1) Add the Firefox button to replace the window icon
I'm suggesting two options for that:
   1. Classic button as showed on mockups, when tabs stay in tab bar.
   2. Small version with just FX's logo in it when tabs are in titlebar.
      (I have no idea how are you intend do it, but lets say like Chrome and Opera did)
      2.1 Keep classic appereance in non-maximized window, when tabs are slightly in titlebar (you might and probably will argue that tabs will have less space, but keep in mind that in every none-maximized and for myself I can tell I'm working in non-maximized window only in rare and very specific occasions, like installing add-on from my hard drive. In this case, enough space for tabs are irrelevant for me (note: I don't now where will be new managers created), but I will need easy acces to app menu fro opening add-ons manager. Even if I will need to open specific tab for some reason, I'll use CTRL-TAB function)
      2.2 Small version with just FX's logo in it when windows is maximized).
In Chrome and Opera, transition between max/min are rough. I think som ekind of animation for both tabs and app button would be nice and fit with others animation (with closing/opening tab for example).

> 3) Darken all of the glass/title bar when the user is in private browsing mode
I'm not entirelly sure if ment only to darken glass, or to change it compoletely, but perhaps changing to system's native dark color scheme with transparency disabled?

Comment 12

[Rob Arnold [:robarnold]]

(In reply to comment #9)
> 3) Darken all of the glass/title bar when the user is in private browsing mode
> 
> I haven't had a chance to look at this but my initial guess is that this will
> not be possible.

I think we can approximate it by drawing a semi-transparent black background clipped to the window frame. This is a separate bug however (and my awesomebar is failing to find it).

Comment 13

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm super excited to see this bug get an owner :)

>Probably simpler than 'XP' style theme rendering. With DWM desktops, content
>will need to adjust glass up, and we may have to do our own caption buttons
>(possibly, this is optimal in that personas will want to customize these
>buttons?). With non-DWM desktops, we'll already be rendering everything anyway.

I agree with Johnath that we probably want to just go with the easiest solution at this point, (perhaps unless the ability to customize the window controls with a persona in DWM desktops is very nearly the same amount of work, and changing it later would cause us to have to significantly rearchitect stuff so that it makes more sense overall to have the full capability from the beginning).

Keeping the window controls isn't a big deal though, since in DWM desktops the caption buttons are basically transparent, they should end up matching pretty well with all of the personas.  (less ideal would be to for instance to have XP luna blue style caption controls on top of a bright yellow persona).

>I think we can approximate it by drawing a semi-transparent black background
>clipped to the window frame. This is a separate bug however (and my awesomebar
>is failing to find it).

It's bug 513418, just listing it here to make sure we communicate all the stuff we are interested in doing.  The solution of a semi transparent black background is in fact exactly what we want, especially if it maintains the native glass refraction effect (as opposed to having to draw everything from scratch and losing the refraction effect).

Two things that I forgot to add to the list of things we might be doing in the title bar:

-We might be later adding a button to control which user account is active (Firefox Sync / Weave), doesn't really add any new requirements beyond the current Firefox button.

-We might be placing some controls to the right of the Firefox button (posisbly context sensitive to what has focus) but the more interesting part is that we may also want this area to be user customizable.  Mockup here: http://newdefault.tumblr.com/post/491792968

-We might want to allow the option of having tabs enter into the title bar area, similar to Chrome.

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

Jim: can you give us an update on when we'll be getting this capability?  It's currently blocking a lot of the upcoming theme work and we're on a pretty tight schedule this quarter.

Comment 15

[Jim Mathies [:jimm]]

(In reply to comment #14)
> Jim: can you give us an update on when we'll be getting this capability?  It's
> currently blocking a lot of the upcoming theme work and we're on a pretty tight
> schedule this quarter.

I'll probably get to this in a couple weeks or so. Finishing up 3.7/Win7 integration work at the moment.

Comment 16

[Peter Henkel [:Terepin]]

What integration?

Comment 17

[Robert Strong (they/them - no direct email)]

Jump lists, previews, etc.

Comment 18

[Peter Henkel [:Terepin]]

Cca how long it will take you ti finishe them and how long drawing titlebar? Do you think you will be done with both this month?

Comment 19

[Robert Strong (they/them - no direct email)]

Not sure but Jim might be able to give an estimate. Any particular reason why you want to know?

Comment 20

[Peter Henkel [:Terepin]]

I want to know when I can celebrate. :) This is my fav bugs of all time and I want to see it in action.

Comment 21

[Jim Mathies [:jimm]]

This has moved to the top of the priority list, so I'll be starting in on it presently.

Comment 22

[Biju]

(In reply to comment #4)
> -Add the Firefox button to replace the window icon

Whether double clicking on that icon close the window ?
and single click pull Restore/Move/Size/Min/Max/Close ?
And Alt-Spacebar do same as single click on window icon

Comment 23

[Peter Henkel [:Terepin]]

I don't think you understand what Firefox button is.

Comment 24

[Jim Mathies [:jimm]]

(In reply to comment #22)
> (In reply to comment #4)
> > -Add the Firefox button to replace the window icon
> 
> Whether double clicking on that icon close the window ?
> and single click pull Restore/Move/Size/Min/Max/Close ?
> And Alt-Spacebar do same as single click on window icon

(In reply to comment #23)
> I don't think you understand what Firefox button is.

Those are valid usability questions to be discussed by the folks doing the upper level work. All of those commands are default system behaviors on the standard application icon in the upper left of the frame. With full ncclient rendering in this area the default behaviors will go away.

Comment 25

[d]

(In reply to comment #24)
> (In reply to comment #22)
> > (In reply to comment #4)
> > > -Add the Firefox button to replace the window icon
> > 
> > Whether double clicking on that icon close the window ?
> > and single click pull Restore/Move/Size/Min/Max/Close ?
> > And Alt-Spacebar do same as single click on window icon
> 
> (In reply to comment #23)
> > I don't think you understand what Firefox button is.
> 
> Those are valid usability questions to be discussed by the folks doing the
> upper level work. All of those commands are default system behaviors on the
> standard application icon in the upper left of the frame. With full ncclient
> rendering in this area the default behaviors will go away.

Since the original idea of the App button is based on Microsoft's Ribbon interface, how do they handle it?

Comment 26

[Wesley Johnston (:wesj)]

In both Word 2007 and Word 2010, right clicking anywhere on the titlebar will still bring up the "system" menu, as will Alt-Spacebar. IMO, double-click to close isn't really an accessibility issue. Just a useful little trick that's been lying around since Win... 3.1? But it doesn't work in newer versions of Office if that is an issue.

Comment 27

[Darren Kalck [:D-Kalck]]

(In reply to comment #25)
> Since the original idea of the App button is based on Microsoft's Ribbon
> interface, how do they handle it?
The window icon is still there, the question is : how Google Chrome handle it ?
Clicking on the upper left corner of the window brings the system menu.

Comment 28

[Alex Faaborg [:faaborg] (Firefox UX)]

The system menu will still be available through a right click anywhere on the title bar (including where the window icon used to be, but a Firefox button is now placed).  This is similar to Windows Explorer, where the window icon has been removed but the system menu remains available to users.

Comment 29

[Alex Faaborg [:faaborg] (Firefox UX)]

Also, for users who really want the window icon back, they can get it by enabling the standard menu bar.

Comment 30

[Biju]

(In reply to comment #25)
> Since the original idea of the App button is based on Microsoft's Ribbon
> interface, how do they handle it?

on office 2007, double click close the window.
single click, pull down the menu

> been lying around since Win... 3.1
Probably win 1.0 or even copied from Mac.
Before it was application icon in Win 3.1, 2, 1 and Mac it had an appearance like Television's Power Switch. So when I use to teach people Windows, I tell them it is like the TV power switch. And it will stop application. Sadly later may be in win95 MS added the [x] button (close button) next to Maximize/Restore bottom. This is a trouble because people accidentally start closing window, when they were trying to do Maximize/Restore. Now applications start adding another dialog "Do you really want close?" 

> Just a useful little trick that's
So it is not just a trick, but really the better option than the current popular way of clicking [x] button on the right-top of window.

Also I want add moving mouse left-top is easier than right top for a right handed person. So I always use double clicking on sys-control box to close application.  double click title bar to Maximize/Restore, press escape key to dismiss dialog. Only reason I go window left-top is when I need to minimize and occasionally when I am using mouse only and a dialog pop-up and I need to close it.

Comment 31

[Biju]

(In reply to comment #29)
> Also, for users who really want the window icon back, they can get it by
> enabling the standard menu bar.

I dont care window icon, just add double click to "close application" as Office 2007 do.

Comment 32

[Paul [pwd]]

(In reply to comment #27)
> (In reply to comment #25)
> > Since the original idea of the App button is based on Microsoft's Ribbon
> > interface, how do they handle it?
> The window icon is still there, the question is : how Google Chrome handle it ?
> Clicking on the upper left corner of the window brings the system menu.

Google follow the same behaviour that's been proposed here: Right click anywhere in the title bar and it will bring up a system menu.

Comment 33

[Darren Kalck [:D-Kalck]]

(In reply to comment #32)
> Google follow the same behaviour that's been proposed here: Right click
> anywhere in the title bar and it will bring up a system menu.
That's the behavior of all windows in XP, Vista, Seven. So, no surprises here.

Comment 34

[Biju]

> -Add contextual edit controls to the right of the Firefox button 
> based on if a form element has the focus or there is a 
> selection (cut, copy, paste)

Dont forget DESIGN MODE

> To nix the default controls and titlebar, 
> we'll need to do the drawing ourselves. 

If possible make all those controls XUL 
and allow old style theme to style it.
Sky Pilot theme ( https://addons.mozilla.org/en-US/firefox/addon/76238 )
use to even theme all form controls and scroll bar

> -We might want to allow the option of having tabs enter 
> into the title bar area, similar to Chrome.

Office 2007 allow to place toolbar icons/buttons at Title Bar to save space.

Comment 35

[Jim Mathies [:jimm]]

Alex, will we need the ability to control the size of the non-client area, or for simplicity sake can we start with a simple on/off switch?

Comment 36

[Alex Faaborg [:faaborg] (Firefox UX)]

>will we need the ability to control the size of the non-client area

We can start with a fixed height for the title bar.  The only use case I can think of for needing to adjust this value is if we need to increase the height of the Firefox button because the user has a larger UI font.

Comment 37

[Jim Mathies [:jimm]]

Ok, I've got basic non-client rendering wedged in for testing purposes. From this I'm finding rendering glass and the caption buttons to be pretty easy to do. A few things I have learned - rendering in the upper non-client area using XUL content by extending the client area out, at least on DWM desktops, shouldn’t be an issue (screen cap 1). However, we will have to restrict this to content with the css background property set to transparent. (See screen cap 3 for an example of why.) Also, unfortunately, DWM caption buttons render under our content based on initial tests, so personas background rendering isn't as simple as painting across the top of the window, which is a bummer (screen cap 2).

I think what we should do here is break this down into parts based on os and dwm support. I'd like to start by getting basic support landed for removing the non-client area for DWM desktops on Vista and Win7, which will give the theming folks something to work with for June deadlines. From there we can spin off bugs for XP + Vista/Win7 non-DWM non-client background theme rendering. Hopefully the follow up work won't be too bad. (I spoke with Rob Arnold today on irc, he felt getting the non-dwm, non-client background theme rendering pieced put together would be fairly straight forward, which is good news!)

Looking at the original feature list, thus far:

-Add the Firefox button to replace the window icon

* doable

-Add contextual edit controls to the right of the Firefox button based on if a
form element has the focus or there is a selection (cut, copy, paste)

* doable as long as xul elements use background:transparent

----

-Darken all of the glass/title bar when the user is in private browsing mode

* unknown

-Draw a persona on the title bar and window frame

* tricky if we want to keep default caption buttons with the DWM's nifty button glow. Doable if we replace the default buttons with our own.

-(potentially) add a full screen button to the window controls

* same as above, the solution to reproducing the default caption button glow is currently unknown. (although we've barely scratched the surface on trying to figure it out.)

Comment 38

[Jim Mathies [:jimm]]

Attachment: screen caps

Comment 39

[Jim Mathies [:jimm]]

Here's an update - I had the chance to start in on this last week before getting pulled off on OOPP work. I'm back working on this as of today. There are a few parts to what I'd like to try and do, here's a basic synopsis -

Rendering in the non-client area on windows is pretty straight forward. Through a set of calls and responses to certain windowing messages, you can tell windows where your client area is positioned within the main window chrome. With normal windows, the default settings place the client within the non-client 'chrome' area of the window, and windows takes care of chrome area rendering and functionality, including drawing the window title and icon, displaying the command buttons and their states, resizing functionality, drawing the window's themed chrome, and reacting to user input. If however an app decides to extend the client area out into the chrome area, windows will continue to do what it can to the chrome areas it has control over, and query the app for everything else.

The basic design I'm working on looks like this:

1) Provide an attribute that allow content to specify chrome margins on a top level window.

This is basically the same thing we did in a minimal way for mac, where we have the "drawintitlebar" boolean flag. However since we want to do full texturing of the chrome area for personas, I've expanded this to a set of margins for the entire window.

2) Implement all the underlying piping that makes windows with custom chrome margins work just like normal windows.

This involves responding to a set of events so that features like sizing and command buttons work normally. I've got the basics for this put together.

3) Implement support for a chrome rendering provider.

The idea here is to expose a new interface on a window which allows upper level content to hook into the process of chrome rendering. In psuedo code, something like:

---

chrome-renderer
{
  function getPartDimensions(type) {
    switch(type) {
      case closebutton:
      (return rect)
      case minbutton:
      (return rect)
      ...
    }
  }

  function getPartPosition(type) {
    switch(type) {
      case closebutton:
      (return point)
      ..
    }
  }

  function drawPart(type, state, dimensions, canvas) {
    switch(type) {
      case closebutton:
      (draw to canvas)
      case minbutton:
      (draw to canvas)
      case backgroundsection:
      (draw to canvas)
      ..
    }
  }
}

// top, right, bottom, left, w/-1 == default spacing
window.setAttribute("chromemargins", "0, -1, -1, -1");
window.setChromeRenderer(chrome-renderer);

---

A registered chrome renderer (for personas customization) would be able to control the look and feel of a window's chrome via this callback mechanism. Animated transitions could be supported for things like command button state changes.

There would also be a 'default' chrome renderer implemented down in widget, which would be lightning fast and independent of content. This would handle rendering chrome areas using default windows theming when a custom renderer isn't registered for a window. The Fx button would rely on this default renderer entirely for rendering the titlebar area once the top chrome margin is pushed up to the top of the window frame.

An interesting note regarding glass desktops - the renderer code doesn't have to be 100% complete in order to get a basic Fx button working on desktops that support glass. One of the unique things about DWM/glass desktops is that if content that's pushed up into the titlebar is transparent, that content, glass, and the default Vista/Win7 command buttons can all co-exist with each other in the titlebar. (So the 'default' chrome renderer in this case has little to do.) However on desktops that do not support glass, a full default renderer will be needed as these desktops do not render anything into the extended client area that overlaps the default chrome area of a normal window.

The steps would be:

3.0 - simple support for a chrome renderer in widget
3.1 - 'default' renderer for glass desktops
3.2 - 'default' renderer for non-glass desktops
3.3 - support for personas custom renderers

Currently I'm working on 2 & 3.0, with the goal of finishing up that work plus 3.1, so that the Fx button can be tested. A full chrome renderer for non-glass desktops (3.2) would be implemented after the initial code lands, w/personas custom render (3.3) support after that.

I have however run into a problem that I need to solve to get this to work. Our windowing structure (at least on windows) involves two windows for every top level chrome window. Basically we have an nsXULWindow as a base container frame, w/a child content window resting in the client area of the xul window parent. This is a major problem, in that extending the client area up into the titlebar area causes the child window to overlap the areas of the titlebar that contain the command buttons, making them inoperative on glass desktops. I can't hack these two windows to produce a working model, the child window needs to "go away" and content needs to rest in the top level frame. I'm not quite sure how to do this yet. Changing up the structure of our windowing system looks to be a pita, if not impossible based on the relationships baked into our upper level code. So I'm trying to find something I can do down in widget that will allow the child window to go away without breaking anything higher up. 

Comments welcome before I get too far into this.

Comment 40

[Peter Henkel [:Terepin]]

GO! :D

Comment 41

[Boris Zbarsky [:bzbarsky]]

I assume "window" in the "two windows" sense near the end of comment 39 means HWND or equivalent?

Comment 42

[Jim Mathies [:jimm]]

(In reply to comment #41)
> I assume "window" in the "two windows" sense near the end of comment 39 means
> HWND or equivalent?

Yes, native window (hwnd), and a widget, parented together.

Comment 43

[(no longer active)]

(In reply to comment #39)
> Rendering in the non-client area on windows is pretty straight forward. Through
> a set of calls and responses to certain windowing messages, you can tell
> windows where your client area is positioned within the main window chrome.
> With normal windows, the default settings place the client within the
> non-client 'chrome' area of the window, and windows takes care of chrome area
> rendering and functionality, including drawing the window title and icon,
> displaying the command buttons and their states, resizing functionality,
> drawing the window's themed chrome, and reacting to user input. If however an
> app decides to extend the client area out into the chrome area, windows will
> continue to do what it can to the chrome areas it has control over, and query
> the app for everything else.

I'm not sure what has changed since all the glassiness stuff came to Windows, but back in the day, this wasn't completely true.  I mean, drawing and handling functionality seems to be two very different things.  If you take the drawing matters in your hands, you could return the correct values from WM_NCHITTEST, and Windows will give you all the good functionality for things like the system menu, resizing, maximizing, minimizing, etc. for free (as in beer).

With the introduction of glassiness crap, DwmDefWindowProc enters the picture, and I'm not really sure how it changes things, but I thought I'd mention this anyway.

Comment 44

[Jim Mathies [:jimm]]

(In reply to comment #43)
> (In reply to comment #39)
> > Rendering in the non-client area on windows is pretty straight forward. Through
> > a set of calls and responses to certain windowing messages, you can tell
> > windows where your client area is positioned within the main window chrome.
> > With normal windows, the default settings place the client within the
> > non-client 'chrome' area of the window, and windows takes care of chrome area
> > rendering and functionality, including drawing the window title and icon,
> > displaying the command buttons and their states, resizing functionality,
> > drawing the window's themed chrome, and reacting to user input. If however an
> > app decides to extend the client area out into the chrome area, windows will
> > continue to do what it can to the chrome areas it has control over, and query
> > the app for everything else.
> 
> I'm not sure what has changed since all the glassiness stuff came to Windows,
> but back in the day, this wasn't completely true.  I mean, drawing and handling
> functionality seems to be two very different things.  If you take the drawing
> matters in your hands, you could return the correct values from WM_NCHITTEST,
> and Windows will give you all the good functionality for things like the system
> menu, resizing, maximizing, minimizing, etc. for free (as in beer).
> 
> With the introduction of glassiness crap, DwmDefWindowProc enters the picture,
> and I'm not really sure how it changes things, but I thought I'd mention this
> anyway.

That's exactly what we're relying on - WM_NCCALCSIZE for custom margin setting, WM_NCHITTEST for hit testing the various custom controls we will be drawing via personas, and WM_NCPAINT for rendering.

The use of DwmDefWindowProc allows for the unique mixture of content and glass in the Vista/Win7 titlebar I mentioned, which makes getting this up and running on glass desktops simpler.

Unfortunately, the child window I spoke of overlaps the DWM driven controls, so they display but can't be interacted with. You can shorten the width of the first child so that it doesn't overlap controls, but the child also acts as the main content container, so you end up skrunching all the content as well.

It looks like this:
___________________________
| |               X X X | |
| |                     | | 
| |                     | |
| |
    ^ MozillaWindowClass (document viewer containter)
 ^ MozillaUIWindowClass (nsXULWindow via xpfe/appshell)

Comment 45

[Boris Zbarsky [:bzbarsky]]

Hmm.  Can we make the MozillaWindowClass widget do whatever it is the other one does to let events through?

Or can we use the nsXULWindow's nsIWidget for the root document viewer in the xul app case?

Comment 46

[Jim Mathies [:jimm]]

(In reply to comment #45)
> Hmm.  Can we make the MozillaWindowClass widget do whatever it is the other one
> does to let events through?

Been mucking around with this idea, maybe forwarding the child events up to the parent, or hand them directly to windows dwm routines as if they came from the parent. Haven't had much luck but I'm still playing with it.

> Or can we use the nsXULWindow's nsIWidget for the root document viewer in the
> xul app case?

This was what I was wondering, my knowledge of the document viewer code isn't very 'robust' though (e.g., I have none. :)).  If we piped the widget up from nsXULWindow to DocumentViewerImpl's MakeWindow, would things espode? I have no idea, but it's worth experimenting with.

Comment 47

[Boris Zbarsky [:bzbarsky]]

> If we piped the widget up from nsXULWindow to DocumentViewerImpl's MakeWindow,
> would things espode?

No obviously, no.  That seems like the best thing to try to me.

Comment 48

[Markus Stange [:mstange]]

(In reply to comment #46)
> If we piped the widget up from
> nsXULWindow to DocumentViewerImpl's MakeWindow, would things espode? I have no
> idea, but it's worth experimenting with.

This would need some changes on Mac, but they shouldn't be large since our popup window setup is very similar.
Our nsIWidgets really have two faces: I'll call them "window" face and "view" face. The window part cares about window size, position, title, active state etc. and usually represents real windows. The view part represents a drawing area inside a window and primarily cares about painting and about mouse and keyboard events.
nsXULWindow only needs the window part (eWindowType_toplevel, dialog and sheet), and non-floating nsViews only need the view part (eWindowType_child). Popup window widgets (eWindowType_popup) however are different - they need to provide both window and view functionality.
On Mac, the window and view parts are implemented by two separate classes: nsCocoaWindow and nsChildView, unlike on the other platforms where nsWindow does both. So for popup windows nsCocoaWindow creates its own nsChildView (mPopupContentView) and forwards all view-style calls to it. A popup window's nsChildView in turn ensures that the events it sends correctly point to its parent nsCocoaWindow.

So with your proposed change we'd only have to remove the check for eWindowType_popup and just unconditionally create an nsChildView for every window.

(In reply to comment #45)
> Hmm.  Can we make the MozillaWindowClass widget do whatever it is the other one
> does to let events through?

On Mac I got around a similar problem by just clipping the part of the child widget that extended into the titlebar. That means that it's neither visible in the titlebar nor responds to mouse events there. Drawing in the titlebar is implemented by having the nsCocoaWindow forward the paint events from the titlebar to the clipped child widget.

We'll need to revisit this when implementing mouse event handling in the titlebar, but I think we can just tell OS X to place the widget under the window control buttons, so we won't have the problems you're having on Windows.

Comment 49

[Peter Henkel [:Terepin]]

Ability to put tabs into title bar is still part of the plan?

Comment 50

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

Jimm: ETA?

Comment 51

[Alex Faaborg [:faaborg] (Firefox UX)]

>Ability to put tabs into title bar is still part of the plan?

hopefully it will be fully customizable, but that will come later, first we need to get the Firefox button in there and personas support.

Comment 52

[Jim Mathies [:jimm]]

(In reply to comment #50)
> Jimm: ETA?

I've been working on solving the problem illustrated in comment 44. Shoehorning the use of the parent chrome widget into document viewer wasn't too hard. But when widgets are setup, we hand in an event callback that receives all gui events generated by widget. The underlying xul / web shell window depends on some of these for focus, size, window movement, creation and destruction, to name a few. The dom window also expect to receive these. My hope was that mirroring these events to both consumers would generally work with maybe minor issues with size and position. But the problems have turned out to be more frustrating to solve. I actually have an fx window "working" (as in displaying) but there are a number of issues with areas such as focus that are not acting properly yet. So I have been stepping through these events to figure out where things are going wrong. My hope is that this work will come together here over the next day or so.

Assuming that works out, I'll post the patch for selectively removing the child window (ifdef'd to windows only since the event work I'm doing is primarily in platform code in widget) for feedback and reviews. I'd like to get this done asap so I can get people discussing it.

From there, we're back to the list of things to do in comment 39. I was in the middle of step 2 when this child window problem showed up. Finishing up that work for glass only desktops shouldn't be too much additional work.   

ETA, assuming I can work out these event problems and everybody is happy with this child widget removal patch, I'd like think we can get through step 2 for glass windows by next week. (That's kinda ball park.)

Comment 53

[Jim Mathies [:jimm]]

Attachment: browser top

Just posting status since a lot of folks are curious. I've removed the underlying content widget and have events working between the web shell and the view (mostly). I still have some window sizing issues (which can be seen on the right) to work out.

Most of the changes for this were in the view w/a little work in DocumentViewerImpl. Some event tweaking took place in widget, but I'd like to try and move that out to the view manager maybe so it'll work on all platforms. I should have the child widget removal patch posted here in a day or so.

One open question I have relates to pushing tabs up into the client area along side the fx button. Does anyone have any ideas on how we should expose information on the dimensions of the nc client area minus the command buttons? If we don't provide that to content some how, they'll not know where to stop drawing. I can expose that via widget, or maybe the base window interfaces, or maybe through css? Curious how the content folks would want to consume that.

Comment 54

[Markus Stange [:mstange]]

(In reply to comment #53)
> Does anyone have any ideas on how we should expose
> information on the dimensions of the nc client area minus the command buttons?
> If we don't provide that to content some how, they'll not know where to stop
> drawing. I can expose that via widget, or maybe the base window interfaces, or
> maybe through css? Curious how the content folks would want to consume that.

I was thinking about this when I did the Mac implementation but ended up not caring about it because on Mac the title bar is always 22px high.
One idea I came up with was this: Create a new -moz-appearance type called -moz-windowcontrols-placeholder, return the size of the non-drawable window frame corner in its GetMinimumWidgetSize, and then put a <windowcontrolsplaceholder/> into browser.xul (and set -moz-appearance: -moz-windowcontrols-placeholder on it) in such a way that it will move the content XUL boxes down far enough.

Dão, does that sound like a useful solution for your use case?

Comment 55

[Dão Gottwald [:dao]]

sounds ok

Comment 56

[Wesley Johnston (:wesj)]

Just thinking about this quickly, that would also help on non-aero Windows where Firefox will have to draw the window controls anyway, right? I mean, I'm not exactly clear from this if the plan is to make those XUL elements or not, but if they are:

1.) Aero could apply -moz-appearance rules that draw nothing but fill the space with appropriate sized boxes
2.) Non-Aero could draw in the controls (potentially with separate XUL buttons for min, max, and close and separate -moz-appearance rules to use system theme images/sizes/margins/etc for each one)?

Comment 57

[Jim Mathies [:jimm]]

(In reply to comment #54)
> I was thinking about this when I did the Mac implementation but ended up not
> caring about it because on Mac the title bar is always 22px high.
> One idea I came up with was this: Create a new -moz-appearance type called
> -moz-windowcontrols-placeholder, return the size of the non-drawable window
> frame corner in its GetMinimumWidgetSize, and then put a
> <windowcontrolsplaceholder/> into browser.xul (and set -moz-appearance:
> -moz-windowcontrols-placeholder on it) in such a way that it will move the
> content XUL boxes down far enough.

For aero, we wouldn't have control over button placement, so this would have to be positioned correctly and wouldn't paint anything. Is that going to be a problem?
  
(In reply to comment #56)
> Just thinking about this quickly, that would also help on non-aero Windows
> where Firefox will have to draw the window controls anyway, right?

Yes. For basic themes and for xp, receding margins basically carves a square chunk out of the theme rendering and removes everything.

On aero, we don't control the button placement or size, on non-aero we can control placement but I don't believe we can control the size if we use the default button bitmaps.

Comment 58

[Jim Mathies [:jimm]]

Attachment: browser top non-aero

For example

Comment 59

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Mockup of maximized window

This will be a follow up bug, but just to make sure everyone is on the same page, here is a mockup of how we would like to render the tab strip when the window is maximized.

Comment 60

[Peter Henkel [:Terepin]]

The "follow up bug" already exists. And not only one...

Comment 61

[Jim Mathies [:jimm]]

We'll need bug 555081 fixed in unison with this, otherwise we won't be able to move the window around in nightly builds.

Comment 62

[Alex Faaborg [:faaborg] (Firefox UX)]

>The "follow up bug" already exists. And not only one...

I actually can't find one for the specific case of moving the tabs all the way to the edge just when the window is maximized (although I only spent a few minutes looking).  Filed bug 572480, please dupe and move the mockup over if there is an existing bug.

Comment 63

[Peter Henkel [:Terepin]]

Bug 572160 - Add option for placing tabs into Title Bar

Comment 64

[Jim Mathies [:jimm]]

Attachment: recycle widgets v.1

Comment 65

[Jim Mathies [:jimm]]

Comment on attachment 451667 [details] [diff] [review]
recycle widgets v.1

This is failing tests due to a problem with popup windows.

Comment 66

[Jim Mathies [:jimm]]

Attachment: initial cleanup

- superficial cleanup of dead code

Comment 67

[Jim Mathies [:jimm]]

Attachment: fix titlebar test fail

This fixes a test failure that always fails for me on debug builds and on try. The focus event from the caret test leaks over into the titlebar test causing it to fail.

Comment 68

[Jim Mathies [:jimm]]

Attachment: widget setup part1

This sets up widget for event mirroring for attached views and fixes some problems in win's GetBounds for child window with chromed parents.

Comment 69

[Jim Mathies [:jimm]]

Attachment: view and view manager

Adds support for attaching views to top level widgets, and addresses various issues with coordinates. View and view manager have never dealt with chromed widgets before, since the content child was always the first widget to be associated with a view and managed by view manager.

Comment 70

[Jim Mathies [:jimm]]

Attachment: document to widget glue

Glue needed to connect document view impl. to a existing top level, chromed widget.

Comment 71

[Jim Mathies [:jimm]]

Attachment: widget tests

Adding a test in toolkit that confirms event client coordinates calculated by sub views offsets are correct.

Comment 72

[Jim Mathies [:jimm]]

Attachment: chromemargin attrib

Adds basic support for top, right, bottom, left margin values in nsAttrValue. Subsequent patches will use this to set chrome margins.

Comment 73

[Jim Mathies [:jimm]]

Attachment: chrome margins

Implement chromemargin attribute on XUL windows.

Comment 74

[Jim Mathies [:jimm]]

Attachment: widget setup part2

Add widget support for setting/getting chrome margin values.

Comment 75

[Jim Mathies [:jimm]]

Attachment: win chrome margins

Add support for chrome margins on windows.

Comment 76

[Shawn Wilsher :sdwilsh]

Comment on attachment 452755 [details] [diff] [review]
fix titlebar test fail

>+++ b/toolkit/content/tests/chrome/test_showcaret.xul
>+  otherWindow.removeEventListener("focus", otherWindowFocused, false);
I am pretty sure you actually want to change the first line of this method to removeEventListener instead of its current form of addEventListener.  We add the event listener twice (I think the second time it is ignored).

Comment 77

[Shawn Wilsher :sdwilsh]

Comment on attachment 452761 [details] [diff] [review]
widget tests

I am technically not a peer of toolkit, but I don't think anyone cares about it for these tests.

Please add some more comments in the tests explaining what is going on, and please add the public domain license text to each test file (http://www.mozilla.org/MPL/license-policy.html).

r=sdwilsh with these changes

Comment 78

[Jim Mathies [:jimm]]

(In reply to comment #76)
> (From update of attachment 452755 [details] [diff] [review])
> >+++ b/toolkit/content/tests/chrome/test_showcaret.xul
> >+  otherWindow.removeEventListener("focus", otherWindowFocused, false);
> I am pretty sure you actually want to change the first line of this method to
> removeEventListener instead of its current form of addEventListener.  We add
> the event listener twice (I think the second time it is ignored).

That would explain the spurious event! Will confirm this fixes the problem.

Comment 79

[Jim Mathies [:jimm]]

Attachment: fix titlebar test fail

That worked.

Comment 80

[Jim Mathies [:jimm]]

Attachment: view and view manager

Removed some commented out cruft.

Comment 81

[Shawn Wilsher :sdwilsh]

Comment on attachment 452784 [details] [diff] [review]
fix titlebar test fail

r=sdwilsh

Comment 82

[Timothy Nikkel (:tnikkel)]

Comment on attachment 452757 [details] [diff] [review]
widget setup part1

>+NS_METHOD nsWindow::GetClientOffset(nsIntPoint &aPt)
>+  POINT pt = {r2.top, r2.left};

Is this backwards?

Comment 83

[Jim Mathies [:jimm]]

(In reply to comment #82)
> (From update of attachment 452757 [details] [diff] [review])
> >+NS_METHOD nsWindow::GetClientOffset(nsIntPoint &aPt)
> >+  POINT pt = {r2.top, r2.left};
> 
> Is this backwards?

Hmm, actually it is, but it will make no difference, since the client rect origin will always be zero. Nice catch though, I can remove that call completely and just use 0,0.

Comment 84

[Jim Mathies [:jimm]]

(In reply to comment #83)
> (In reply to comment #82)
> > (From update of attachment 452757 [details] [diff] [review] [details])
> > >+NS_METHOD nsWindow::GetClientOffset(nsIntPoint &aPt)
> > >+  POINT pt = {r2.top, r2.left};
> > 
> > Is this backwards?
> 
> Hmm, actually it is, but it will make no difference, since the client rect
> origin will always be zero. Nice catch though, I can remove that call
> completely and just use 0,0.

I've updated this to use WidgetToScreenOffset() as well:

RECT r1;
GetWindowRect(mWnd, &r1);
nsIntPoint pt = WidgetToScreenOffset();
aPt.x = pt.x - r1.left; 
aPt.y = pt.y - r1.top;

Comment 85

[Jim Mathies [:jimm]]

Attachment: command button issue in window

A recap of the original feature list, based on what I've learned: 

-Add the Firefox button to replace the window icon

* working nicely on glass. Need window frame theme rendering parts for non-glass desktops. (currently in the works)

-Add contextual edit controls to the right of the Firefox button based on if a
form element has the focus or there is a selection (cut, copy, paste)

* working, just add them next to the fx button

-Draw a persona on the title bar and window frame

For non-glass, this will be possible. For glass desktops we have a pretty big problem in that command buttons and the associated button glow effects are drawn under the window's content. See attached screen grab.

-Darken all of the glass/title bar when the user is in private browsing mode

* See above, same issue.

-(potentially) add a full screen button to the window controls

* non-glass, no problem. glass, button glow effects present a problem again.

Comment 86

[Stephen Horlander [:shorlander]]

(In reply to comment #85)
> -Draw a persona on the title bar and window frame
> 
> For non-glass, this will be possible. For glass desktops we have a pretty big
> problem in that command buttons and the associated button glow effects are
> drawn under the window's content. See attached screen grab.
> 
> -Darken all of the glass/title bar when the user is in private browsing mode
> 
> * See above, same issue.

In an effort to make things more difficult for you, could native buttons be used unless either of those criteria is true? But if they are true use custom buttons?

If not I would be ok with loosing the glow and using custom buttons all the time.

Comment 87

[Jim Mathies [:jimm]]

(In reply to comment #86)
> (In reply to comment #85)
> > -Draw a persona on the title bar and window frame
> > 
> > For non-glass, this will be possible. For glass desktops we have a pretty big
> > problem in that command buttons and the associated button glow effects are
> > drawn under the window's content. See attached screen grab.
> > 
> > -Darken all of the glass/title bar when the user is in private browsing mode
> > 
> > * See above, same issue.
> 
> In an effort to make things more difficult for you, could native buttons be
> used unless either of those criteria is true? But if they are true use custom
> buttons?
> 
> If not I would be ok with loosing the glow and using custom buttons all the
> time.

We have the css media query -moz-windows-compositor, which could be used to detect glass, and content knows the privacy / persona state. Removing the default caption buttons can be handled via window style properties passed when the widget is created. (Not sure if those are settable via content but if not they could be.) So yes, under certain circumstances the default buttons could be removed and custom versions added to rendered content, just like the fx button.

Comment 88

[Jim Mathies [:jimm]]

(In reply to comment #87)
> (In reply to comment #86)
> > (In reply to comment #85)
> > > -Draw a persona on the title bar and window frame
> > > 
> > > For non-glass, this will be possible. For glass desktops we have a pretty big
> > > problem in that command buttons and the associated button glow effects are
> > > drawn under the window's content. See attached screen grab.
> > > 
> > > -Darken all of the glass/title bar when the user is in private browsing mode
> > > 
> > > * See above, same issue.
> > 
> > In an effort to make things more difficult for you, could native buttons be
> > used unless either of those criteria is true? But if they are true use custom
> > buttons?
> > 
> > If not I would be ok with loosing the glow and using custom buttons all the
> > time.
> 
> We have the css media query -moz-windows-compositor, which could be used to
> detect glass, and content knows the privacy / persona state. Removing the
> default caption buttons can be handled via window style properties passed when
> the widget is created. (Not sure if those are settable via content but if not
> they could be.) So yes, under certain circumstances the default buttons could
> be removed and custom versions added to rendered content, just like the fx
> button.

Poking around, looks like all we have in widget is HideWindowChrome, so for updating the visibility of the the default caption buttons we'd have to add some code. Maybe through -moz-appearance?

Comment 89

[Jim Mathies [:jimm]]

Attachment: widget setup part1

Neil, do you have the time for some reviews?

Comment 90

[Jim Mathies [:jimm]]

Comment on attachment 452789 [details] [diff] [review]
view and view manager

dbaron, would you be a good reviewee for view changes? (roc is busy with retained layers)

Comment 91

[Jim Mathies [:jimm]]

Attachment: document to widget glue

Splitting this patch up into just document viewer changes and another widget patch for events.

Comment 92

[Jim Mathies [:jimm]]

Attachment: widget events

Comment 93

[Cheng Khoon]

FYI, Skype draws custom command buttons on glass with glow. Probably worth investigating how it is being done.

Comment 94

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Skype's buttons definitely don't look native to me.

Comment 95

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

(In reply to comment #90)
> (From update of attachment 452789 [details] [diff] [review])
> dbaron, would you be a good reviewee for view changes? (roc is busy with
> retained layers)

I could try.  Could you explain in a few sentences what the changes do, and how they fit with the rest of what you're doing?

Comment 96

[Jim Mathies [:jimm]]

(In reply to comment #95)
> (In reply to comment #90)
> > (From update of attachment 452789 [details] [diff] [review] [details])
> > dbaron, would you be a good reviewee for view changes? (roc is busy with
> > retained layers)
> 
> I could try.  Could you explain in a few sentences what the changes do, and how
> they fit with the rest of what you're doing?

Top level widgets have always been isolated from content by a borderless child that fits in the client area. These changes allow for the removal of that child window and instead use the parent widget in it's place. Instead of creating a new child window via nsDocumentViewerImpl, we create a view and tell it to attach itself to a widget (mParentWidget in nsDocumentViewer). 

mParentWidget is a nsXULWindow, and is created by app shell when the window is created. App shell sets an event callback on the widget to listen to certain key events (close, size, etc.). These have to be sent, while the attached view also needs to receive copies of these events and others.

The changes to nsView facilitate connecting the view to the widget, registering a secondary even handler on the widget to receive events, and handle view tear down (the widget should not be destroyed). View manager changes are mostly cosmetic. Some commenting has also been added to clarify coordinate values. Generally speaking the view and the event state manager need to continue to think of this window as a borderless child, since everything underneath expects it as such.

Comment 97

[Jim Mathies [:jimm]]

(In reply to comment #94)
> Skype's buttons definitely don't look native to me.

Curious, do they display glow above the window frame?

Comment 98

[Cheng Khoon]

(In reply to comment #97)
> (In reply to comment #94)
> > Skype's buttons definitely don't look native to me.
> 
> Curious, do they display glow above the window frame?

Yes, the glow extends above the window frame. Not sure how they do it though.

One possibility is to draw the native buttons, followed by another pixel-perfect custom button on top. This will preserve the glow and allow the private browsing/persona effect, but it is essentially a hack...

Comment 99

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

Olly, Neil, David, Roc, bz: could you please expedite reviews on this bug? It's one of the last hard b1 blockers we have.

Comment 100

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

Comment on attachment 452754 [details] [diff] [review]
initial cleanup

(Bailing roc out a bit here)

Comment 101

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 452763 [details] [diff] [review]
chromemargin attrib

>@@ -419,16 +441,33 @@ nsAttrValue::ToString(nsAString& aResult
> #endif
>     case eFloatValue:
>     {
>       nsAutoString str;
>       str.AppendFloat(GetFloatValue());
>       aResult = str;
>       break;
>     }
>+    case eIntMarginValue:
>+    {
>+      aResult.Truncate();
>+      nsAutoString str;
>+      nsIntMargin tmp;
>+      GetIntMarginValue(tmp);
>+      // top, right, bottom, left
>+      str.AppendInt(tmp.top);
>+      str.Append(PRUnichar(','));
>+      str.AppendInt(tmp.right);
>+      str.Append(PRUnichar(','));
>+      str.AppendInt(tmp.bottom);
>+      str.Append(PRUnichar(','));
>+      str.AppendInt(tmp.left);
>+      aResult = str;
>+      break;
>+    }
Do you really need this?
The string value should be stored in the misc container.

Please add some tests for the margin parsing.

Comment 102

[Olli Pettay [:smaug][[e-mail]]]

Please test cases which fail to parse.

Comment 103

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 452763 [details] [diff] [review]
chromemargin attrib

r- because of missing tests

Comment 104

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 452764 [details] [diff] [review]
chrome margins


>+nsXULWindow::ParseMarginString(nsAutoString& aString,
>+                               nsIntMargin& margins)
>+{
>+  nsAutoString marginStr(aString);
>+  marginStr.CompressWhitespace(PR_TRUE, PR_TRUE);
>+  if (marginStr.IsEmpty()) {
>+    return PR_FALSE;
>+  }
>+
>+  PRInt32 start = 0, end = 0;
>+  for (int count = 0; count < 4; count++) {
>+    if (end >= marginStr.Length())
>+      return PR_FALSE;
>+
>+    // top, right, bottom, left
>+    if (count < 3)
>+      end = Substring(marginStr, start).FindChar(',');
>+    else
>+      end = Substring(marginStr, start).Length();
>+
>+    if (end <= 0)
>+      return PR_FALSE;
>+
>+    PRInt32 errorCode;
>+    PRInt32 value;
>+    nsAutoString clip;
>+    PRInt32 appPerDev = AppUnitsPerDevPixel();
>+
>+    clip = Substring(marginStr, start, end);
>+    value = clip.ToInteger(&errorCode);
>+    if (NS_FAILED(errorCode))
>+      return PR_FALSE;
>+    switch(count) {
>+      case 0:
>+        margins.top = CSSToDevPixels(value, appPerDev);
>+      break;
>+      case 1:
>+        margins.right = CSSToDevPixels(value, appPerDev);
>+      break;
>+      case 2:
>+        margins.bottom = CSSToDevPixels(value, appPerDev);
>+      break;
>+      case 3:
>+        margins.left = CSSToDevPixels(value, appPerDev);
>+      break;
>+    }
>+    start += end + 1;
>+  }
>+  return PR_TRUE;
>+}
>+

Would it be possible to share the parsing logic. Perhaps put a method to nsIContentUtils and nsContentUtils?

Comment 105

[Jim Mathies [:jimm]]

Attachment: css frame rendering

This is a first rev, and works fine without the other patches here. A couple notes:

- frameleft, frameright: min-width is set to the default frame width. Note, sometimes there is a min-height on these parts. (92px on aero basic) 

- caption, captionmax: min-height is valid

- buttons: min-width, min-height are valid, states work

- windows class drawing is currently not implemented (and may never be, is there a point?)

Open question - window frames have different appearances depending on the active state of the window. State change needs to trigger on all the frame elements when the window focus changes, either through something in the content or under the hood. I'm not sure yet how best to handle this.

Comment 106

[Jim Mathies [:jimm]]

* "windows class" -> "windows classic"

Comment 107

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

Comment on attachment 452927 [details] [diff] [review]
widget setup part1

Looks good, one nit:

>+// Attach a view to our widget which we'll send events to. 
>+NS_IMETHODIMP
>+nsBaseWidget::AttachViewToTopLevel(EVENT_CALLBACK aViewEventFunction,
>+                                   nsIDeviceContext *aContext)
>+{
>+  NS_ASSERTION((mWindowType == eWindowType_toplevel), "Can't attach to child?");

This assertion is really "Trying to attach to child window", not "Can't attach to child?" -- that is, we're not failing attaching to a child, we should just never do that.

Comment 108

[Jim Mathies [:jimm]]

Attachment: chromemargin attrib

- updated to use content utils

Comment 109

[Jim Mathies [:jimm]]

Attachment: chrome margins

- ditto

Comment 110

[Jim Mathies [:jimm]]

Attachment: chromemargin attrib tests

Ended up having to put this in widget, due to link issues.

Comment 111

[Jim Mathies [:jimm]]

Attachment: widget setup part2

this is the second set of widget interface changes for chrome margin support. (windows code is in "win chrome margins".)

Comment 112

[Jim Mathies [:jimm]]

Attachment: win chrome margins

Rob, do you currently have any time for reviews? If not please let me know.

Comment 113

[Jim Mathies [:jimm]]

Attachment: win chrome margins

Comment 114

[Rob Arnold [:robarnold]]

Comment on attachment 453287 [details] [diff] [review]
win chrome margins

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -82,16 +82,17 @@
> #endif
>         titlemenuseparator="&mainWindow.titlemodifiermenuseparator;"
>         lightweightthemes="true"
>         lightweightthemesfooter="browser-bottombox"
>         windowtype="navigator:browser"
>         screenX="4" screenY="4"
>         browsingmode="normal"
>         toggletoolbar="true"
>+        chromemargin="-1,-1,-1,-1"

Does this need to be set? Isn't it the default?

>     /**
>-     * Sets the non-client area dimensions of the window. Pass -1 for any
>-     * value to set or restore default desktop setting.
>+     * Sets the non-client area dimensions of the window. Pass -1 to restore
>+     * the system default frame size for that border. Pass zero to remove
>+     * a border, or pass a specific value adjust a border.
>      *
>      * Platform notes:
>      *  Windows: shrinking top non-client height will remove application
>-     *  icon and window title text.
>+     *  icon and window title text. Glass desktops will refuse to set
>+     *  dimensions between zero and size < system default.

Is the bit about 0 valid for other platforms that implement this API? What is the system default and does it change with DPI scaling? Can it be queried?

>+  mDisplayPanFeedback   = PR_FALSE;
>+  mCustomNonClient      = PR_FALSE;
>+  mHideChrome           = PR_FALSE;
>   mWindowType           = eWindowType_child;


>+void
>+nsWindow::SetDrawsInTitlebar(PRBool aState)
>+{
>+  nsWindow * window = GetTopLevelWindow(PR_TRUE);
>+  if (window && window != this) {
>+    return window->SetDrawsInTitlebar(aState);
>+  }
>+
>+  if (aState) {
>+     // left, top, right, bottom
>+    nsIntMargin margins(0, -1, -1, -1);
>+    SetNonClientMargins(margins);
>+  }

Per spec & comment here, we're removing the left border?

>+NS_IMETHODIMP
>+nsWindow::GetNonClientMargins(nsIntMargin &margins)
>+{
>+  nsWindow * window = GetTopLevelWindow(PR_TRUE);
>+  if (window && window != this) {
>+    return window->GetNonClientMargins(margins);
>+  }
>+
>+  if (mCustomNonClient) {
>+    margins = mNonClientOffset;
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsILookAndFeel> lookAndFeel =
>+    do_GetService("@mozilla.org/widget/lookandfeel;1");
>+  if (lookAndFeel) {
>+    PRInt32 val;
>+    lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowTitleHeight, val);
>+    margins.top = val;
>+    lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowBorderHeight, val);
>+    margins.top += val;
>+    margins.bottom = val;
>+    lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowBorderWidth, val); 
>+    margins.left = margins.right = val;
>+    return NS_OK;
>+  }
>+
>+  return NS_ERROR_FAILURE;
>+}

I hope this isn't called often - I don't want to go through the XPCOM lookup every time. Aren't these just calling GetSystemMetric(SM_C{X|Y}BORDER | SM_CYCAPTION, ...)? I'm not a huge fan of the indirection and if the L&F service isn't available we have bigger problems - how about an assert?

>+NS_IMETHODIMP
>+nsWindow::SetNonClientMargins(nsIntMargin &margins)
>...
>+  nsCOMPtr<nsILookAndFeel> lookAndFeel =
>+    do_GetService("@mozilla.org/widget/lookandfeel;1");

Same comment as above.

>+  if (!lookAndFeel)
>+    return NS_ERROR_NOT_AVAILABLE;
>+
>+  // Used in hit testing, provides the resize cursor margin
>+  lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowBorderWidth, mResizeMargin);
>+  lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowTitleHeight, mCaptionHeight);
>+  mCaptionHeight += mResizeMargin;
>+
>+  // XXX fullscreen windows seem to do something different, fixme

I don't think we can be in fullscreen - mHideChrome would be true in that case, right?

>+  mNonClientOffset.top = mNonClientOffset.bottom =
>+    mNonClientOffset.left = mNonClientOffset.right = 0;
>+
>+  // If a margin value is 0, set the offset to the default size of the frame.
>+  // If a margin is -1, leave as default, and if a margin > 0, set the offset
>+  // so that the frame size is equal to the margin value.
>+  if (!margins.top)
>+    mNonClientOffset.top = mCaptionHeight;
>+  else if (margins.top > 0)
>+    mNonClientOffset.top = mCaptionHeight - margins.top;
>+
>+  if (!margins.left)
>+    mNonClientOffset.left = mResizeMargin;
>+  else if (margins.left > 0)
>+    mNonClientOffset.left = mResizeMargin - margins.left;
>+
>+  if (!margins.right)
>+    mNonClientOffset.right = mResizeMargin;
>+  else if (margins.right > 0)
>+    mNonClientOffset.right = mResizeMargin - margins.right;
>+
>+  PRInt32 val;
>+  lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowBorderHeight, val);
>+  if (!margins.bottom)
>+    mNonClientOffset.bottom = val;
>+  else if (margins.bottom > 0)
>+    mNonClientOffset.bottom = val - margins.bottom;

Why is bottom different than left and right? This seems inconsistent with the hit testing.

>@@ -2194,27 +2344,30 @@ NS_IMETHODIMP nsWindow::HideWindowChrome
>   if (aShouldHide) {
>+    mHideChrome = PR_TRUE;
>...
>   }
>   else {
>+    mHideChrome = PR_FALSE;
>...
>   }

mHideChrome = aShouldHide; ?

>+      // hit return constants:
>+      // HTBORDER                     - non-resizable border
>+      // HTBOTTOM, HTLEFT, HTRIGHT, HTTOP - resizable border
>+      // HTBOTTOMLEFT, HTBOTTOMRIGHT  - resizable corner
>+      // HTTOPLEFT, HTTOPRIGHT        - resizable corner
>+      // HTCAPTION                    - general title bar area
>+      // HTCLIENT                     - area considered the client
>+      // HTCLOSE                      - hovering over the close button
>+      // HTMAXBUTTON                  - maximize button
>+      // HTMINBUTTON                  - minimize button
>+
>+      PRInt32 testResult = HTCLIENT;
>+
>+      // non-resizable windows
>+      if (mBorderStyle == eBorderStyle_none) {
>+        if (my > (winRect.top + mResizeMargin) && my <=
>+            (winRect.top + (mCaptionHeight - mNonClientOffset.top)))
>+          testResult = HTCAPTION;
>+        *aRetValue = testResult;
>+        result = PR_TRUE;
>+        break;
>+      }
>+
>+      // resizable windows
>+      PRBool top    = PR_FALSE;
>+      PRBool bottom = PR_FALSE;
>+      PRBool left   = PR_FALSE;
>+      PRBool right  = PR_FALSE;
>+
>+      if (my >= winRect.top && my <=
>+          (winRect.top + (mCaptionHeight - mNonClientOffset.top)))
>+        top = PR_TRUE;
>+      else if (my <= winRect.bottom && my >= (winRect.bottom - mResizeMargin))
>+        bottom = PR_TRUE;
>+
>+      if (mx >= winRect.left && mx <= (winRect.left + mResizeMargin))
>+        left = PR_TRUE;
>+      else if (mx <= winRect.right && mx >= (winRect.right - mResizeMargin))
>+        right = PR_TRUE;
>+
>+      if (top) {
>+        testResult = HTTOP;
>+        if (left)
>+          testResult = HTTOPLEFT;
>+        else
>+        if (right)
>+          testResult = HTTOPRIGHT;
>+      }
>+      else if (bottom) {
>+        testResult = HTBOTTOM;
>+        if (left)
>+          testResult = HTBOTTOMLEFT;
>+        else
>+        if (right)
>+          testResult = HTBOTTOMRIGHT;
>+      }
>+      else {
>+        if (left)
>+          testResult = HTLEFT;
>+        if (right)
>+          testResult = HTRIGHT;
>+      }

Your bracing style is rather strange here and doesn't fit into either of the standard styles.

>@@ -7025,20 +7322,18 @@ void nsWindow::SetWindowTranslucencyInne
>     case eTransparencyGlass:
>       topWindow->mTransparencyMode = aMode;
>       break;
>   }
> 
>   style |= topWindow->WindowStyle();
>   exStyle |= topWindow->WindowExStyle();
> 
>-  if (aMode == eTransparencyTransparent) {
>-    style &= ~(WS_CAPTION | WS_THICKFRAME | WS_SYSMENU | WS_MINIMIZEBOX | WS_MAXIMIZEBOX);
>-    exStyle &= ~(WS_EX_DLGMODALFRAME | WS_EX_WINDOWEDGE | WS_EX_CLIENTEDGE | WS_EX_STATICEDGE);
>-  }
>+  if (aMode == eTransparencyTransparent)
>+    HideWindowChrome(PR_TRUE);

This makes it so much clearer what's going on - thanks! But since HideWindowChrome now sets mHideChrome, I think you need to make sure it gets unset when switching from transparent windows to opaque or glass ones.

>@@ -445,16 +448,21 @@ protected:
>   DWORD_PTR             mOldExStyle;
>   HIMC                  mOldIMC;
>   PRUint32              mIMEEnabled;
>   nsNativeDragTarget*   mNativeDragTarget;
>   HKL                   mLastKeyboardLayout;
>   nsPopupType           mPopupType;
>   PRPackedBool          mDisplayPanFeedback;
>   WindowHook            mWindowHook;
>+  nsIntMargin           mNonClientOffset;
>+  PRBool                mCustomNonClient;
>+  PRPackedBool          mHideChrome;
>+  PRInt32               mResizeMargin;
>+  PRInt32               mCaptionHeight;

I would like simple one-line comments for these describing what they are. Why is mCustomNonClient not a PRPackedBool?

>+void PrintEvent(void* obj, UINT msg, PRBool aShowAllEvents, PRBool aShowMouseMoves)
>+{
>+  int inx = 0;
>+  while (gAllEvents[inx].mId != (long)msg && gAllEvents[inx].mStr != NULL) {
>+    inx++;
>+  }
>+  if (aShowAllEvents || (!aShowAllEvents && gLastEventMsg != (long)msg)) {
>+    if (aShowMouseMoves || (!aShowMouseMoves && msg != 0x0020 && msg != 0x0200 && msg != 0x0084)) {
>+      printf("[%X] - 0x%04X %s\n", obj, msg, gAllEvents[inx].mStr ? gAllEvents[inx].mStr : "Unknown");
>+      gLastEventMsg = msg;
>+    }
>+  }
>+}
>+

Though this is debugging code, I still frown on these hardcoded message numbers. Assuming they're not anonymous, can we give them the right names?

Comment 115

[Jim Mathies [:jimm]]

(In reply to comment #114)
> >+        chromemargin="-1,-1,-1,-1"
> 
> Does this need to be set? Isn't it the default?

This is just a placeholder with the defaults for now. It doesn't need to be set but it makes it easy to find.
 
> 
> >     /**
> >-     * Sets the non-client area dimensions of the window. Pass -1 for any
> >-     * value to set or restore default desktop setting.
> >+     * Sets the non-client area dimensions of the window. Pass -1 to restore
> >+     * the system default frame size for that border. Pass zero to remove
> >+     * a border, or pass a specific value adjust a border.
> >      *
> >      * Platform notes:
> >      *  Windows: shrinking top non-client height will remove application
> >-     *  icon and window title text.
> >+     *  icon and window title text. Glass desktops will refuse to set
> >+     *  dimensions between zero and size < system default.
> 
> Is the bit about 0 valid for other platforms that implement this API? What is
> the system default and does it change with DPI scaling? Can it be queried?

I would think content folks would like the zero trick implemented on other platforms. It saves the step of having to calculate the correct values using system metrics. The current system defaults can be retrieved through GetNonClientMargins(). The value is in pixels and is relative to DPI settings.

> >+     // left, top, right, bottom
> >+    nsIntMargin margins(0, -1, -1, -1);
> >+    SetNonClientMargins(margins);
> >+  }
> 
> Per spec & comment here, we're removing the left border?

Typo, fixed. Added a comment clarifying the difference in order for nsIntMargin from css order.

> 
> >+NS_IMETHODIMP
> >+nsWindow::GetNonClientMargins(nsIntMargin &margins)
> >+{
> >+  nsWindow * window = GetTopLevelWindow(PR_TRUE);
> >+  if (window && window != this) {
> >+    return window->GetNonClientMargins(margins);
> >+  }
> >+
> >+  if (mCustomNonClient) {
> >+    margins = mNonClientOffset;
> >+    return NS_OK;
> >+  }
> >+
> >+  nsCOMPtr<nsILookAndFeel> lookAndFeel =
> >+    do_GetService("@mozilla.org/widget/lookandfeel;1");
> >+  if (lookAndFeel) {
> >+    PRInt32 val;
> >+    lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowTitleHeight, val);
> >+    margins.top = val;
> >+    lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowBorderHeight, val);
> >+    margins.top += val;
> >+    margins.bottom = val;
> >+    lookAndFeel->GetMetric(nsILookAndFeel::eMetric_WindowBorderWidth, val); 
> >+    margins.left = margins.right = val;
> >+    return NS_OK;
> >+  }
> >+
> >+  return NS_ERROR_FAILURE;
> >+}
> 
> I hope this isn't called often - I don't want to go through the XPCOM lookup
> every time. Aren't these just calling GetSystemMetric(SM_C{X|Y}BORDER |
> SM_CYCAPTION, ...)? I'm not a huge fan of the indirection and if the L&F
> service isn't available we have bigger problems - how about an assert?

Generally, you would set this once on creation and not set it again. It might change for some window transitions like full screen depending on what the content folks want to do. But it's not going to change often.

I think an assert is bit heavy, but I don't mind either way. How about a warning? The call does fail, so callers will know something is wrong.

> 
> >+  mCaptionHeight += mResizeMargin;
> >+
> >+  // XXX fullscreen windows seem to do something different, fixme
> 
> I don't think we can be in fullscreen - mHideChrome would be true in that case,
> right?

Good catch. I left that in to investigate that. :) Never got back to it.

> >+  else if (margins.bottom > 0)
> >+    mNonClientOffset.bottom = val - margins.bottom;
> 
> Why is bottom different than left and right? This seems inconsistent with the
> hit testing.

eMetric_WindowBorderHeight returns the lower and upper border height. eMetric_WindowBorderWidth returns left and right border width. I was just being consistent with the metrics code which return SM_CXFRAME and SM_CYFRAME.

> >   }
> >   else {
> >+    mHideChrome = PR_FALSE;
> >...
> >   }
> 
> mHideChrome = aShouldHide; ?

Updated.

> >+      else {
> >+        if (left)
> >+          testResult = HTLEFT;
> >+        if (right)
> >+          testResult = HTRIGHT;
> >+      }
> 
> Your bracing style is rather strange here and doesn't fit into either of the
> standard styles.

if () {
}
else if {
}
else {
}

?

Which braces are you referring to? The if's without braces? 

> 
> >@@ -7025,20 +7322,18 @@ void nsWindow::SetWindowTranslucencyInne
> >     case eTransparencyGlass:
> >       topWindow->mTransparencyMode = aMode;
> >       break;
> >   }
> > 
> >   style |= topWindow->WindowStyle();
> >   exStyle |= topWindow->WindowExStyle();
> > 
> >-  if (aMode == eTransparencyTransparent) {
> >-    style &= ~(WS_CAPTION | WS_THICKFRAME | WS_SYSMENU | WS_MINIMIZEBOX | WS_MAXIMIZEBOX);
> >-    exStyle &= ~(WS_EX_DLGMODALFRAME | WS_EX_WINDOWEDGE | WS_EX_CLIENTEDGE | WS_EX_STATICEDGE);
> >-  }
> >+  if (aMode == eTransparencyTransparent)
> >+    HideWindowChrome(PR_TRUE);
> 
> This makes it so much clearer what's going on - thanks! But since
> HideWindowChrome now sets mHideChrome, I think you need to make sure it gets
> unset when switching from transparent windows to opaque or glass ones.

I update it in SetWindowTranslucencyInner(). Did I miss an appropriate spot?

> >+  nsIntMargin           mNonClientOffset;
> >+  PRBool                mCustomNonClient;
> >+  PRPackedBool          mHideChrome;
> >+  PRInt32               mResizeMargin;
> >+  PRInt32               mCaptionHeight;
> 
> I would like simple one-line comments for these describing what they are. Why
> is mCustomNonClient not a PRPackedBool?

Added.

> 
> >+void PrintEvent(void* obj, UINT msg, PRBool aShowAllEvents, PRBool aShowMouseMoves)
> >+{
> >+  int inx = 0;
> >+  while (gAllEvents[inx].mId != (long)msg && gAllEvents[inx].mStr != NULL) {
> >+    inx++;
> >+  }
> >+  if (aShowAllEvents || (!aShowAllEvents && gLastEventMsg != (long)msg)) {
> >+    if (aShowMouseMoves || (!aShowMouseMoves && msg != 0x0020 && msg != 0x0200 && msg != 0x0084)) {
> >+      printf("[%X] - 0x%04X %s\n", obj, msg, gAllEvents[inx].mStr ? gAllEvents[inx].mStr : "Unknown");
> >+      gLastEventMsg = msg;
> >+    }
> >+  }
> >+}
> >+
> 
> Though this is debugging code, I still frown on these hardcoded message
> numbers. Assuming they're not anonymous, can we give them the right names?

Removed!

Comment 116

[Jim Mathies [:jimm]]

(In reply to comment #115)
> (In reply to comment #114)
> 
> > >+  else if (margins.bottom > 0)
> > >+    mNonClientOffset.bottom = val - margins.bottom;
> > 
> > Why is bottom different than left and right? This seems inconsistent with the
> > hit testing.
> 
> eMetric_WindowBorderHeight returns the lower and upper border height.
> eMetric_WindowBorderWidth returns left and right border width. I was just being
> consistent with the metrics code which return SM_CXFRAME and SM_CYFRAME.
> 

I think I understand the question better - basically, I didn't cache both. I figured it was a waste of a variable. mResizeMargin is used to hit test the border for resizing, so I figured the vert border width would suffice. The other calculations are for actual widths to calculate offsets, so I was more exact in those calculations.

Comment 117

[Jim Mathies [:jimm]]

Attachment: win chrome margins

- updates

Comment 118

[Jim Mathies [:jimm]]

Attachment: full patch

- for felipe

Comment 119

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

dbaron, bz, olli and rob: please let us know if you won't be able to get to review today or not

Comment 120

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 453281 [details] [diff] [review]
chromemargin attrib tests

I'm surprised that you wrote .cpp tests, and not a mochitest
where you would create xul window element and set attribute to it.
But I guess this is ok too.

Seems like you include some Windows only headers, so make sure the 
test runs only on windows, or even better, don't include those headers
and make it so that the test runs everywhere.

Comment 121

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 453280 [details] [diff] [review]
chrome margins

>+nsXULElement::SetChromeMargins(const nsAString* aValue)
>+{
>+    if (!aValue)
>+        return;
>+
>+    nsIWidget* mainWidget = GetWindowWidget();
>+    if (!mainWidget)
>+        return;
>+
>+    // top, right, bottom, left - see nsAttrValue
>+    nsAttrValue attrValue;
>+    nsIntMargin margins;
>+
>+    nsAutoString data;
>+    data.Assign(*aValue);
>+    if (attrValue.ParseIntMarginValue(data) &&
>+        attrValue.GetIntMarginValue(margins)) {
>+        mainWidget->SetNonClientMargins(margins);
>+    }
>+}
Why do you need data variable?
Wouldn't attrValue.ParseIntMarginValue(*data) do the same thing?

Comment 122

[Jim Mathies [:jimm]]

(In reply to comment #120)
> (From update of attachment 453281 [details] [diff] [review])
> I'm surprised that you wrote .cpp tests, and not a mochitest
> where you would create xul window element and set attribute to it.
> But I guess this is ok too.

Possibly something isn't setup right then. When I first played with chrome tests, I would set the value on a top level window to something incorrect, like "asdasd,asdasd,asdasd,asdasd". Then when I requested the value back, I would get that same string. Possibly I need to add special handling in nsXULElement's BeforeSetAttr to parse the value, and and fail if it's formatted incorrectly? It would keep the old value, and script would not know it failed, but I supposed that's expected.

Comment 123

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

Comment on attachment 453154 [details] [diff] [review]
css frame rendering

What feedback did you want on this patch?  Or did you want review?

Does something describe what these different things are?  (I have a good idea for some of them.)

Comment 124

[Jim Mathies [:jimm]]

(In reply to comment #122)
> (In reply to comment #120)
> > (From update of attachment 453281 [details] [diff] [review] [details])
> > I'm surprised that you wrote .cpp tests, and not a mochitest
> > where you would create xul window element and set attribute to it.
> > But I guess this is ok too.
> 
> Possibly something isn't setup right then. When I first played with chrome
> tests, I would set the value on a top level window to something incorrect, like
> "asdasd,asdasd,asdasd,asdasd". Then when I requested the value back, I would
> get that same string. Possibly I need to add special handling in nsXULElement's
> BeforeSetAttr to parse the value, and and fail if it's formatted incorrectly?
> It would keep the old value, and script would not know it failed, but I
> supposed that's expected.

That actually worked out great, and it throws an exception when the wrong param gets passed in. So I think I will stick with it.

Comment 125

[Jim Mathies [:jimm]]

(In reply to comment #123)
> (From update of attachment 453154 [details] [diff] [review])
> What feedback did you want on this patch?  Or did you want review?
> 
> Does something describe what these different things are?  (I have a good idea
> for some of them.)

A review would be great. The values are pretty self explanatory. Maybe some commenting in layout/style/nsCSSProps.cpp would suffice? Re feedback, I was looking for confirmation that spewing -moz-appearance with win specific values for building frames was acceptable. (It works quite well, FWIW.)

Comment 126

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

Comment on attachment 452789 [details] [diff] [review]
view and view manager

In the future, please don't mix unrelated cleanup (un-inlining of
DoSetWindowDimensions) into other patches.

I'm not happy about the "attached" terminology here, since it seems
like the term could easily apply to all views that have widgets, rather
than just only those that were tied to a top-level widget.  Maybe
mAttachedToWidget should be mWidgetIsTopLevel?

GetAttachedView for should probably be declared |static|.  (Any idea
why HandleEvent isn't?  And the new AttachedHandleEvent along with it?)

>+  nsView *view = reinterpret_cast<nsView*>(
>+    GetAttachedViewFor(aEvent->widget));

static_cast, not reinterpret_cast

Or, actually, even better, just change the return type of
GetAttachedViewFor to nsView* (no changes to the code are needed), and
have no cast at all.

Also, seems like GetAttachedWrapperFor shouldn't accept null aWidget.

Both GetAttachedWrapperFor and GetAttachedViewFor should have the normal
flow of control be non-indented, and the unexpected things be indented,
i.e.:
  if (!aWidget)
    return nsnull;
  etc.

Also, GetAttachedWrapperFor is using widget APIs that really look
non-ideal.  I presume they're added in another patch on this bug.  This
code:
>+    void* clientData;
>+    aWidget->GetAttachedViewPtr(clientData);
>+    nsISupports* data = (nsISupports*)clientData;
>+    
>+    if (data) {
>+      ViewWrapper* wrapper;
>+      CallQueryInterface(data, &wrapper);
seems excessive.  The nsIWidget API could deal in a type more useful
than void* -- perhaps even ViewWrapper*.  And please don't add new APIs
that use references rather than pointers for out parameters.  (Or, even
better, use return values rather than out parameters.)

In nsIView::GetViewFor:
>+  ViewWrapper* wrapper = nsnull;
> 
>-  ViewWrapper* wrapper = GetWrapperFor(aWidget);
>+  wrapper = GetWrapperFor(aWidget);

Please leave this as it was.

+  // Child views have no chrome, this is safe.

Maybe s/have no chrome/are not attached to a top-level widget/ , if
that's what it means?  Or does it mean something else?  (Also,
 s/, /, so /.)

>+// Attach to a top level widget and start receiving mirrored events.
>+nsresult nsIView::AttachToTopLevelWidget(nsIWidget* aWidget)
>+{
>+  NS_ENSURE_ARG_POINTER(aWidget);

Can't this be NS_PRECONDITION instead?

>+  // If we are already attached, detach from the old view.

This seems broken.  It would be better to assert that the widget isn't
already attached.  If it *is* already attached, things will break badly
because there's still a view that thinks it's attached, and it'll try to
detach later, and end up detaching some other view.

>+  nsView *oldView = reinterpret_cast<nsView*>(
>+    GetAttachedViewFor(aWidget));

like above, no cast (but I think the code should go away)

AttachToTopLevelWidget should have an NS_PRECONDITION that mWindow
is null.

>+  if (!wrapper)
>+    return NS_ERROR_OUT_OF_MEMORY;

Don't handle null here; the handling looks incorrect anyway, and we
have
http://groups.google.com/group/mozilla.dev.platform/msg/e49e5be28bd526cc
now.

I don't like the way you manage the asymmetry between
nsIView::AttachToTopLevelWidget and nsIView::DetachWidget .  In
particular:
 * DetachWidget should probably be called DetachFromTopLevelWidget,
   if that's what it does
 * DetachFromTopLevelWidget should either:
   (a) not take a widget argument (it's currently unused, except for
   null-checking it)
   (b) assert that the argument given is the widget currently attached
   to (i.e., mWindow == aWidget)

It also seems like making DetachWidget just silently return NS_OK when
!mAttachedToWidget could lead to hidden bugs.

I'm really not in a good position to review the comment changes at
the end of nsView.cpp and in nsViewManager.cpp, though I tend to think
some of them might add rather than resolve confusion.

I'd particularly suggest removing:
>+          // Usually the width x height of the entire child window.


r=dbaron with those things fixed.

Comment 127

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

Out of the 8, I don't consider these 5 self-explanatory:

+#define NS_THEME_WIN_WINDOW_CAPTION                        231
+#define NS_THEME_WIN_WINDOW_CAPTIONMAX                     232
+#define NS_THEME_WIN_WINDOW_FRAMELEFT                      233
+#define NS_THEME_WIN_WINDOW_FRAMERIGHT                     234
+#define NS_THEME_WIN_WINDOW_FRAMEBOTTOM                    235

Comment 128

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

(In reply to comment #125)
> A review would be great. The values are pretty self explanatory. Maybe some
> commenting in layout/style/nsCSSProps.cpp would suffice? Re feedback, I was
> looking for confirmation that spewing -moz-appearance with win specific values
> for building frames was acceptable. (It works quite well, FWIW.)

We have a bunch of platform-specific values of -moz-appearance.  I'm not sure what you mean by "building frames", though?  I'm guessing it has something to do with drawing things that Windows would normally have drawn in the titlebar if we hadn't taken over drawing of the titlebar, so now we have to draw them.  But I'm not sure what caption and captionmax are (or what the difference is), nor why you have right, left, and bottom sides of a "frame", but not top.  (I'd have thought the titlebar would have right, left, and top sides.)

Comment 129

[Jim Mathies [:jimm]]

(In reply to comment #127)
> Out of the 8, I don't consider these 5 self-explanatory:
> 
> +#define NS_THEME_WIN_WINDOW_CAPTION                        231
> +#define NS_THEME_WIN_WINDOW_CAPTIONMAX                     232
> +#define NS_THEME_WIN_WINDOW_FRAMELEFT                      233
> +#define NS_THEME_WIN_WINDOW_FRAMERIGHT                     234
> +#define NS_THEME_WIN_WINDOW_FRAMEBOTTOM                    235

(In reply to comment #128)
> (In reply to comment #125)
> > A review would be great. The values are pretty self explanatory. Maybe some
> > commenting in layout/style/nsCSSProps.cpp would suffice? Re feedback, I was
> > looking for confirmation that spewing -moz-appearance with win specific values
> > for building frames was acceptable. (It works quite well, FWIW.)
> 
> We have a bunch of platform-specific values of -moz-appearance.  I'm not sure
> what you mean by "building frames", though?  I'm guessing it has something to
> do with drawing things that Windows would normally have drawn in the titlebar
> if we hadn't taken over drawing of the titlebar, so now we have to draw them. 
> But I'm not sure what caption and captionmax are (or what the difference is),
> nor why you have right, left, and bottom sides of a "frame", but not top.  (I'd
> have thought the titlebar would have right, left, and top sides.)

They represent the components of a window frame: 'caption' is the upper titlebar area and border ('captionmax' is the same for a maximized window, which can be slightly different visually from 'caption'), 'frameleft' is the left hand frame, 'frameright' is the right hand frame, and 'framebottom' is the bottom frame below the statusbar. They represent the background textures of a window, which we are going to render manually in some cases. The do fit together nicely, if you draw each of the components in the proper place, they connect up to form a complete window frame.

For general clarity, I suppose 'titlebar' might be better than 'caption'. However, these are windows constants so, :) it's probably acceptable to use the windows convention.

I will add some commenting I guess to clarify what each value means. (unless you would prefer different names entirely for some components?)

Comment 130

[Wesley Johnston (:wesj)]

Seems like there should also be a restore button (WP_RESTOREBUTTON). Also I thought you could pull appropriate size and margin spacings out of the theme.

Comment 131

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

I'd probably go for something like:
 -moz-titlebar
 -moz-titlebar-maximized
 -moz-window-frame-left
 -moz-window-frame-right
 -moz-window-frame-bottom

The concepts don't seem windows-specific, so I think it's better to drop "win" from all the names (including the other three) even if they're currently only implemented on Windows.

Still, I don't really understand how the window-frame-* appearance styles will be used:  are there actually going to be elements in browser.xul, etc., that will take those -moz-appearance styles?

Comment 132

[Jim Mathies [:jimm]]

Misc. naming and preconditions address. A few comments:

(In reply to comment #126)
> (From update of attachment 452789 [details] [diff] [review])
> I'm not happy about the "attached" terminology here, since it seems
> like the term could easily apply to all views that have widgets, rather
> than just only those that were tied to a top-level widget.  Maybe
> mAttachedToWidget should be mWidgetIsTopLevel?

Not a problem, I'll use "top level". I still like "attached" for the work we do in hooking up to a widget though. From your comments I'm guessing continuing with that convention for the nsView / widget interface is ok.

> Both GetAttachedWrapperFor and GetAttachedViewFor should have the normal
> flow of control be non-indented, and the unexpected things be indented,
> i.e.:
>   if (!aWidget)
>     return nsnull;
>   etc.
> 
> Also, GetAttachedWrapperFor is using widget APIs that really look
> non-ideal.  I presume they're added in another patch on this bug.  This
> code:
> >+    void* clientData;
> >+    aWidget->GetAttachedViewPtr(clientData);
> >+    nsISupports* data = (nsISupports*)clientData;
> >+    
> >+    if (data) {
> >+      ViewWrapper* wrapper;
> >+      CallQueryInterface(data, &wrapper);
> seems excessive.  The nsIWidget API could deal in a type more useful
> than void* -- perhaps even ViewWrapper*.  And please don't add new APIs
> that use references rather than pointers for out parameters.  (Or, even
> better, use return values rather than out parameters.)

The reason this works this way is due to existing code. There's already a widget event callback in place w/apis (see get/set clientdata), so this new code is basically a mirror of the existing functionality.

There's a lot of cleanup taking place in here right now and that work could involve improving the widget interfaces, but I would prefer to stick with the current convention for this new code. (I don't mind cleaning up legacy code as I go but I always seem to get dinged on it.. "file another bug!")
 

> This seems broken.  It would be better to assert that the widget isn't
> already attached.  If it *is* already attached, things will break badly
> because there's still a view that thinks it's attached, and it'll try to
> detach later, and end up detaching some other view.

Good point. Since document viewer was the only code calling this I put functionality here that should have been in the caller. I've updated this and added new attach preconditions on aWidget, mWindow, and GetAttachedViewFor(aWidget).   
 
> 
> I'd particularly suggest removing:
> >+          // Usually the width x height of the entire child window.

I'll purge some of this. tn has been working in here and has been doing a lot of more accurate commenting than I have.

> 
> 
> r=dbaron with those things fixed.

Comment 133

[Jim Mathies [:jimm]]

(In reply to comment #130)
> Seems like there should also be a restore button (WP_RESTOREBUTTON).

I don't believe we have a use for restore since we don't support mdi. (These are all very easy to add though so if we find something people want we can add them easily.

> Also I
> thought you could pull appropriate size and margin spacings out of the theme.

You can, in cases where it's available the style data will be populated. For example frameleft and right have minimum heights in aero basic of 92 px.

Comment 134

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

Comment on attachment 453154 [details] [diff] [review]
css frame rendering

sounds (from IRC) like comment 131 was the feedback you wanted on this

Comment 135

[Jim Mathies [:jimm]]

(In reply to comment #133)
> (In reply to comment #130)
> > Seems like there should also be a restore button (WP_RESTOREBUTTON).
> 
> I don't believe we have a use for restore since we don't support mdi. (These
> are all very easy to add though so if we find something people want we can add
> them easily.

Yes we'll need a restore button!

Comment 136

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

(In reply to comment #132)
> > >+    void* clientData;
> > >+    aWidget->GetAttachedViewPtr(clientData);
> > >+    nsISupports* data = (nsISupports*)clientData;
> > >+    
> > >+    if (data) {
> > >+      ViewWrapper* wrapper;
> > >+      CallQueryInterface(data, &wrapper);
> > seems excessive.  The nsIWidget API could deal in a type more useful
> > than void* -- perhaps even ViewWrapper*.  And please don't add new APIs
> > that use references rather than pointers for out parameters.  (Or, even
> > better, use return values rather than out parameters.)
> 
> The reason this works this way is due to existing code. There's already a
> widget event callback in place w/apis (see get/set clientdata), so this new
> code is basically a mirror of the existing functionality.
> 
> There's a lot of cleanup taking place in here right now and that work could
> involve improving the widget interfaces, but I would prefer to stick with the
> current convention for this new code. (I don't mind cleaning up legacy code as
> I go but I always seem to get dinged on it.. "file another bug!")

I don't think it's a problem to add a clean API next to an ugly API.  But if you really want to leave it, maybe it would be better to assume that the AttachedViewPtr is a ViewWrapper* rather than nsISupports*, and just skip the QueryInterface?

Comment 137

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 452930 [details] [diff] [review]
document to widget glue

>+++ b/layout/base/nsDocumentViewer.cpp
>+  PRPackedBool mAttachedToParent; // view is attached to the parent widget

It doesn't make sense to make this a packed bool given its location in the class.  Either make it a PRBool, or put it next to other packed bools.
 
>+    // When we are attached to a parent chrome window, only apply changes to
>+    // the client area, the view owner manages the actual window.

Not sure what you mean by "view owner" here.  Perhaps we should just say that we don't own the widget, so shouldn't mess with its actual sizing?

>+  // Prior to creating a new widget, check to see if our parent is a base
>+  // chrome ui window. If so, drop the content into that widget instead of
>+  // creating a new child widget. This eliminates the main content child
>+  // widget we've had forever. Also allows for the recycling of the base
>+  // widget of each window, vs. creating/discarding multiple child widgets
>+  // during each load.

But there is no "each load" in practice for root chrome.  There's just one load.  So I'm not sure what that last sentence means....  

>+#ifdef XP_WIN

There should be some documentation here about why this is Windows-only.

>+      // Reset the visible state until the new content loads
>+      Hide();

Why is this needed?  All the relevant callers I see offhand would be calling Hide() anyway once this method returns.  Why is that not enough?  Or does SetDOMDocument get called from chrome?  The comment here should probably explain why we're hiding.

>+      // Reuse the parent chrome widget. We may already be attached to this
>+      // widget if we're being called from SetDOMDocument. nsIView will handle
>+      // this situation for us. Note we are attaching to a NS_WINDOW_CID,
>+      // not an NS_CHILD_CID.

Why is that last sentence relevant?

r=bzbarsky with that Hide stuff sorted out.

Comment 138

[Jim Mathies [:jimm]]

Attachment: document to widget glue

Addressed comments. Also, changes in view have made it so the document is now responsible for detaching the parent widget. This needs to happen before the next document tries to attach, because the view will assert now if you attach to a window that is already attached to another view. Based on a little break point testing, Close seemed like a good fit for this.

Comment 139

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 453655 [details] [diff] [review]
 document to widget glue

r=bzbarsky

Comment 140

[Jim Mathies [:jimm]]

Attachment: view and view manager

dbaron - addressed all the comments, including changing the widget interface to return a view wrapper. One caveat though, it turns out that the "auto-detach from a widget" functionality in AttachToToTopLevelWidget is critical in making this work. I revamped the document viewer patch to do the detach in Close(), but that created a gap in event delivery that caused all sort of funny anomalies in the ui on startup.

The correct fox for that is to address the problem w/ document viewer where we call MakeWindow multiple times during the creation and load of a base window. Honestly though don't know how much work that may be, so if it's alright with you, I've put the auto-detach back in, added a comment about it being temporary, and will file a follow up bug to address the MakeWindow problem post beta freeze.

Comment 141

[Jim Mathies [:jimm]]

Attachment: win chrome margins

Comment 142

[Jim Mathies [:jimm]]

Attachment: css frame rendering

changes: updated css constants

Comment 143

[Rob Arnold [:robarnold]]

Comment on attachment 453768 [details] [diff] [review]
win chrome margins

>+NS_IMETHODIMP
>+nsWindow::GetNonClientMargins(nsIntMargin &margins)
>+{
>+  nsWindow * window = GetTopLevelWindow(PR_TRUE);
>+  if (window && window != this) {
>+    return window->GetNonClientMargins(margins);
>+  }
>+
>+  nsCOMPtr<nsILookAndFeel> lookAndFeel =
>+    do_GetService("@mozilla.org/widget/lookandfeel;1");
>+  if (!lookAndFeel) {
>+    NS_WARNING("We need nsILookAndFeel for GetNonClientMargins!");
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  if (mCustomNonClient) {
>+    margins = mNonClientMargins;
>+    return NS_OK;
>+  }

Let's push the do_GetService call down here to avoid it for the mCustomNonClient case.
>+  // If a margin value is 0, set the offset to the default size of the frame.
>+  // If a margin is -1, leave as default, and if a margin > 0, set the offset
>+  // so that the frame size is equal to the margin value.
>+  if (!mNonClientMargins.top)
>+    mNonClientOffset.top = mCaptionHeight;
>+  else if (mNonClientMargins.top > 0)
>+    mNonClientOffset.top = mCaptionHeight - mNonClientMargins.top;
>+
>+  if (!mNonClientMargins.left)
>+    mNonClientOffset.left = mResizeMargin;
>+  else if (mNonClientMargins.left > 0)
>+    mNonClientOffset.left = mResizeMargin - mNonClientMargins.left;
>+
>+  if (!mNonClientMargins.right)
>+    mNonClientOffset.right = mResizeMargin;
>+  else if (mNonClientMargins.right > 0)
>+    mNonClientOffset.right = mResizeMargin - mNonClientMargins.right;

I liked it better when you had these as a single if statement per side.

>+  NS_ASSERTION(mNonClientOffset.top >= 0, "non-client top margin is negative!");
>+  NS_ASSERTION(mNonClientOffset.left >= 0, "non-client left margin is negative!");
>+  NS_ASSERTION(mNonClientOffset.right >= 0, "non-client right margin is negative!");
>+  NS_ASSERTION(mNonClientOffset.bottom >= 0, "non-client bottom margin is negative!");
>+
>+  if (mNonClientOffset.top < 0)
>+    mNonClientOffset.top = 0;
>+  if (mNonClientOffset.left < 0)
>+    mNonClientOffset.left = 0;
>+  if (mNonClientOffset.right < 0)
>+    mNonClientOffset.right = 0;
>+  if (mNonClientOffset.bottom < 0)
>+    mNonClientOffset.bottom = 0;
>+  return PR_TRUE;
>+}

Didn't you just assert that all these if branches should be false?

>+  mNonClientMargins = margins;
>+  mCustomNonClient = PR_TRUE;
>+  if (!UpdateNonClientMargins())
>+    return NS_ERROR_NOT_AVAILABLE;

This can only fail if we couldn't get the L&F service. How about at least a warning?

>+#if 0
>+  printf("non-client margins: left:%02d right:%02d top:%02d bottom:%02d\n",
>+         mNonClientOffset.left, mNonClientOffset.right,
>+         mNonClientOffset.top, mNonClientOffset.bottom);
>+#endif

nit: can you remove this or pick a define name?

>+#if 0
>+  printf("glass margin: %d x %d %d x %d\n", mGlassMargins.cxLeftWidth,
>+         mGlassMargins.cyTopHeight, mGlassMargins.cxRightWidth,
>+         mGlassMargins.cyBottomHeight);
>+#endif

Ditto.

>@@ -445,16 +450,17 @@ protected:
>   DWORD_PTR             mOldExStyle;
>   HIMC                  mOldIMC;
>   PRUint32              mIMEEnabled;
>   nsNativeDragTarget*   mNativeDragTarget;
>   HKL                   mLastKeyboardLayout;
>   nsPopupType           mPopupType;
>   PRPackedBool          mDisplayPanFeedback;
>   WindowHook            mWindowHook;
>+  PRPackedBool          mHideChrome;

Let's put this someplace it can be packed. Above mWindowHook?

> // Enabled main event loop debug event output
>-//#define EVENT_DEBUG_OUTPUT
>+#define EVENT_DEBUG_OUTPUT

I don't think you meant this.

r+ with those changes.

Comment 144

[Rob Arnold [:robarnold]]

Comment on attachment 453776 [details] [diff] [review]
css frame rendering

>+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 145

[Jim Mathies [:jimm]]

dbaron, can you can sign off on this last patch? Everything else is approved and ready to land.

Comment 146

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

Comment on attachment 453749 [details] [diff] [review]
view and view manager

r=dbaron

Comment 147

[Jim Mathies [:jimm]]

Attachment: additional chromemargin tests

Comment 148

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 453916 [details] [diff] [review]
additional chromemargin tests

>diff --git a/toolkit/content/tests/chrome/test_chromemargin.xul b/toolkit/content/tests/chrome/test_chromemargin.xul

>+   - The Initial Developer of the Original Code is
>+   - Mozilla Corporation.

The Mozilla Foundation. (applies to both files; see http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html )

>diff --git a/toolkit/content/tests/chrome/window_chromemargin.xul b/toolkit/content/tests/chrome/window_chromemargin.xul

>+chrome margins rock!

Is this needed?

>+function doSingleTest(param, shouldSucceed)
>+{
>+  var exception = false;

>+  } catch (ex) {
>+    exception = true;
>+  }
>+  if (shouldSucceed)
>+    ok(!exception, "failed for param:'" + param + "'");

You could initialize exception to null, set |exception = ex| in the catch block, and then add it to the test description message to help diagnose any failures.

>+function runTests()

This could just use document.documentElement rather than document.getElementById("window") (perhaps assigned to a local var for shorter test lines).

Comment 149

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

Jim: now that the tree is re-opened, think we'll see this land tonight?

Comment 150

[Jim Mathies [:jimm]]

(In reply to comment #149)
> Jim: now that the tree is re-opened, think we'll see this land tonight?

yep.

Comment 151

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/2d094a5e0062
http://hg.mozilla.org/mozilla-central/rev/8fa991369392
http://hg.mozilla.org/mozilla-central/rev/720daae69c14
http://hg.mozilla.org/mozilla-central/rev/1d4ef53ab997
http://hg.mozilla.org/mozilla-central/rev/345fa36190e4
http://hg.mozilla.org/mozilla-central/rev/95d2ea5d74fe
http://hg.mozilla.org/mozilla-central/rev/c2b05f3a1b13
http://hg.mozilla.org/mozilla-central/rev/a053227f5f96
http://hg.mozilla.org/mozilla-central/rev/5b1c95b84304
http://hg.mozilla.org/mozilla-central/rev/30ecff316b34
http://hg.mozilla.org/mozilla-central/rev/36684120d44c
http://hg.mozilla.org/mozilla-central/rev/ec0e7c69316a

Comment 152

[Jacek Caban]

Attachment: Fix for MOZ_WINSDK_TARGETVER < MOZ_NTDDI_LONGHORN

These patches broke compilation for MOZ_WINSDK_TARGETVER < MOZ_NTDDI_LONGHORN. The attached patch fixes the problem.

Comment 153

[Bas Schouten (:bas.schouten)]

Does anyone know if this could have caused bug 574631?

Comment 154

[Gary [:streetwolf52]]

No Problems:  522df66198cf

Big Problems: 51bd519736c4

So I would say this patch caused the problem

Comment 155

[Ria Klaassen (not reading all bugmail)]

I see a new taskbar button instead of a tooltip.

Comment 156

[Paul [pwd]]

(In reply to comment #155)
> I see a new taskbar button instead of a tooltip.

Have you applied today's nightly? It was an issue yesterday but has been fixed today.

Comment 157

[Jim Mathies [:jimm]]

Comment on attachment 454002 [details] [diff] [review]
Fix for  MOZ_WINSDK_TARGETVER < MOZ_NTDDI_LONGHORN

sorry about the busted ifdef'ing!

Comment 158

[Jacek Caban]

http://hg.mozilla.org/mozilla-central/rev/1233da24d7f3

Comment 159

[budogirls]

No bug about drawing in the Title Bar for XP.

Comment 160

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #159)
> No bug about drawing in the Title Bar for XP.

that's bug 574454, apparently.

Comment 161

[Alfred Kayser]

This is apparently not for Windows XP, but for Vista and 7.

Comment 162

[Peter Henkel [:Terepin]]

Jim, regarding bug 577132, can we change the color of glow of Min/Max buttons?

Comment 163

[Jim Mathies [:jimm]]

(In reply to comment #162)
> Jim, regarding bug 577132, can we change the color of glow of Min/Max buttons?

On glass desktops w/out personas probably not, since we're using the default caption buttons the dwm provides which we have no control over.

Comment 164

[Jean-Yves Perrier [:teoli]]

If I understand this bug correctly it, as a "side effect" of its main goal, added a new attribute to XUL Window element: chromemargin.

Could somebody add the dev-doc-needed keyword so that we don't forget to document it on the MDC?

Thanks!

Comment 165

[murat]

When fix 600315?

Comment 166

[Eric Shepherd [:sheppy]]

Now documented:

https://developer.mozilla.org/en/XUL/Attribute/chromemargin

Also listed on the XUL window page and Fx 4 for developers.