At the moment, with hardware acceleration on, we paint window contents through an OpenGL context that covers the whole window except for the title bar. The title bar is painted using traditional NSView drawing into the window buffer. That means we have two window layers (probably NSSurfaces) that are shipped separately to the window server and composited there. If we want to fix the bugs that I've put into the "blocks" list of this bug, we'll probably have to make the title bar part of our GL context, too. In order to get proper rounded corners in the title bar we'll need to make the GL context transparent.

I doubt that there are two NSSurfaces, at least not if you "do it right". The right way to draw OpenGL directly into a NSWindow is to create a NSWindow, then a NSOpenGLContext and then use the setView: method of the NSOpenGLContext and pass the contentView of the NSWindow as parameter. Now the default OpenGL framebuffer (framebuffer 0) draws directly into the backing storage (NSSurface) of the window (limited to the window bounds minus the area covered by the title bar). Basically all OpenGL rendering will now be offscreen rendering into the pixel backing store of the window and only when refreshing the window on screen, the content of the backing store is sent to the window server for compositing. Also note that the context doesn't have to (actually should not be) double buffered, since a NSWindow is doubled buffered anyway and by making the context double buffered, it would in fact be tripple buffered. I think drawing the title bar in OpenGL is a very bad idea. If Apple ever changes the way titlebars look, the titlebar will look wrong. If the user has somehow altered/tweaked the look of his titlebars, the titlebar will not look native to him/her any longer. Drawing window decorations (titlebars, borders, etc.) is task of the operating system (Mac OS, Windows) or the window/desktop mananger (Linux), not of the app. The app is responsible for the title bar of its window and nothing else. I looked at the blocking bugs and all these can be fixed without implementing this feature. I know this, since I recently worked on an OS X project, which also paints all its window content using OpenGL, and even though this was a real PITA in some cases, the project finally suffered by none of the problems described in the blocker bugs. So I can assure you, it is possible. Also you don't have to draw round corners yourself, if you have a window with titlebar and this is much preferable, since even here Apple may change the default corner radius of windows in future releases and here the app would automatically look right, too.

(In reply to TGOS from comment #1) > I doubt that there are two NSSurfaces, at least not if you "do it right". > The right way to draw OpenGL directly into a NSWindow is to create a > NSWindow, then a NSOpenGLContext and then use the setView: method of the > NSOpenGLContext and pass the contentView of the NSWindow as parameter. This is what we do, except that we don't use the contentView directly but a subview of the contentView (which covers the whole contentView). > Now > the default OpenGL framebuffer (framebuffer 0) draws directly into the > backing storage (NSSurface) of the window (limited to the window bounds > minus the area covered by the title bar). This sounds interesting. Can you prove it? When does it make a difference? > Basically all OpenGL rendering > will now be offscreen rendering into the pixel backing store of the window > and only when refreshing the window on screen, the content of the backing > store is sent to the window server for compositing. Also note that the > context doesn't have to (actually should not be) double buffered, since a > NSWindow is doubled buffered anyway and by making the context double > buffered, it would in fact be tripple buffered. As above, is there a way to find out whether that's really the case? > I think drawing the title bar in OpenGL is a very bad idea. If Apple ever > changes the way titlebars look, the titlebar will look wrong. If the user > has somehow altered/tweaked the look of his titlebars, the titlebar will not > look native to him/her any longer. Drawing window decorations (titlebars, > borders, etc.) is task of the operating system (Mac OS, Windows) or the > window/desktop mananger (Linux), not of the app. The app is responsible for > the title bar of its window and nothing else. I see what you're saying. If we can forgo this bug and still fix the ones it's blocking, we probably should. But I don't see how. > I looked at the blocking bugs and all these can be fixed without > implementing this feature. How? Let's go through them one by one: - Bug 635566 apparently is already fixed. - Bug 661856: Are you sure? Maybe your app doesn't have a unified title bar, so the black line wasn't visible under the normal title bar separator line. - Bug 616232: ...How? Also, there's another thing I haven't mentioned yet in this bug's rationale, and that is tabs in the title bar. If we're going to move the tabs into the title bar (and that's the plan), they'll probably be partly in the title bar and partly in the content area. We'll definitely want drawing in those areas to be synchronized, for example during tab opening animations, or the throbber animation. If we draw into both the OpenGL context and the normal window buffer at a high rate, I think we're going to have performance problems.

If anybody wants to pick this bug up, here's a demo app that demonstrates how it could be done. A long time has passed since I wrote it; it's only good for inspirational purposes. The important points are: - The NSView that holds the GL context is no longer a subview of the window's content view, but of its border view, and it's ordered on top of the content view but below the window buttons (which are subviews of the border view, too). This makes it visible in the titlebar, instead of being clipped to the content area. It also makes it accept mouse input in the right areas. - Even though the titlebar buttons are conceptually on top of the GL view, visually they are covered by it. That's because they draw into the window's normal buffer surface which is visually below the OpenGL surface (determined by the default value of NSOpenGLCPSurfaceOrder, which is "GL on top"). In order to make the titlebar buttons visible, they need to be drawn into the GL context which covers them. The test app creates a texture for each, fills it by calling cacheDisplayInRect on the button's NSView and drawing a quad. - The buffer for any of the title bar buttons, or more generally, for any subview of the border view outside our control, needs to be refreshed whenever the view's visuals change. This is done in updateSubviewBuffers, which is called in drawRect and checks whether the affected element intersects the window's dirty region.

Before anybody attempts to work on this bug, the patches of bug 675410 and its dependencies should be fixed and landed.

I built the demo app. On a retina MBP, it shows "large" pixels everywhere: title font, title bar buttons, and of course angles. Maybe Steven and the hdpi guys know how to get around this issue.

I think I'll have time to spend on this (and the other titlebar bugs) next week. Let's see if my reverse engineering skills turn up something interesting.

According to Steven Michaud, this no longer blocks our efforts to draw the tabs in the titlebar performantly.

I've started working on this.

This patch applies on the UX repo minus attachment 744892 [details] [diff] [review] plus attachment 744861 [details] [diff] [review]. Except for drawing glitches in the titlebar corners it mostly works.

This is awesome - I just tested this, and with this patch I can't reproduce any of the glitches in bug 868107. Not only that, but the whole thing just feels more responsive. Steven - will this make moving the window buttons down easier?

> Steven - will this make moving the window buttons down easier? It might. I'll give it a try. I'll also check for performance problems.

This fixes the corner drawing glitches and the problem that you could no longer resize the window with the inner window edges. Still needs cleanup, and still need to fix the programtype bug (marked with XXX debug).

> Steven - will this make moving the window buttons down easier? I still don't know, but I suspect the answer is "no". A straight port of my partial patch for bug 851652 (attachment 733092 [details] [diff] [review]) doesn't work in several ways. I'll probably be able to adapt it somewhat, but I suspect the central problem will remain -- that it's difficult to get the parts of the titlebar buttons to draw that should appear "above" our browser window's content frame (the part below the top 22 pixels). So ... back to the salt mines :-)

(Following up comment #13) By the way, my "port" of my partial fix for bug 851652 made it conform to Markus' patch for bug 868211.

Now with support for moved titlebar buttons. Steven, you can try your ported patch for bug 851652 again with this patch. The difference is that instead of only including the top 22px of the window in the titlebar buffer, I size it to RectContainingTitlebarControls() which automatically allocates space for moved buttons.

The buttons do initially display in the correct (lowered) locations with your new patch, Markus. They didn't do this before. And that it's even possible is a big relief. But the first time you resize the window they jump back to their original (incorrect) locations. Next week I'll try to find ways to alter your patch so this no longer happens.

New version which fixes all bugs I'm aware of, updates comments, and even works reasonably well in OMTC mode. The next step is to split this patch up into smaller reviewable pieces.

This patch is on top of the ones in bug 871590 on mozilla-central. Since we area going to apply our own rounded corner masking in the window corners, it's better if the source has square corners so that different rounded corner masks don't interact badly with each other.

This code will become unnecessary.

The reason why our drawing (both OpenGL and CGContext) didn't show in the titlebar before was the fact that it was clipped by the contentView bounds: Our mainChildView is a subview of the window's contentView which is automatically sized to not cover the titlebar. By overriding the private methods -[NSWindow contentRectForFrameRect:styleMask:] and -[NSWindow frameRectForContentRect:styleMask:], we make the automatic content view sizing do what we want. These are the methods that NSWindow calls internally when determining the size of the content view. We also make sure that the contentView is below the window buttons so that they can still be clicked.

This patch applies the optimizations from bug 676250 to accelerated OMTC drawing, too.

Benoit, I'm requesting review from you because you reviewed bug 675410 and this patch is similar. Let me know if somebody else should review it. This patch is the main part of this bug. When the contentView is extended into the titlebar, our rendering no longer benefits from the automatic window frame effects that NSThemeFrame background rendering gave us, specifically the highlight line and the rounded top corners. Now we have to apply them manually. Additionally, in OpenGL mode we also have to draw the titlebar controls into the OpenGL context.

Comment on attachment 748887 [details] [diff] [review] part 6: correct titlebar drawing I should mention that this patch only works correctly when the patch from bug 861332 is applied.

This just completes the backout of bug 820839.

Here's a try run with these patches (minus a few tweaks) on mozilla-central: https://tbpl.mozilla.org/?tree=Try&rev=03f647e660fe I'd like to land the patches on mozilla-central and not only on the UX repo because they interact with drawing stuff which can bitrot quickly. There will be one regression I'm aware of if we take them on mozilla-central: When a lightweight theme is applied, the window title will no longer be drawn. (That's because we cover it with the window contents and don't draw it on top again.) In the future we want this behavior anyway when the tabs are moved into the titlebar.

Comment on attachment 748887 [details] [diff] [review] part 6: correct titlebar drawing Review of attachment 748887 [details] [diff] [review]: ----------------------------------------------------------------- r- since I'd like to see the revision to mDirtyRegion. ::: widget/cocoa/nsChildView.mm @@ +1825,5 @@ > > void > +nsChildView::NotifyDirtyRegion(const nsIntRegion& aDirtyRegion) > +{ > + if ([(ChildView*)mView isCoveringTitlebar]) { Why do you only update the dirty region if the view covers the titlebar? Perhaps this means that mDirtyRegion doesn't really described what you're tracking. @@ +1828,5 @@ > +{ > + if ([(ChildView*)mView isCoveringTitlebar]) { > + // We store the dirty region so that we know what to repaint in the titlebar. > + mDirtyRegion.Or(mDirtyRegion, aDirtyRegion); > + mDirtyRegion.SimplifyOutward(8); Why do you SimplifyOutward(8) here? Why 8? It might be worth documenting. @@ +1893,5 @@ > } > > + manager->gl()->PushScissorRect(aRect); > + > + MaybeDrawTitlebar(manager, aRect); The title bar drawing will need to be made thread safe so that OMTC can draw the titlebar from the compositor thread. This doesn't need to happen here but we shouldn't introduce anything that would make this harder.

(In reply to Benoit Girard (:BenWa) from comment #29) > ::: widget/cocoa/nsChildView.mm > @@ +1825,5 @@ > > > > void > > +nsChildView::NotifyDirtyRegion(const nsIntRegion& aDirtyRegion) > > +{ > > + if ([(ChildView*)mView isCoveringTitlebar]) { > > Why do you only update the dirty region if the view covers the titlebar? Because it isn't used in that case. > Perhaps this means that mDirtyRegion doesn't really described what you're > tracking. I agree, it is a bit yucky. All I really need to know is the dirty region in the titlebar. I could apply the intersection with the titlebar rect, which I'm currently doing in PrepareWindowEffects, already here in NotifyDirtyRegion. But I'd still have to do it in PrepareWindowEffects, too, because the titlebar rect might have changed in the meantime. I wanted to avoid doing the work (i.e. computation of mTitlebarRect and intersection with it) twice. What do you think I should do? > > @@ +1828,5 @@ > > +{ > > + if ([(ChildView*)mView isCoveringTitlebar]) { > > + // We store the dirty region so that we know what to repaint in the titlebar. > > + mDirtyRegion.Or(mDirtyRegion, aDirtyRegion); > > + mDirtyRegion.SimplifyOutward(8); > > Why do you SimplifyOutward(8) here? Why 8? It might be worth documenting. Out of habit. I'll remove it. It's not necessary because this region is already very likely to be simple enough - it's either the whole window (after resizes), or an invalidation from Gecko (which has already been SimplifyOutward(8)ed in nsViewManager.cpp) or an invalidation from Cocoa due to hovered or clicked titlebar buttons, or a combination thereof. > @@ +1893,5 @@ > > } > > > > + manager->gl()->PushScissorRect(aRect); > > + > > + MaybeDrawTitlebar(manager, aRect); > > The title bar drawing will need to be made thread safe so that OMTC can draw > the titlebar from the compositor thread. This doesn't need to happen here > but we shouldn't introduce anything that would make this harder. Everything except the subview traversal and the calls of drawRect on the titlebar buttons should already be thread safe. And these things are only risky when we paint during window destruction, or maybe when window subviews are shuffled around during full screen mode transitions.

(In reply to Markus Stange from comment #30) > (In reply to Benoit Girard (:BenWa) from comment #29) > > ::: widget/cocoa/nsChildView.mm > > @@ +1825,5 @@ > > > > > > void > > > +nsChildView::NotifyDirtyRegion(const nsIntRegion& aDirtyRegion) > > > +{ > > > + if ([(ChildView*)mView isCoveringTitlebar]) { > > > > Why do you only update the dirty region if the view covers the titlebar? > > Because it isn't used in that case. Uhm, I meant "Because it's only used in that case." - it's ignored when we're not drawing in the titlebar.

(In reply to Markus Stange from comment #30) > (In reply to Benoit Girard (:BenWa) from comment #29) > > ::: widget/cocoa/nsChildView.mm > > @@ +1825,5 @@ > > > > > > void > > > +nsChildView::NotifyDirtyRegion(const nsIntRegion& aDirtyRegion) > > > +{ > > > + if ([(ChildView*)mView isCoveringTitlebar]) { > > > > Why do you only update the dirty region if the view covers the titlebar? > > Because it isn't used in that case. > > > Perhaps this means that mDirtyRegion doesn't really described what you're > > tracking. > > I agree, it is a bit yucky. All I really need to know is the dirty region in > the titlebar. I could apply the intersection with the titlebar rect, which > I'm currently doing in PrepareWindowEffects, already here in > NotifyDirtyRegion. But I'd still have to do it in PrepareWindowEffects, too, > because the titlebar rect might have changed in the meantime. I wanted to > avoid doing the work (i.e. computation of mTitlebarRect and intersection > with it) twice. What do you think I should do? > We should just use a more descriptive name perhaps like mTitleBarDirtyRegion. With a name like mDirtyRegion I can see someone using it to mean the real dirty region and get confused because it doesn't always work correctly. The part about the titlebar height changing isn't obvious. Is that documented? Perhaps we should add it in the declaration for mTitleBarDirtyRegion then it will be clear why we're And-ing with the titlebar at the end.

(In reply to Benoit Girard (:BenWa) from comment #32) > (In reply to Markus Stange from comment #30) > > (In reply to Benoit Girard (:BenWa) from comment #29) > > > ::: widget/cocoa/nsChildView.mm > > > @@ +1825,5 @@ > > > > > > > > void > > > > +nsChildView::NotifyDirtyRegion(const nsIntRegion& aDirtyRegion) > > > > +{ > > > > + if ([(ChildView*)mView isCoveringTitlebar]) { > > > > > > Why do you only update the dirty region if the view covers the titlebar? > > > > Because it isn't used in that case. > > > > > Perhaps this means that mDirtyRegion doesn't really described what you're > > > tracking. > > > > I agree, it is a bit yucky. All I really need to know is the dirty region in > > the titlebar. I could apply the intersection with the titlebar rect, which > > I'm currently doing in PrepareWindowEffects, already here in > > NotifyDirtyRegion. But I'd still have to do it in PrepareWindowEffects, too, > > because the titlebar rect might have changed in the meantime. I wanted to > > avoid doing the work (i.e. computation of mTitlebarRect and intersection > > with it) twice. What do you think I should do? > > > > We should just use a more descriptive name perhaps like > mTitleBarDirtyRegion. Well, I already have both: mDirtyRegion and mDirtyTitlebarRegion. In the current patch, the intersection is not stored in mDirtyRegion, only in mDirtyTitlebarRegion. > With a name like mDirtyRegion I can see someone using > it to mean the real dirty region and get confused because it doesn't always > work correctly. At the moment it does contain the whole dirty region - until it's .SetEmpty()'d in PrepareWindowEffects. > The part about the titlebar height changing isn't obvious. > Is that documented? Perhaps we should add it in the declaration for > mTitleBarDirtyRegion then it will be clear why we're And-ing with the > titlebar at the end. I was more concerned about the width changing, not the height, but I think the case I was worried about can't happen. When the window width changes, I think we always enter NotifyDirtyRegion (through synchronous resize painting) before we enter PrepareWindowEffects (from a refresh driver tick). But I'm not sure if we should rely on that. I can change the patch to get rid of mDirtyRegion and only modify mDirtyTitlebarRegion. mDirtyTitlebarRegion is accessed by the compositor thread, so I'll have to take mEffectsLock in NotifyDirtyRegion.

(In reply to Markus Stange from comment #33) > (In reply to Benoit Girard (:BenWa) from comment #32) > > (In reply to Markus Stange from comment #30) > > > (In reply to Benoit Girard (:BenWa) from comment #29) > > > > ::: widget/cocoa/nsChildView.mm > > > > @@ +1825,5 @@ > > > > > > > > > > void > > > > > +nsChildView::NotifyDirtyRegion(const nsIntRegion& aDirtyRegion) > > > > > +{ > > > > > + if ([(ChildView*)mView isCoveringTitlebar]) { > > > > > > > > Why do you only update the dirty region if the view covers the titlebar? > > > > > > Because it isn't used in that case. > > > > > > > Perhaps this means that mDirtyRegion doesn't really described what you're > > > > tracking. > > > > > > I agree, it is a bit yucky. All I really need to know is the dirty region in > > > the titlebar. I could apply the intersection with the titlebar rect, which > > > I'm currently doing in PrepareWindowEffects, already here in > > > NotifyDirtyRegion. But I'd still have to do it in PrepareWindowEffects, too, > > > because the titlebar rect might have changed in the meantime. I wanted to > > > avoid doing the work (i.e. computation of mTitlebarRect and intersection > > > with it) twice. What do you think I should do? > > > > > > > We should just use a more descriptive name perhaps like > > mTitleBarDirtyRegion. > > Well, I already have both: mDirtyRegion and mDirtyTitlebarRegion. In the > current patch, the intersection is not stored in mDirtyRegion, only in > mDirtyTitlebarRegion. > > > With a name like mDirtyRegion I can see someone using > > it to mean the real dirty region and get confused because it doesn't always > > work correctly. > > At the moment it does contain the whole dirty region - until it's > .SetEmpty()'d in PrepareWindowEffects. Except if the view doesn't intersect with the titlebar, adding to the confussion. > > > The part about the titlebar height changing isn't obvious. > > Is that documented? Perhaps we should add it in the declaration for > > mTitleBarDirtyRegion then it will be clear why we're And-ing with the > > titlebar at the end. > > I was more concerned about the width changing, not the height, but I think > the case I was worried about can't happen. When the window width changes, I > think we always enter NotifyDirtyRegion (through synchronous resize > painting) before we enter PrepareWindowEffects (from a refresh driver tick). > But I'm not sure if we should rely on that. > > I can change the patch to get rid of mDirtyRegion and only modify > mDirtyTitlebarRegion. mDirtyTitlebarRegion is accessed by the compositor > thread, so I'll have to take mEffectsLock in NotifyDirtyRegion. That would work.

(In reply to Benoit Girard (:BenWa) from comment #34) > Except if the view doesn't intersect with the titlebar, adding to the > confussion. Oops, right. > > I can change the patch to get rid of mDirtyRegion and only modify > > mDirtyTitlebarRegion. mDirtyTitlebarRegion is accessed by the compositor > > thread, so I'll have to take mEffectsLock in NotifyDirtyRegion. > > That would work. Here you are.

Comment on attachment 748884 [details] [diff] [review] part 5: tweak GL OMTC handling Review of attachment 748884 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsChildView.mm @@ +2854,5 @@ > + } else if (mUsingOMTCompositor) { > + // XXX If this is a Cocoa-initiated repaint (for example due to hovered > + // window buttons), we should enforce a recomposition on the compositor > + // thread somehow. Otherwise, the new button hover appearance will only > + // become visible on the next Gecko-initiated composition. We still need to call mGeckoChild->PaintWindow() (to get the DidPaintWindow call) otherwise we fail reftests.

Comment on attachment 748892 [details] [diff] [review] part 8: remove alternate layer manager handling Review of attachment 748892 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! There's a bunch of 'secondary layer manager' stuff in FrameLayerBuilder too, but we can get rid of that as a follow-up.

Comment on attachment 748991 [details] [diff] [review] part 6: correct titlebar drawing + // Add the rects of the titlebar controls. + for (id view in [(BaseWindow*)[mView window] titlebarControls]) { + rect = NSUnionRect(rect, [mView convertRect:[view bounds] fromView:view]); + } A nit about the "for (id view in NSArray ..." syntax: I'm not sure how far back this is supported (in XCode versions), so I've been keeping this "new" syntax out of the tree. It's probably best if you go back to the "for (int i = 0; i < arrayCount; ++i) id view = [array objectAtIndex:i]; ..." syntax. As I understand it, we still need to build on OS X 10.6. And in fact some tinderbox mochitest builds are still done on 10.6. The last XCode version available for SnowLeopard seems to be 4.2, but I'm not sure what version those SnowLeopard tinderbox machines use. We can probably stop worrying about this when we drop support for SnowLeopard.

Or use an NSEnumerator which has been around since a very long time. As far as I know the for (id view in ...) is only syntactic sugar over the NSEnumerator construct. http://cocoadev.com/wiki/NSEnumerator has some more insight into this.

> Or use an NSEnumerator ... Yes, that'd be fine too.

Bug 661661 gave the green light for fast enumeration about two years ago. According to Wikipedia, Objective C 2 is supported starting with XCode 3.0, which was released with Mac OS 10.5. Our 10.6 build slaves use clang 3.2 since bug 823906 - they have "clang_version": "r170890" in their build logs.

Oh well, I guess I stand corrected. Do a tryserver run to check that there aren't any problems.

By the way, Markus, what order do you advise these patches be applied in?

(In reply to Steven Michaud from comment #42) > Do a tryserver run to check that there aren't any problems. Sure, already done: https://tbpl.mozilla.org/?tree=Try&rev=03f647e660fe&showall=true (In reply to Steven Michaud from comment #43) > By the way, Markus, what order do you advise these patches be applied in? On mozilla-central: https://bugzilla.mozilla.org/attachment.cgi?id=746897 https://bugzilla.mozilla.org/attachment.cgi?id=748859 https://bugzilla.mozilla.org/attachment.cgi?id=748866 https://bugzilla.mozilla.org/attachment.cgi?id=748870 https://bugzilla.mozilla.org/attachment.cgi?id=748874 https://bugzilla.mozilla.org/attachment.cgi?id=748876 https://bugzilla.mozilla.org/attachment.cgi?id=748883 https://bugzilla.mozilla.org/attachment.cgi?id=748884 https://bugzilla.mozilla.org/attachment.cgi?id=748991 https://bugzilla.mozilla.org/attachment.cgi?id=748889 https://bugzilla.mozilla.org/attachment.cgi?id=748892 You can test by applying a lightweight theme. I think it's best if I wait until the patches are reviewed before I create versions of them which apply on the UX branch.

> You can test by applying a lightweight theme. > > I think it's best if I wait until the patches are reviewed before I > create versions of them which apply on the UX branch. OK then, I'll test on the trunk. (I'd been trying to apply them to the UX branch.) So the order to apply your patches is as follows: 1) Patch v1 from bug 861332 comment #5 2) Patch part1 from bug 871590 comment #1 3) Parch part2 from bug 871590 comment #2 4) Patch part1 from this bug 5) Patch part2 from this bug 6) Patch part3 from this bug 7) Patch part4 from this bug 8) Patch part5 from this bug 9) Patch part6 from this bug 10) Patch part7 from this bug 11) Patch part8 from this bug Right? :-)

Right :-)

I've tested the debug builds of your try run. The good news: it's awesome :D. But i can crash it with OMTC enabled. STR: 1. enable OMTC a) layers.offmainthreadcomposition.animate-opacity, true b) layers.offmainthreadcomposition.animate-transform, true c) layers.offmainthreadcomposition.enabled, true d) layers.offmainthreadcomposition.testing.enabled, true 2. go to http://ie.microsoft.com/testdrive/Performance/RoboHornetPro/ 3. crash I know that OMTC is still in development, but with the m-c nightly it runs very smooth and without any crash. Thanks for this hard work!

(In reply to comment #47) I can't reproduce this (running my own build on OS X 10.7.5, on a Retina MBP). What OS version did you test on, and what hardware?

(Following up comment #48) OK, I *do* crash with the 10.7 debug build from https://tbpl.mozilla.org/?tree=Try&rev=03f647e660fe&showall=true, though not with the opt build. Just visiting http://ie.microsoft.com/testdrive/Performance/RoboHornetPro/ is enough -- I don't have to start RoboHornet. Unfortunately the debug build has its symbols stripped, so gdb stack traces are meaningless. I'll see if I can come up with a build that crashes and *doesn't* have its symbols stripped.

(In reply to Steven Michaud from comment #49) Yes, I can't reproduce this with the opt build myself. Just for reference: Hardware: - Mac mini (Mid 2011 - SandyBridge / HD3000) with OS X 10.8.3 - MacBook Pro (Mid 2012 - IvyBridge / HD4000) with OS X 10.8.3

The RoboHornet crashes have nothing to do with this patch, and happen whether or not drawing in the titlebar is turned on. I see them in standard mozilla-central builds (with the STR from comment #47) whose mozconfigs have the following lines: ac_add_options --enable-debug ac_add_options --enable-trace-malloc ac_add_options --enable-accessibility ac_add_options --disable-strip ac_add_options --disable-install-strip I strongly suspect OMTC and trace-malloc don't get along. (trace-malloc isn't enabled in the 10.7 opt build from https://tbpl.mozilla.org/?tree=Try&rev=03f647e660fe&showall=true.) I'll open a bug on the subject later today ... if no-one else beats me to it.

I've opened bug 872678.

Just a progress report, since my review is taking longer than expected. The part4 patch Markus asked me to review looks fine, but I will have some changes to suggest. The part6 patch also basically looks fine. But I'm looking into the consequences of calling -[NSView drawRect:] (on the titlebar buttons) without ever calling -[NSView lockFocus] and -[NSView unlockFocus], and also of calling -[NSView drawRect:] on a secondary thread (again on the titlebar buttons, when OMTC is on).

(In reply to Steven Michaud from comment #53) > The part6 patch also basically looks fine. But I'm looking into the > consequences of calling -[NSView drawRect:] (on the titlebar buttons) > without ever calling -[NSView lockFocus] and -[NSView unlockFocus], and also > of calling -[NSView drawRect:] on a secondary thread (again on the titlebar > buttons, when OMTC is on). These are good points. For the first point, I hope that it does what we want it to: simply draw into the current NSGraphicsContext. As far as I know, -[NSView lockFocus] sets up a graphics context that draws into the window, and that's exactly what we want to avoid. I've changed my mind on the OMTC case. I don't think it would cause any problems as is, but drawing the titlebar buttons on the main thread isn't too hard, so I'll change the patch to do that. The titlebar buttons return NO for canDrawConcurrently.

Comment on attachment 748883 [details] [diff] [review] part 4: make the window's contentView cover the whole window in drawsContentsIntoWindowFrame mode This look fine to me. But I recommend changes I'll post in my next comment.

Your patch overrides the following undocumented NSWindow instance methods, as it should (since the OS can call them): - (NSRect)contentRectForFrameRect:(NSRect)aRect styleMask:(NSUInteger)aMask - (NSRect)frameRectForContentRect:(NSRect)aRect styleMask:(NSUInteger)aMask But it should also call them on super, if super implements them. The only other thing "missing" from your patch is code to override the corresponding NSWindow class methods: + (NSRect)contentRectForFrameRect:(NSRect)aRect styleMask:(NSUInteger)aMask + (NSRect)frameRectForContentRect:(NSRect)aRect styleMask:(NSUInteger)aMask Though of course there'd be no point in this, since -[BaseWindow drawsContentsIntoWindowFrame] is an instance method. Apple's docs on the class methods say you should use the instance methods if an NSWindow instance is available. And Apple's own code seems to follow this rule. So we should be safe not implementing the class methods in BaseWindow, even though this means the corresponding NSWindow class methods will occasionally be called instead, and potentially return the "wrong" answer. You probably already concluded this on your own. But I just wanted to make explicit mention of it.

> As far as I know, -[NSView lockFocus] sets up a graphics context > that draws into the window, and that's exactly what we want to > avoid. You're right about this on both counts. I used Hopper Disassembler (http://www.hopperapp.com/) and an interpose library to find out that (at least on OS X 10.7.X) -[NSView lockFocus] always (indirectly) calls +[NSGraphicsContext setCurrentContext:] to set the current context to an NSWindowGraphicsContext object associated with the NSView instance's NSWindow object (its "_threadContext"). But, though (as I also found) you can get away with calling -[NSView drawRect:] on NSControl objects without locking the focus (as your code currently does), this is bad form, and potentially very confusing to others reading your code. Apple's doc on -[NSView drawRect:] says that "the receiver can assume the focus has been locked ...". And in any case we don't need to call -[NSControl drawRect:]. I used Hopper Disassembler to look at AppKit's implementation of -[NSControl drawRect:] on SnowLeopard, Lion and MountainLion. The code is the same on all platforms, and is a pretty thin wrapper around a call to -[NSCell drawWithFrame:inView:], which we've been using successfully for years in nsNativeThemeCocoa.mm. There are a couple things that -[NSControl drawRect:] does before it calls -[NSCell drawWithFrame:inView:] that my suggested changes don't include. But I suspect they're not needed. And we can revisit this issue if we notice problems drawing the titlebar buttons.

I saw one more issue, which I'll leave unresolved for now: The OS can draw the titlebar buttons (call -[NSControl drawRect:] on them "in the proper channels") without nsChildView::UpdateTitlebarImage() redrawing them. Thanks to what part4 of your patch does in -[BaseWindow setContentView:], the titlebar buttons are always above BaseWindow's "main child view". So redrawing them doesn't make any part of that ChildView dirty, and doesn't result in call(s) to -[ChildView drawRect:] or to nsChildView::NotifyDirtyRegion(). This doesn't make any noticeable difference with OMTC off. But with OMTC on (at least without your part9 patch) the titlebar buttons update a second or two late when you hover the mouse over them. This should be easy to fix once I land a patch for bug 851652. We could just add code to hook the titlebar buttons' -[NSView setNeedsDisplayInRect:] methods (or something similar). Those methods could either call -[NSView setNeedsDisplayInRect:] on the appropriate part of the main child view, or perhaps just call nsChildView::NotifyDirtyRegion() directly. I'll open a new bug on this when I get the chance.

(In reply to Steven Michaud from comment #57) > Created attachment 750610 [details] [diff] [review] > part 4 recommended changes > > Your patch overrides the following undocumented NSWindow instance methods, > as it should (since the OS can call them): > > - (NSRect)contentRectForFrameRect:(NSRect)aRect styleMask:(NSUInteger)aMask > - (NSRect)frameRectForContentRect:(NSRect)aRect styleMask:(NSUInteger)aMask > > But it should also call them on super, if super implements them. OK. I agree with your conclusions in the rest of the comment. (In reply to Steven Michaud from comment #58) > Created attachment 750626 [details] [diff] [review] > part 6 recommended changes > > > As far as I know, -[NSView lockFocus] sets up a graphics context > > that draws into the window, and that's exactly what we want to > > avoid. > > You're right about this on both counts. I used Hopper Disassembler > (http://www.hopperapp.com/) This tool looks very interesting. Thanks, I hadn't heard about it before. > and an interpose library to find out that > (at least on OS X 10.7.X) -[NSView lockFocus] always (indirectly) > calls +[NSGraphicsContext setCurrentContext:] to set the current > context to an NSWindowGraphicsContext object associated with the > NSView instance's NSWindow object (its "_threadContext"). > > But, though (as I also found) you can get away with calling -[NSView > drawRect:] on NSControl objects without locking the focus (as your > code currently does), this is bad form, and potentially very confusing > to others reading your code. Apple's doc on -[NSView drawRect:] says > that "the receiver can assume the focus has been locked ...". Well, they don't say it explicitly, but I read this as "the receiver can assume that the graphics context has been set up in a way that makes sense, which for usual direct-to-the-window drawing means that lockFocus will have been called". And it's exactly this assumption that we use: drawRect: doesn't have to do any locking, so we can use drawRect: to draw into any context we like. > And in > any case we don't need to call -[NSControl drawRect:]. > > I used Hopper Disassembler to look at AppKit's implementation of > -[NSControl drawRect:] on SnowLeopard, Lion and MountainLion. The > code is the same on all platforms, and is a pretty thin wrapper around > a call to -[NSCell drawWithFrame:inView:], which we've been using > successfully for years in nsNativeThemeCocoa.mm. > > There are a couple things that -[NSControl drawRect:] does before it > calls -[NSCell drawWithFrame:inView:] that my suggested changes don't > include. But I suspect they're not needed. And we can revisit this > issue if we notice problems drawing the titlebar buttons. I disagree. We can use the same argument for the drawRect approach: I suspect going the route via NSCell isn't needed, and we can revisit the issue if we notice problems. The drawRect approach doesn't use the private -[NSButton cell] method and doesn't rely on the fact that the NSViews in the titlebar are NSButtons. Is your objection to the drawRect approach only due to the reference to lockFocus in the docs? (In reply to Steven Michaud from comment #59) > I saw one more issue, which I'll leave unresolved for now: > > The OS can draw the titlebar buttons (call -[NSControl drawRect:] on them > "in the proper channels") without nsChildView::UpdateTitlebarImage() > redrawing them. > > Thanks to what part4 of your patch does in -[BaseWindow setContentView:], > the titlebar buttons are always above BaseWindow's "main child view". So > redrawing them doesn't make any part of that ChildView dirty, and doesn't > result in call(s) to -[ChildView drawRect:] or to > nsChildView::NotifyDirtyRegion(). I think this is incorrect. When a button is hovered, its rect is saved in the NSWindow's dirty region. When these rects are drawn, Cocoa will start at the nearest opaque superview and draw the whole NSView subtree which intersects with the dirty region. The titlebar buttons are transparent - their NSViews return NO for isOpaque. Our view is under the titlebar buttons, so it's redrawn first, and then the titlebar buttons are drawn on top. > This doesn't make any noticeable difference with OMTC off. But with OMTC on > (at least without your part9 patch) the titlebar buttons update a second or > two late when you hover the mouse over them. This bug happens in a different way. In OMTC mode, hovering the titlebar buttons calls NotifyDirtyRect with the correct values. But MaybeDrawTitlebar doesn't paint their updated state right away because nothing triggers a call to MaybeDrawTitlebar. The OMTC thread only recomposites the window (and calls DrawWindowOverlay) when Gecko tells it that it's necessary. This is the part that is not hooked up yet. I refer to that problem in the comment in drawUsingOpenGL added by part 5.

Comment on attachment 750626 [details] [diff] [review] part 6 recommended changes > Well, they don't say it explicitly, but I read this as "the receiver > can assume that the graphics context has been set up in a way that > makes sense, which for usual direct-to-the-window drawing means that > lockFocus will have been called". Apple's docs are often ambiguous. But this interpretation is really pushing it. My interpretation is much simpler, and much more likely to be how most people interpret what Apple says. > the private -[NSButton cell] method -[NSCell drawWithFrame:inView:] is *not* private. And like I said we've been using it for years in nsNativeThemeCocoa.mm. > Is your objection to the drawRect approach only due to the reference > to lockFocus in the docs? Both of our approaches rely on undocumented behavior, and would probably have to be revisited if Apple makes big changes in how it manages the titlebar buttons, or adds new content to the titlebar. But your approach is more extreme, which is why I don't like it. At the very least you'd need to write a detailed comment in the code explaining why you use drawRect: without first locking the focus, and why this even works. Given Apple's docs, and how most of Apple's own code behaves, it should be very startling to an experienced Cocoa programmer that you can use drawRect: without locking and unlocking the view focus. My approach is functionally equivalent, and much easier for someone reading the code to understand.

WRT to what you say about what I said in comment #59: You may be right. I'll look more carefully into the problem before I open a new bug.

> I used Hopper Disassembler (http://www.hopperapp.com/) > > This tool looks very interesting. Thanks, I hadn't heard about it before. I only discovered it myself a few months ago, when I realized that gdb just doesn't cut it as a disassembler, and started looking for alternatives. I think it's quite new. Do you know any decent disassemblers for OS X? Ones that handle cross-references reasonably well? I'd particularly like to know if there are any open-source ones. But I did look pretty hard a few months ago, and wasn't able to find any.

(Following up comment #61) > My approach is functionally equivalent, and much easier for someone > reading the code to understand. Actually, now that I think more about it, my case is much stronger than this. Both our approaches make assumptions about undocumented behavior. But my approach makes those assumptions explicit, while your's doesn't. My approach explicitly only works with titlebar buttons that are NSButton objects. But it should continue to work properly with NSButton objects that behave as NSButton objects are documented to behave. So if Apple changes them to no longer be NSButton objects, or adds new objects to the titlebar that aren't NSButton objects, they'll just stop being displayed. Your approach makes hidden assumptions that it's dealing with NSControl objects, and that it will continue to be safe to call drawRect: on them without locking the display focus. But it doesn't do any checking to see these assumptions are followed. So if Apple breaks your assumptions, your patch is liable to fail in unpredictable and potentially catastrophic ways.

Question on the landing of this -- will this go straight to mozilla-central (and work fine with today's theme), or will it need to ride with other Australis changes?

I think this can go straight to mozilla-central. But it does depend on two other bugs -- bug 861332 and bug 871590.

(In reply to Steven Michaud from comment #61) > > the private -[NSButton cell] method > > -[NSCell drawWithFrame:inView:] is *not* private. And like I said > we've been using it for years in nsNativeThemeCocoa.mm. I actually was referring to the "cell" method, which I didn't see on NSButton and thus assumed to be private. But I realize now that it's a (public) method of NSControl. (In reply to Steven Michaud from comment #63) > Do you know any decent disassemblers for OS X? No, sorry, but I haven't looked. (In reply to Steven Michaud from comment #64) > (Following up comment #61) > > > My approach is functionally equivalent, and much easier for someone > > reading the code to understand. > > Actually, now that I think more about it, my case is much stronger > than this. > > Both our approaches make assumptions about undocumented behavior. But > my approach makes those assumptions explicit, while your's doesn't. I agree with that argument and will make the changes you suggested. Another argument against the drawRect approach is that it pretends to be a generic NSView hierarchy drawing implementation (like a replacement for -[NSWindow display]), but it doesn't actually descend into subviews of the views it draws. So it's actually not as generic as it pretends.

There was one remaining problem with OMTC. The patches here rely on the following order of calls: 1. drawRect which calls NotifyDirtyRegion 2. PrepareWindowEffects which updates the titlebar image buffer 3. DrawWindowOverlay which uploads the titlebar image buffer to a texture and paints. Unfortunately, PrepareWindowEffects was called during viewWillDraw, so before drawRect. This patch moves the call to PrepareWindowEffects to the same places that MakeSnapshot is called in. Does this make sense? Or should I move the titlebar image buffer updating step into NotifyDirtyRegion?

A try run with updated patches is at https://tbpl.mozilla.org/?tree=Try&rev=bb754ca35e31 .

Comment on attachment 752761 [details] [diff] [review] part 10: call PrepareWindowEffects during DrawWindow, not before This doesn't actually do what it's supposed to.

Actually it did work, but only with this change to nsPresShell.cpp from the patch in bug 873944. This feels a little like a hack and I'm not sure that I'm not triggering two compositions per OS-caused repaint - one from viewWillDraw and one from drawRect.

There is another way how the call order I mentioned in comment 68 can be screwed up: If a composition on the compositor thread happens between NotifyDirtyRegion and PrepareWindowEffects (during the same drawRect call), mDirtyTitlebarRegion will be cleared by UpdateTitlebarImage on the compositor thread before the main thread can update mTitlebarImageBuffer in PrepareWindowEffects. I'm fixing this by having separate variables for "region which needs to be repainted in mTitlebarImageBuffer on the main thread" and "region which was repainted in mTitlebarImageBuffer and needs to be uploaded to mTitlebarImage on the compositor thread" - the former is called mDirtyTitlebarRegion and the latter is called mUpdatedTitlebarRegion.

New try run which should fix the crashes in the previous one: https://tbpl.mozilla.org/?tree=Try&rev=bbb7079dd772

(In reply to Steven Michaud from comment #63) > Do you know any decent disassemblers for OS X? You might try asking the JS folks in #jsapi... ISTR they were looking a such things a year or so ago (because JITting), but I don't know if any of that was OS X specific.

Comment on attachment 752891 [details] [diff] [review] part 11: fix race condition with mDirtyTitlebarRegion Review of attachment 752891 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsChildView.h @@ +636,5 @@ > nsRefPtr<mozilla::gl::TextureImage> mCornerMaskImage; > nsRefPtr<mozilla::gl::TextureImage> mTitlebarImage; > > + // Main thread only > + nsIntRegion mDirtyTitlebarRegion; This whole section is a bit short on comments, but it would be nice to clarify here what these two region are for exactly. Something like: // The area of mTitlebarImageBuffer that needs to be redrawn during the next transaction and // The area of mTitlebarImageBuffer that has changed and needs to be uploaded to to mTitlebarImage

(In reply to Markus Stange from comment #71) > Created attachment 752886 [details] [diff] [review] > part 10: do extra empty transaction during drawRect which calls > PrepareWindowEffects > > Actually it did work, but only with this change to nsPresShell.cpp from the > patch in bug 873944. This feels a little like a hack and I'm not sure that > I'm not triggering two compositions per OS-caused repaint - one from > viewWillDraw and one from drawRect. Yeah, it probably will, which would regress android. Without the patches from bug 873944, we're still getting the drawRect equivalent calls for every gecko invalidation as well as the OS initiated ones. We have this issue with bug 873944 as well, but it's much rarer (though I guess I should fix that). Can we instead handle NotifyDirtyRegion inside of PrepareWindowEffects? The idea is that PrepareWindowEffects is called at the same time as we paint contents into ThebesLayers and forward them to the compositor. Code inside drawRect probably won't run often enough since will never be calling setNeedsDisplayInRect.

Actually, that's not right. drawRect will be called correctly (with the other patches), since the OS is what's invalidating the titlebar. I think what we really want here is to *not* do painting inside of viewWillDraw, and instead have the drawRect path do a synchronous draw and composite. We can't change the behaviour of non-OMTC though. I'll work on getting my bug 873944 patches to do that, and get them landed, then this should all be much simpler.

(In reply to Matt Woodrow (:mattwoodrow) from comment #76) > (In reply to Markus Stange from comment #71) > > Created attachment 752886 [details] [diff] [review] > > part 10: do extra empty transaction during drawRect which calls > > PrepareWindowEffects > > > > Actually it did work, but only with this change to nsPresShell.cpp from the > > patch in bug 873944. This feels a little like a hack and I'm not sure that > > I'm not triggering two compositions per OS-caused repaint - one from > > viewWillDraw and one from drawRect. > > Yeah, it probably will, which would regress android. Without the patches > from bug 873944, we're still getting the drawRect equivalent calls for every > gecko invalidation as well as the OS initiated ones. We have this issue with > bug 873944 as well, but it's much rarer (though I guess I should fix that). > > Can we instead handle NotifyDirtyRegion inside of PrepareWindowEffects? Well, do we want PrepareWindowEffects to rely on the fact that it's only called during viewWillDraw or drawRect? Actually, what we could do is to call NotifyDirtyRegion from viewWillDraw, before we call widgetListener->WillPaintWindow (which will call PrepareWindowEffects). During viewWillDraw all Cocoa-caused invalidations should already be present in getRectsBeingDrawn. If we did that, this change wouldn't be necessary: (In reply to Matt Woodrow (:mattwoodrow) from comment #77) > I think what we really want here is to *not* do painting inside of > viewWillDraw, and instead have the drawRect path do a synchronous draw and > composite.

(In reply to Markus Stange from comment #78) > If we did that, this change wouldn't be necessary: > > (In reply to Matt Woodrow (:mattwoodrow) from comment #77) > > I think what we really want here is to *not* do painting inside of > > viewWillDraw, and instead have the drawRect path do a synchronous draw and > > composite. Oh, right, you want this in order to stop the double composition caused by bug 873944.

Let's finish the remaining OMTC work in bug 875335. I've gone ahead and landed the patches on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/380081b560fa https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2d80599bfb https://hg.mozilla.org/integration/mozilla-inbound/rev/3669991846ce https://hg.mozilla.org/integration/mozilla-inbound/rev/9361a3202287 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7e55eea0a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7e2203b363 https://hg.mozilla.org/integration/mozilla-inbound/rev/534004bf42b8 https://hg.mozilla.org/integration/mozilla-inbound/rev/89df86518fb2 Let's see if it sticks.

I pushed to UX, too, to make the next mozilla-central merge simpler: https://hg.mozilla.org/projects/ux/rev/e540eb3276a1 https://hg.mozilla.org/projects/ux/rev/9d1f8bc1e27a https://hg.mozilla.org/projects/ux/rev/18fc103cb90e https://hg.mozilla.org/projects/ux/rev/78b90e6c491b https://hg.mozilla.org/projects/ux/rev/23d8885f5f10 https://hg.mozilla.org/projects/ux/rev/d096a54ea77a https://hg.mozilla.org/projects/ux/rev/bc1f0124ee2f https://hg.mozilla.org/projects/ux/rev/c895cfe0c09a https://hg.mozilla.org/projects/ux/rev/99480bf38444 https://hg.mozilla.org/projects/ux/rev/1019becd7a2a

I just noticed that I broke HiDPI, see bug 875441.

https://hg.mozilla.org/mozilla-central/rev/380081b560fa https://hg.mozilla.org/mozilla-central/rev/6c2d80599bfb https://hg.mozilla.org/mozilla-central/rev/3669991846ce https://hg.mozilla.org/mozilla-central/rev/9361a3202287 https://hg.mozilla.org/mozilla-central/rev/fa7e55eea0a0 https://hg.mozilla.org/mozilla-central/rev/6b7e2203b363 https://hg.mozilla.org/mozilla-central/rev/534004bf42b8 https://hg.mozilla.org/mozilla-central/rev/89df86518fb2

<3

https://hg.mozilla.org/mozilla-central/rev/9d1f8bc1e27a https://hg.mozilla.org/mozilla-central/rev/18fc103cb90e https://hg.mozilla.org/mozilla-central/rev/78b90e6c491b https://hg.mozilla.org/mozilla-central/rev/d096a54ea77a https://hg.mozilla.org/mozilla-central/rev/bc1f0124ee2f https://hg.mozilla.org/mozilla-central/rev/c895cfe0c09a https://hg.mozilla.org/mozilla-central/rev/99480bf38444 https://hg.mozilla.org/mozilla-central/rev/1019becd7a2a