User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7) AppleWebKit/534.48.3 (KHTML, like Gecko) Version/5.1 Safari/534.48.3 Steps to reproduce: Run Latest Trunk on 10.7 Lion and take a look at the corners at the bottom. Actual results: The corners at the bottom have no fade out effect. Expected results: The corners at the bottom should have a fade out effect.

Yes, OS X cuts us off without anti-aliasing because we're using OpenGL :( It's possible to recreate a smooth anti-aliased rounded corner even with OpenGL, but it won't look perfect because the three pixels in the corner always stay completely transparent (while proper anti-aliasing would need two of them to be slightly opaque).

We can't say "I want square corners on my window", or at least I haven't found a documented way of doing that. OS X just cuts off the bottom corners of all windows with title bars, and for windows that use OpenGL it does so without anti-aliasing. But now I've found one way of stopping OpenGL context bottom corner cutoff, using brute force: extern "C" { typedef CFTypeRef CGSRegionObj; CGError CGSNewRegionWithRect(const CGRect *rect, CGSRegionObj *outRegion); } @interface NSSurface @end @implementation NSSurface(DontCutOffCorners) - (CGSRegionObj)_createRoundedBottomRegionForRect:(CGRect)rect { // Create a normal rect region without rounded bottom corners. CGSRegionObj region; CGSNewRegionWithRect(&rect, &region); return region; } @end

Actually, scratch the last two comments. I was testing with a 3px corner radius. With a corner radius of 4.5px, the three pixels in each corner are completely transparent anyway, so the forced erasure doesn't hurt us.

With the hidpi bugs that have just landed, this got a bit worse -- the corners are still "rounded" the same way, but the rounding appears to be done at low-resolution. Any possibility we can at least get this rounding at full res? Or is this just completely controlled by OSX?

(In reply to Justin Dolske [:Dolske] from comment #7) > With the hidpi bugs that have just landed, this got a bit worse -- the > corners are still "rounded" the same way, but the rounding appears to be > done at low-resolution. Any possibility we can at least get this rounding at > full res? Or is this just completely controlled by OSX? It's controlled by OSX, but the patch now deactivates that control and lets us do our own rounding. It looks really good in HiDPI mode :) https://tbpl.mozilla.org/?tree=Try&rev=8b914e5bdff0

Comment on attachment 675321 [details] [diff] [review] v1 Review of attachment 675321 [details] [diff] [review]: ----------------------------------------------------------------- You're much better off having Benoit Girard review this. ::: widget/cocoa/nsCocoaWindow.h @@ +113,2 @@ > - (void)setBottomCornerRounded:(BOOL)rounded; > +- (BOOL)bottomCornerRounded; What is this for? If this is a hidden API or something please document it.

Comment on attachment 675321 [details] [diff] [review] v1 Review of attachment 675321 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsChildView.mm @@ +1866,5 @@ > + LOCAL_GL_CLAMP_TO_EDGE, > + TextureImage::UseNearestFilter); > + > + // Creation of texture images can fail. > + if (!mCornerMaskImage) I wonder if we rather crash in this case. It's unexpected and likely means we're out of memory. Simply returning here isn't a good way to handle it because we will try again the next frame and get into a bad performance loop. I'd almost rather just see this show up in crash stats if it's a problem and if so properly handle the failure. @@ +1869,5 @@ > + // Creation of texture images can fail. > + if (!mCornerMaskImage) > + return; > + > + nsIntRegion update(nsIntRect(0, 0, cornerRadius, cornerRadius)); This should be devPixelCornerRadius @@ +1876,5 @@ > + mCornerMaskImage = nullptr; > + return; > + } > + > + NS_ABORT_IF_FALSE(asurf->GetType() == gfxASurface::SurfaceTypeQuartz, There's no guarantee this will be a quartz surface. And its make refactoring that method fragile. It would be perfectly valid to return a gfxImageSurface. Maybe it's safer to create your own quartz surface and simply upload that to the texture yourself.

The bottom round corners became square. I guess it's a result of changing cornerRadius to devPixelCornerRadius. Maybe you have a better suggestion. I'm probably not the best person to care of this bug as it seems to involve much more knowledge as I currently have on layers. But I'm willing to learn.

Comment on attachment 722844 [details] [diff] [review] Updated Markus' patch and took into account Benoit's comments Review of attachment 722844 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsChildView.mm @@ +1910,5 @@ > mResizerImage = nullptr; > return; > } > > + // is the CairoSurface of a non QuartzSurface usable in the gfxQuartzSurface constructor ? No it's not :( http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-quartz-surface.c#3604 I think the right thing to do here would be to allocate a Quartz Surface, draw to it and then upload that instead of trying to draw using Begin/EndUpdate which doesn't guarantee to provide a quartz surface (it could for example draw to the texture using skiagl). Feel free to have this work like before since it is a pre-existing problem and this patch doesn't make it worse.

I tried another way of building the gfxQuartzSurface in which corners mask and resizer picture were drawn, that was using lower level cairo-quartz APIs. But it didn't work out for incompatible (and even not assignable) types of images: I could not create mCornerMaskImage and mResizerImage that way. I also realized in the process that those surfaces likely had to share the LayerManager passed as parameter, so they all had the same GL context, which is the point for unblocking c. So the only way was indeed to use the gl()->CreateTextureImage method, even though its drawback is it doesn't guarantee we have the right type of supporting surface underneath. Sorry, I found no better for now. Also, the bottom corners are not rounded in Mac OS 10.8. I still have to investigate this. But at least my patch should allow work on 676241 to begin.

Comment on attachment 725523 [details] [diff] [review] Cleaned previous patch - replacing it Forgot to mark previous attachment (725401) as obsolete. Sorry.

I wont have time to review until early next week. Feel free to move the review if you don't want to wait.

Comment on attachment 727145 [details] [diff] [review] Bottom rounded corners are ok even on a retina display using the same openGL context as the rest of the window. >diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm >--- a/widget/cocoa/nsChildView.mm >+++ b/widget/cocoa/nsChildView.mm >@@ -47,16 +47,17 @@ > #endif > > #include "gfxContext.h" > #include "gfxQuartzSurface.h" > #include "nsRegion.h" > #include "Layers.h" > #include "LayerManagerOGL.h" > #include "GLTextureImage.h" >+#include "GLContext.h" Is this still necessary? >+@implementation NSSurface(DontCutOffCorners) >+- (CGSRegionObj)_createRoundedBottomRegionForRect:(CGRect)rect >+{ >+ // Create a normal rect region without rounded bottom corners. >+ CGSRegionObj region; >+ CGSNewRegionWithRect(&rect, &region); >+ return region; >+ } wrong indent before } >+ // is the CairoSurface of a non QuartzSurface usable in the gfxQuartzSurface constructor ? I don't think so. I think these are the options we have here: 1. Don't bother - if the surface is not a Quartz surface, print a warning and return. 2. Use gfxQuartzNativeDrawing to get a Quartz surface. 3. Don't use a CGContext at all and convert the drawing to use gfxContext APIs. Option 3 would probably be the cleanest, option 1 the quickest. >diff --git a/widget/cocoa/nsCocoaWindow.mm b/widget/cocoa/nsCocoaWindow.mm >--- a/widget/cocoa/nsCocoaWindow.mm >+++ b/widget/cocoa/nsCocoaWindow.mm >@@ -2808,17 +2808,17 @@ static const NSString* kStateShowsToolba > [super setBackgroundColor:mColor]; > mBackgroundColor = [[NSColor whiteColor] retain]; > > mUnifiedToolbarHeight = 0.0f; > > // setBottomCornerRounded: is a private API call, so we check to make sure > // we respond to it just in case. > if ([self respondsToSelector:@selector(setBottomCornerRounded:)]) >- [self setBottomCornerRounded:NO]; >+ [self setBottomCornerRounded:nsCocoaFeatures::OnMountainLionOrLater()]; This is not strictly necessary, because starting with Lion the window ignores calls to setBottomCornerRounded and returns YES from bottomCornerRounded anyway, but I can see how that can be confusing. If you want to make this change we should use OnLionOrLater().

See comment 19.

Comment on attachment 728199 [details] [diff] [review] Changes reflecting Markus' comments. >diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm >--- a/widget/cocoa/nsChildView.mm >+++ b/widget/cocoa/nsChildView.mm >@@ -43,20 +43,22 @@ > #include "nsMenuUtilsX.h" > #include "nsMenuBarX.h" > #ifdef __LP64__ > #include "ComplexTextInputPanel.h" > #endif > > #include "gfxContext.h" > #include "gfxQuartzSurface.h" >+#include "gfxQuartzNativeDrawing.h" You're not using this. >+ if (asurf->GetType() != gfxASurface::SurfaceTypeQuartz) { >+ NS_WARN_IF_FALSE(FALSE, "mResizerImage's surface is not Quartz"); >+ mCornerMaskImage = nullptr; This needs to be mResizerImage. So now, if the surfaces are not Quartz surfaces, we attempt to recreate them on every paint and spam the console with warnings. That's not beautiful it's OK I guess.

(In reply to Markus Stange from comment #21) > >+ if (asurf->GetType() != gfxASurface::SurfaceTypeQuartz) { > >+ NS_WARN_IF_FALSE(FALSE, "mResizerImage's surface is not Quartz"); > >+ mCornerMaskImage = nullptr; > > This needs to be mResizerImage. > > So now, if the surfaces are not Quartz surfaces, we attempt to recreate them > on every paint and spam the console with warnings. That's not beautiful it's > OK I guess. IMO stuff like that should just crash. If it happens I'd like crash-stats to tell us about it loudly rather then having the performance go to zero users that may never report the problem.

(In reply to Benoit Girard (:BenWa) from comment #23) > IMO stuff like that should just crash. If it happens I'd like crash-stats to > tell us about it loudly rather then having the performance go to zero users > that may never report the problem. For now it seems to work fine for all of us. But what if the patch crashes on all users when they upgrade to (say) 10.9? That would force us to patch in emergency. IMO it's preferable to have FF work in a sub-optimal mode rather than not work at all. Still waiting to ask for commit.

Sorry it's not yet clear to me how I should mark this patch for landing in mozilla-central. I just marked it resolved + fixed for now.

Actually it's normal now for patches destined for mozilla-central to land on mozilla-inbound first. Others are responsible for watching the tree on mozilla-inbound, and backing out patches that cause trouble. Generally the developer has to do this on mozilla-central. Mozilla-inbound is merged with mozilla-central about once a day. Whoever does this will mark all relevant bugs "fixed". ("Trouble" includes breaking the tree, of course. But it can also include persistent test failures or significant performance regressions.)