(this is post-Firefox 4 material, but filing it so we can take a look at it for the next release) In Firefox 4, we decided to support tabs in title bar on Windows when the window was in a maximized state. We'd like to add this capability to the OS X version too, but in a slightly different way: Since OS X has no concept of "maximized" (it only has "zoomed"), it should probably be an explicit switch in the preference panel or the Customize palette. The downside of having tabs in titlebar on Mac is of course that you get less of a draggable area for moving the window, but on the upside, you can use any empty area in the main UI for moving the window already, so it will be less of an issue than doing the same on Windows. With OS X 10.7 moving to a structure where the close/maximize/minimize buttons are centered in the topmost toolbar, I'm pretty sure we can pull this off and make it look good too.

(In reply to comment #0) > The downside of having tabs in titlebar on Mac is of course that you get less > of a draggable area for moving the window, but on the upside, you can use any > empty area in the main UI for moving the window already, Bug 615807 wants to change that, by the way. I've started on the implementation of this and now I'm uncertain what to do with the window title. Should the title also be hidden in tabs-on-bottom mode? (Hiding it always would be the simplest to implement.) On Windows it seems to be the case that the title is hidden iff the Firefox button is in the title bar, i.e. if the window is in draw-in-titlebar mode.

is this only to be in 10.7 Lion? fx6 or 7,8?

(In reply to comment #3) > I've started on the implementation of this and now I'm uncertain what to do > with the window title. Should the title also be hidden in tabs-on-bottom > mode? (Hiding it always would be the simplest to implement.) > On Windows it seems to be the case that the title is hidden iff the Firefox > button is in the title bar, i.e. if the window is in draw-in-titlebar mode. Would it be possible to have the titlebar as a toolbar you can toggle? Like this: http://www.stephenhorlander.com/pages/configs/main-window-configurations-mac.html

Yes, that's possible. Having the title as a XUL label would require more XUL/JS changes but actually simplify things from the widget perspective. I like the idea.

Lion is now released and I am admittedly disappointed that there is no status update on this bug - this should be a priority fix for Firefox 7 as the lack of cross-platform UI compatibility is obviously lacking with regards to the tabs. The most annoying thing is that "browser.tabs.drawInTitlebar" is set to TRUE in about:config and the UI doesn't reflect this setting. Chrome, and a few of Apple's new Lion apps, reflect the titlebar space saving. This shouldn't be something that is postponed for Firefox 8 - it took the Chrome devs only 4 days to released Lion updates for full-screen, gesture and, yes, proper tabs in titlebar support.

Anyone working on this? I´ve seen all new mockups have tabs in title bar on Mac.

Not at the moment, no. But if anybody wants to pick this up, some of the harder parts of the code can be found in bug 647216.

Reassigning this to nobody, this is just a tracking bug. I'm happy to work on any dep bugs in Gecko. I just posted a new patch for the biggest remaining blocker, bug 647216.

I don't see any other dependencies for this bug now that bug 647216 is fixed, so marking it as RESO-FIXED.

This still needs browser changes.

This patch is dependent on the patch in bug 647216. This makes Firefox always draw in the titlebar. When not using a lw-theme, the titlebar gets a style of -moz-window-titlebar, which I've added support for in widget/cocoa.

Comment on attachment 700610 [details] [diff] [review] Always draw in the titlebar on OSX - v1 I haven't been able to connect with Markus Stange, so requesting feedback from you Steven - hope that's OK!

> hope that's OK! No problem. But I'm not going to get to this soon. On top of the usual craziness I'm always trying to stay on top of, I've now got a bunch of fires from the FF 18 release to put out or otherwise deal with.

Comment on attachment 700610 [details] [diff] [review] Always draw in the titlebar on OSX - v1 Hello Stephen - rstrong said you might be able to take a look at this and give me feedback.

Comment on attachment 700610 [details] [diff] [review] Always draw in the titlebar on OSX - v1 Review of attachment 700610 [details] [diff] [review]: ----------------------------------------------------------------- I'm fine with this strategy.

Comment on attachment 700610 [details] [diff] [review] Always draw in the titlebar on OSX - v1 Review of attachment 700610 [details] [diff] [review]: ----------------------------------------------------------------- I too like this approach and it seems to work well from my limited testing. A few things that I'd like to point out: 1. I've observed a rare glitch where a solid light grey bar is drawn underneath the title bar but above the tabs bar, with a height of approximately 10px. When this occurs, the bar doesn't disappear until the browser is restarted. I was able to reproduce a few times when switching from lightweight themes to no theme and meant to take a screenshot, but I've been unable to reproduce more recently. This may be unrelated to this patch, but I thought it was worth pointing out. 2. I'm not a huge fan of the break between the titlebar and the tabs bar. I would prefer the gradient that you pointed out in bug 647216 in comment 69 (http://i.imgur.com/qEzCw.png). If we could fix the gradient to be the same as in the current nightly, that would be ideal. 3. When switching to fullscreen mode (by clicking the button on the right side of the title bar), the tabs bar jumps noticeably (best observed with no theme applied). When going to fullscreen, the tabs bar jumps to the top when the transition finishes. When returning from fullscreen, the tabs bar is drawn on top of the title bar until the transition ends and then jumps back down to underneath the title bar. The nightly has a smooth transition for both scenarios. ::: toolkit/content/LightweightThemeConsumer.jsm @@ +116,3 @@ > root.setAttribute("drawintitlebar", "true"); > + } > + else if (this._hasActivated && !this._drawInTitlebarDefault) { Is there ever a need to reset the attribute to the actual value of the default if one was present? Currently, the behavior is correct if the default had a value of true, but we don't restore the attribute to false if the default was false.

(In reply to Stephen Pohl [:spohl] from comment #19) > Comment on attachment 700610 [details] [diff] [review] > Always draw in the titlebar on OSX - v1 > > Review of attachment 700610 [details] [diff] [review]: > ----------------------------------------------------------------- > > I too like this approach and it seems to work well from my limited testing. > A few things that I'd like to point out: > > 1. I've observed a rare glitch where a solid light grey bar is drawn > underneath the title bar but above the tabs bar, with a height of > approximately 10px. When this occurs, the bar doesn't disappear until the > browser is restarted. I was able to reproduce a few times when switching > from lightweight themes to no theme and meant to take a screenshot, but I've > been unable to reproduce more recently. This may be unrelated to this patch, > but I thought it was worth pointing out. Hrm. No luck reproducing this one yet... > 2. I'm not a huge fan of the break between the titlebar and the tabs bar. I > would prefer the gradient that you pointed out in bug 647216 in comment 69 > (http://i.imgur.com/qEzCw.png). If we could fix the gradient to be the same > as in the current nightly, that would be ideal. Weird - the whole titlebar was flipped when applied this patch on a fresh pull from m-c. I've fixed this patch so that we don't flip anymore. The result is a smooth gradient transition from the titlebar to toolbar, a la that screenshot. > 3. When switching to fullscreen mode (by clicking the button on the right > side of the title bar), the tabs bar jumps noticeably (best observed with no > theme applied). When going to fullscreen, the tabs bar jumps to the top when > the transition finishes. When returning from fullscreen, the tabs bar is > drawn on top of the title bar until the transition ends and then jumps back > down to underneath the title bar. The nightly has a smooth transition for > both scenarios. > Yes, this is not ideal, and I'm open to suggestions. I'm already way out of my depth with this patch - I have no idea how native fullscreen works, and I could use some pointers. I find it particularly strange that (with this latest patch) when initting full-screen, the titlebar gets inverted before disappearing. > ::: toolkit/content/LightweightThemeConsumer.jsm > @@ +116,3 @@ > > root.setAttribute("drawintitlebar", "true"); > > + } > > + else if (this._hasActivated && !this._drawInTitlebarDefault) { > > Is there ever a need to reset the attribute to the actual value of the > default if one was present? Currently, the behavior is correct if the > default had a value of true, but we don't restore the attribute to false if > the default was false. If the default value was false, I believe this is the behaviour: 1) On _update called with active = true, we sample the drawintitlebar attribute and record it (so this._drawInTitlebarDefault = false, since drawintitlebar is not there). 2) On a subsequent call with active = false, if !this._drawInTitlebarDefault, the attribute is removed. Is my logic flawed somehow?

(In reply to Mike Conley (:mconley) from comment #20) > Created attachment 705093 [details] [diff] [review] > Always draw in the titlebar on OSX - v1.1 > > (In reply to Stephen Pohl [:spohl] from comment #19) > > Comment on attachment 700610 [details] [diff] [review] > > Always draw in the titlebar on OSX - v1 > > > > Review of attachment 700610 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > ::: toolkit/content/LightweightThemeConsumer.jsm > > @@ +116,3 @@ > > > root.setAttribute("drawintitlebar", "true"); > > > + } > > > + else if (this._hasActivated && !this._drawInTitlebarDefault) { > > > > Is there ever a need to reset the attribute to the actual value of the > > default if one was present? Currently, the behavior is correct if the > > default had a value of true, but we don't restore the attribute to false if > > the default was false. > > If the default value was false, I believe this is the behaviour: > > 1) On _update called with active = true, we sample the drawintitlebar > attribute and record it (so this._drawInTitlebarDefault = false, since > drawintitlebar is not there). > > 2) On a subsequent call with active = false, if > !this._drawInTitlebarDefault, the attribute is removed. > > Is my logic flawed somehow? I'll have a look at the rest a bit later, but for this one, I think you would want to use root.getAttribute("drawintitlebar"); instead of root.hasAttribute("drawintitlebar"); on line 113, handling the case gracefully when the attribute is not present. Then, the behavior is as you outlined. Currently, you're sampling the presence, but not the value of the attribute. I hope this makes sense.

(In reply to Stephen Pohl [:spohl] from comment #21) > (In reply to Mike Conley (:mconley) from comment #20) > > Created attachment 705093 [details] [diff] [review] > > Always draw in the titlebar on OSX - v1.1 > > > > (In reply to Stephen Pohl [:spohl] from comment #19) > > > Comment on attachment 700610 [details] [diff] [review] > > > Always draw in the titlebar on OSX - v1 > > > > > > Review of attachment 700610 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > ::: toolkit/content/LightweightThemeConsumer.jsm > > > @@ +116,3 @@ > > > > root.setAttribute("drawintitlebar", "true"); > > > > + } > > > > + else if (this._hasActivated && !this._drawInTitlebarDefault) { > > > > > > Is there ever a need to reset the attribute to the actual value of the > > > default if one was present? Currently, the behavior is correct if the > > > default had a value of true, but we don't restore the attribute to false if > > > the default was false. > > > > If the default value was false, I believe this is the behaviour: > > > > 1) On _update called with active = true, we sample the drawintitlebar > > attribute and record it (so this._drawInTitlebarDefault = false, since > > drawintitlebar is not there). > > > > 2) On a subsequent call with active = false, if > > !this._drawInTitlebarDefault, the attribute is removed. > > > > Is my logic flawed somehow? > > I'll have a look at the rest a bit later, but for this one, I think you > would want to use root.getAttribute("drawintitlebar"); instead of > root.hasAttribute("drawintitlebar"); on line 113, handling the case > gracefully when the attribute is not present. Then, the behavior is as you > outlined. Currently, you're sampling the presence, but not the value of the > attribute. I hope this makes sense. Yikes - you're absolutely right; I thought the widget listener was just checking for the presence of drawintitlebar and not actually paying attention to its value. Now I for the actual *value* of drawintitlebar; not just its presence.

Comment on attachment 705109 [details] [diff] [review] Always draw in the titlebar on OSX - v1.2 >+ [NSNumber numberWithBool:NO], @"is.flipped", YES was actually correct. There was a bug in the patch in bug 647216 which caused title bar rendering to be flipped. Other than that, this patch looks very good to me. There's one piece which I think is still missing, which would allow us to make the titlebar smaller without destroying the gradient: We should take the position of the top edge of the toolbar box into consideration when calculating the unified toolbar height. I'll play around with this a little.

Here's what I have so far. This patch, applied on top of yours, makes the gradient look good even if I remove padding-top from #titlebar. There's one part which needs to be looked at again, it's marked with a XXX comment.

De-bitrotted after changes in the patches in bug 647216. I've also folded in mstange's patch. mstange's patch appears to have fixed the issue with the toolbar separator appearing between the titlebar and the nav-bar when transitioning to full-screen mode. Thanks!

I've made some more changes to handle HiDPI mode correctly.

Comment on attachment 705828 [details] [diff] [review] Always draw in the titlebar on OSX - v1.4 The patch looks good to me, but I'm not a reviewer for browser.js (Dão should do that part) and the widget part should be reviewed by Josh, too.

(In reply to Markus Stange from comment #27) > Comment on attachment 705828 [details] [diff] [review] > Always draw in the titlebar on OSX - v1.4 > > The patch looks good to me, but I'm not a reviewer for browser.js (Dão > should do that part) and the widget part should be reviewed by Josh, too. I've noticed that with this last patch, when transitioning to the full-screen state, a toolbar separator briefly appears between the titlebar and the tabstrip. I don't recall seeing this separator in patch v1.3.

De-bitrotting.

Comment on attachment 712910 [details] [diff] [review] Always draw in the titlebar on OSX v1.5 (r=josh, feedback=mstange) Review of attachment 712910 [details] [diff] [review]: ----------------------------------------------------------------- I have some drive-by comments since I wanted to double-check you handled the ifdef in LWTConsumer while I was reviewing bug 813802. ::: browser/base/content/browser.js @@ +4984,5 @@ > // the resize event (handled in tabbrowser.xml). > this.allowedBy("sizemode", false); > > +#ifdef XP_MACOSX > + document.documentElement.setAttribute("drawintitlebar", "true"); Do we really need another attribute instead of using @tabsintitlebar? ::: browser/themes/pinstripe/browser.css @@ +42,1 @@ > #main-window:not([drawintitlebar=true]) > #titlebar { I think we should avoid supporting drawintitlebar/tabsintitlebar = false on pinstripe unless there is a clear need to support it. Adding new variables is a maintenance burden. If some theme/add-on wants to support the false value then they are free to do so but I don't think we should in pinstripe. In other words, I think the attribute checks should be removed.

(In reply to Matthew N. [:MattN] from comment #30) > Comment on attachment 712910 [details] [diff] [review] > Always draw in the titlebar on OSX v1.5 (r=josh, feedback=mstange) > > Review of attachment 712910 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have some drive-by comments since I wanted to double-check you handled the > ifdef in LWTConsumer while I was reviewing bug 813802. > > ::: browser/base/content/browser.js > @@ +4984,5 @@ > > // the resize event (handled in tabbrowser.xml). > > this.allowedBy("sizemode", false); > > > > +#ifdef XP_MACOSX > > + document.documentElement.setAttribute("drawintitlebar", "true"); > > Do we really need another attribute instead of using @tabsintitlebar? > We've been using this attribute for a while - it's our way of signalling to the Cocoa window that we want to draw in the titlebar. If we want tabs in titlebar to be optional via a pref, we will still need this attribute in order for lw-themes to work properly (since we must set drawintitlebar to true in order to draw lw-themes in there). > ::: browser/themes/pinstripe/browser.css > @@ +42,1 @@ > > #main-window:not([drawintitlebar=true]) > #titlebar { > > I think we should avoid supporting drawintitlebar/tabsintitlebar = false on > pinstripe unless there is a clear need to support it. Adding new variables > is a maintenance burden. If some theme/add-on wants to support the false > value then they are free to do so but I don't think we should in pinstripe. > > In other words, I think the attribute checks should be removed. I would agree if it was true that we don't want drawing of the tabs in the titlebar to be optional. Is this the case?

Comment on attachment 712910 [details] [diff] [review] Always draw in the titlebar on OSX v1.5 (r=josh, feedback=mstange) >@@ -4979,16 +4979,19 @@ var TabsInTitlebar = { > #ifdef CAN_DRAW_IN_TITLEBAR > this._readPref(); > Services.prefs.addObserver(this._prefName, this, false); > > // Don't trust the initial value of the sizemode attribute; wait for > // the resize event (handled in tabbrowser.xml). > this.allowedBy("sizemode", false); > >+#ifdef XP_MACOSX >+ document.documentElement.setAttribute("drawintitlebar", "true"); >+#endif > this._initialized = true; > #endif > }, TabsInTitlebar.init being called doesn't actually mean that we're drawing tabs in the title bar. That's the point of the allowedBy calls. You should set the attribute in TabsInTitlebar._update.

(In reply to Dão Gottwald [:dao] from comment #32) > Comment on attachment 712910 [details] [diff] [review] > Always draw in the titlebar on OSX v1.5 (r=josh, feedback=mstange) > > >@@ -4979,16 +4979,19 @@ var TabsInTitlebar = { > > #ifdef CAN_DRAW_IN_TITLEBAR > > this._readPref(); > > Services.prefs.addObserver(this._prefName, this, false); > > > > // Don't trust the initial value of the sizemode attribute; wait for > > // the resize event (handled in tabbrowser.xml). > > this.allowedBy("sizemode", false); > > > >+#ifdef XP_MACOSX > >+ document.documentElement.setAttribute("drawintitlebar", "true"); > >+#endif > > this._initialized = true; > > #endif > > }, > > TabsInTitlebar.init being called doesn't actually mean that we're drawing > tabs in the title bar. That's the point of the allowedBy calls. You should > set the attribute in TabsInTitlebar._update. Hm, I guess we'll have to be careful then about removing the drawintitlebar attribute if allowed=false, since we might have a lightweight theme installed.

Actually, I think my original plan was to *always* draw in the titlebar (hence the name of the patch) - even if we're not drawing tabs up there. That's why I added the drawintitlebar in TabsInTitlebar.init - but I could just have easily put it directly onto the window node itself. Is there a good reason to *not* always draw in the titlebar?

(In reply to Mike Conley (:mconley) from comment #33) > Hm, I guess we'll have to be careful then about removing the drawintitlebar > attribute if allowed=false, since we might have a lightweight theme > installed. You can use mozMatchesSelector and :-moz-lwtheme for that. There's also the problem that a lightweight theme can be applied and removed in different states, making your _drawInTitlebarDefault property unreliable. (In reply to Mike Conley (:mconley) from comment #34) > Actually, I think my original plan was to *always* draw in the titlebar > (hence the name of the patch) - even if we're not drawing tabs up there. So the idea is to always render #titlebar (except in fullscreen mode) such that contend doesn't shift into the title bar area accidentally, and then have the tabs overlay #titlebar when appropriate? > That's why I added the drawintitlebar in TabsInTitlebar.init - but I could > just have easily put it directly onto the window node itself. Right, putting it in TabsInTitlebar.init doesn't make sense either way.

> So the idea is to always render #titlebar (except in fullscreen mode) such > that contend doesn't shift into the title bar area accidentally, and then > have the tabs overlay #titlebar when appropriate? > Yes, that was my idea. > > That's why I added the drawintitlebar in TabsInTitlebar.init - but I could > > just have easily put it directly onto the window node itself. > > Right, putting it in TabsInTitlebar.init doesn't make sense either way. Alright, I'll move it into the window node. > > You can use mozMatchesSelector and :-moz-lwtheme for that. There's also the > problem that a lightweight theme can be applied and removed in different > states, making your _drawInTitlebarDefault property unreliable. > What states are you referring to? Either the window has drawintitlebar set on it by default, or it doesn't - and I don't believe you can (or should) be able to transition from one to the other. What am I missing here?

(In reply to Mike Conley (:mconley) from comment #36) > > You can use mozMatchesSelector and :-moz-lwtheme for that. There's also the > > problem that a lightweight theme can be applied and removed in different > > states, making your _drawInTitlebarDefault property unreliable. > > > > What states are you referring to? Either the window has drawintitlebar set > on it by default, or it doesn't - and I don't believe you can (or should) be > able to transition from one to the other. The state handled in TabsInTitlebar._update resulting from the TabsInTitlebar.allowedBy calls. That is, if you don't set the tabsintitlebar attribute unconditionally in browser.xul.

Hm - so, I remember now - the way nsXULElement works is that it will only initiate drawing into the titlebar on OSX if it sees us set the attribute (via nsXULElement:AfterSetAttr). If the node just starts with the attribute, the signal to Cocoa to let us draw in there is not sent. Markus - I'm not really familiar with the guts of nsXULElement. Do you know if there's a nice way we can read the drawsintitlebar attribute when the element is first manifested, instead of waiting for the attribute to be set?

I encountered the same thing with my changes in bug 829360. You need to add a similar check to nsXULElement::Create, since it doesn't flow through AfterSetAttr when creating an element from a prototype node.

(In reply to Mike Conley (:mconley) from comment #38) > Markus - I'm not really familiar with the guts of nsXULElement. Do you know > if there's a nice way we can read the drawsintitlebar attribute when the > element is first manifested, instead of waiting for the attribute to be set? Yes, this should be done in nsXULWindow::SyncAttributesToWidget(). I've attempted to do that in bug 573973 once, but something held me up there. Maybe you can fix up the patch in that bug.

Instead of using drawintitlebar, I make nsCocoaWindow respond to chromemargin (albeit in a very simple way). chromemargin is now set for OSX directly in the main-window node instead of adding it in TabsInTitlebar.init.

Just realized that we don't need gPrivateBrowsingUI to switch us to drawing in the titlebar, since we *always* draw there with this patch.

Comment on attachment 716568 [details] [diff] [review] Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange) Jim, In bug 573973, you asked Markus to switch us over from using drawintitlebar to chromemargin. I've done so here - though my approach might be a bit simplistic. But does it satisfy your request? -Mike

Hah, I didn't think it would be this simple. Looks good to me, though.

Comment on attachment 716568 [details] [diff] [review] Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange) I'm not the right person for reviewing mac widget code -> smichaud. Front end changes should be reviews by a fx front end peer, Dao maybe?

Comment on attachment 716568 [details] [diff] [review] Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange) Sorry Jim - I should have used feedback? instead. I really just want to know if you're happy with my approach for using chromemargin instead of drawintitlebar, since you requested it in bug 573973.

Comment on attachment 716568 [details] [diff] [review] Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange) The chromemargin usage looks correct. Although I don't fully understand the LightweightThemeConsumer changes. I'm sure Dao will though!

(In reply to Jim Mathies [:jimm] from comment #47) > Comment on attachment 716568 [details] [diff] [review] > Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange) > > The chromemargin usage looks correct. Although I don't fully understand the > LightweightThemeConsumer changes. I'm sure Dao will though! Cool, thanks Jim.

Comment on attachment 716568 [details] [diff] [review] Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange) One small problem in nsNativeThemeCocoa::DrawNativeTitlebar(): + CGContextSaveGState(aContext); + + if (aTitlebarRect.size.width * aTitlebarRect.size.height > CUIDRAW_MAX_AREA) { + return; + } You should put the call to CGContextSaveGState() after the possible early return.

Comment on attachment 716568 [details] [diff] [review] Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange) > LightweightThemeConsumer.prototype = { > _lastData: null, > _lastScreenWidth: null, > _lastScreenHeight: null, >+#ifdef XP_MACOSX >+ _hasActivated: false, >+ _chromemarginDefault: "", >+#endif You can get rid of _hasActivated by setting _chromemarginDefault to null or undefined initially. >+ else if (this._hasActivated && !this._drawInTitlebarDefault) { >+ root.setAttribute("chromemargin", this._chromemarginDefault); >+ } _drawInTitlebarDefault doesn't exist.

Fixed smichaud's CGContextSaveGState(aContext) bug in here, and made sure that the private browsing window mask still draws. (In reply to Dão Gottwald [:dao] from comment #50) > > You can get rid of _hasActivated by setting _chromemarginDefault to null or > undefined initially. > > >+ else if (this._hasActivated && !this._drawInTitlebarDefault) { > >+ root.setAttribute("chromemargin", this._chromemarginDefault); > >+ } > > _drawInTitlebarDefault doesn't exist. Ach, how embarrassing. Is this better?

Comment on attachment 717178 [details] [diff] [review] Always draw in the titlebar on OSX v2.2 (r=josh, feedback=mstange) >+ if (this._chromemarginDefault === undefined) { >+ this._chromemarginDefault = root.getAttribute("chromemargin"); >+ } nit: curly brackets can be dropped per local style >+ if (active) { >+ root.setAttribute("chromemargin", "0,-1,-1,-1"); >+ } >+ else if (this._chromemarginDefault) { >+ root.setAttribute("chromemargin", this._chromemarginDefault); >+ } else { >+ root.removeAttribute("chromemargin"); >+ } nit: I think formatting it as follows would make it easier to comprehend, given that 'active' is a binary switch: if (active) { root.setAttribute("chromemargin", "0,-1,-1,-1"); } else { if (this._chromemarginDefault) root.setAttribute("chromemargin", this._chromemarginDefault); else root.removeAttribute("chromemargin"); }

Thanks! Fixed. Will push to inbound once the tree re-opens.

So I pushed this to try, and got some test failures: https://tbpl.mozilla.org/?tree=Try&rev=683e4d6e8092 I'm currently investigating these - though I think it's taking me into layout-land, so if anybody has any suggestions or advice, I'm all ears.

Hey Markus, I'm not making much headway fixing these tests. The one I'm focusing on particular, is the selectAtPoint test - you can execute this with: ./mach mochitest-chrome dom/tests/mochitest/chrome/test_selectAtPoint.html Note that this test doesn't just fail with this patch - if we draw in the titlebar on trunk (say, but adding a lw-theme), and then run this test again, we fail in the same way. Perhaps you know what's going wrong? jimm suggested that some of our widgetry code might be wrong about their origins or bounds due to our drawing in the titlebar...but, as usual with this stuff, I'm a little out of my depth.

I've filed bug 845595 on the test_selectAtPoint.html failure. About the "ABORT: retaining manager changed out from under us ... HELP!" crash you'll want to talk to Matt Woodrow. I don't know much about that. Also, I've noticed that in windows without a toolbar, the titlebar separator line is on the top instead of on the bottom of the titlebar. I haven't had a chance to look into that yet, though.

Matt, are you able to help out with the crash mentioned in comment #56?

What do I need to do to reproduce that error? I've built with the patch, and nothing seemed to be different. Enabling browser.tabs.drawInTitlebar moves the tabs up (though the draw underneath the close/minimize etc controls). I only see 'GContextRestoreGState: invalid context 0x0' errors with that, not the abort.

Hi Matt, the crash happens during mochitest in debug builds, for example here: https://tbpl.mozilla.org/php/getParsedLog.php?id=20004022&tree=Try&full=1#error1

Alright, I see it. I really don't like this change, because titlebar painting on mac is *really* slow. Because we need to paint the titlebar into a separate CGContext to that of the main window (and one that we can't draw with OpenGL), we need to maintain two retained layer managers. One (OpenGL backed) for the window area, and one (BasicLayers) for the titlebar. Both of these build display lists and layers for the entire browser area, but only do painting for their specific areas. So enabling this is doubling the cost of display list building, layer construction. This particular error is because the remote frame display item doesn't like being in two retained layer trees at once. I'm not sure exactly why this requirement exists, cjones? One option is to try restrict the area that we build display lists for to only the required area, such that they don't overlap. This would fix this current error, but it's still possible (I believe) for an item to lie across the boundary, and thus be required to exist in both. It would also improve the performance a lot. Is it at all possible to draw the titlebar content into the same context as the rest of the window? This would make life a lot easier, especially when we have OpenGL (essentially all mac systems have this enabled).

(In reply to Matt Woodrow (:mattwoodrow) from comment #60) > This particular error is because the remote frame display item doesn't like > being in two retained layer trees at once. I'm not sure exactly why this > requirement exists, cjones? Shadow layers are bound to a single layer manager, like non-shadow layers are. If the layer manager changes, we have to ensure any existing layer tree is thrown out and the shadow-layer machinery is rebuilt for the new manager. We haven't implemented that code yet because we haven't needed it, sorry :(.

Thanks. (In reply to Matt Woodrow (:mattwoodrow) from comment #60) > but it's still possible (I believe) for an item to lie across > the boundary, Tabs will, for example. > Is it at all possible to draw the titlebar content into the same context as > the rest of the window? This would make life a lot easier, especially when > we have OpenGL (essentially all mac systems have this enabled). It is, but it will take quite some work to get there. I filed bug 676241 on that some time ago, and I have a tiny demo app lying around somewhere which demonstrates that it's possible, but I don't have the time to write the Firefox patches at the moment.

I really think it's worth doing so, both because of the significant complexity required to make the current method work (on top of the existing complexity that exists for it), plus the expected performance regression for the entire browser.

Would that essentially require hardware acceleration then? TenFourFox and the various legacy PowerPC Firefox forks are almost entirely rendered in software. Or is the concept simply to unify the window under one context, which could be a CGContext *or* OpenGL?

The latter.

De-bitrotting

UX would like to test the interaction of tabs in the titlebar on OSX, so I've whipped up a really quick 'n dirty patch that puts the tabs in there.

Just so we're clear, attachment 722823 [details] [diff] [review] is purely to test on the UX branch, and is not intended for mozilla-central ever.

This is the patch that I landed (adds comments about how these changes should not leak into mozilla-central).

Perhaps someone could use this. I discovered a way to move the three buttons down very easily. It's a little hacky, but might work. I covered the process here: http://infinite-josiah.blogspot.com/2013/03/obj-c-hack-to-re-position-window-buttons.html Hopefully with some adoption that could help out here.

Comment on attachment 722830 [details] [diff] [review] Puts tabs in the titlebar on UX branch This temporary UX branch patch may have caused bug 853415. I'm noting that here as its something to look for when we make the real patch for m-c.

I hear rumors that Mike Conley's temporary UX-branch patch for this bug (https://hg.mozilla.org/projects/ux/rev/cc13552f009f) has been removed from that branch. See bug 853415 comment #7 and bug 853415 comment #8. Is this true? And if so which changeset removed it? I'd actually prefer this remained on the UX branch. I've been using it for testing in my work on bug 861317 and bug 851652.

(Following up comment #72) Oh and by the way, I now have a working fix for bug 861317, which I'll post shortly.

(In reply to Steven Michaud from comment #72) > I hear rumors that Mike Conley's temporary UX-branch patch for this bug > (https://hg.mozilla.org/projects/ux/rev/cc13552f009f) has been removed from > that branch. See bug 853415 comment #7 and bug 853415 comment #8. > > Is this true? And if so which changeset removed it? Not exactly - looking through the UX repo, I can still see that patch in there: https://hg.mozilla.org/projects/ux/rev/cc13552f009f > > I'd actually prefer this remained on the UX branch. I've been using it for > testing in my work on bug 861317 and bug 851652. The patch that is missing is, I believe, attachment 722830 [details] [diff] [review]. Apply that, and you should be back to the original, hacky state.

Turns out it was this changeset that turned off drawing in the titlebar: https://hg.mozilla.org/projects/ux/rev/34e823eae687

Updated for current trunk.

Missed something. *Now* it's updated for the current trunk.

According to smichaud, the patch that we're waiting on (bug 851652) still needs more research, and will not be ready for M6. So, slipping to M7.

Removing the items from M7 that do not block landing on m-c.

Since the target milestone is Firefox 28 are there any fixes to verify in Firefox 28 beta or the fixes are for Australis only?

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #84) > Since the target milestone is Firefox 28 are there any fixes to verify in > Firefox 28 beta or the fixes are for Australis only? No, there aren't - while some of the platform-y things exist in Firefox 28, the front-end code that makes use of it is not. TL;DR - no verification required here for 28.

Thanks Mike, dropping verifyme based on comment 85.