Closed Bug 595180 Opened 9 years ago Closed 9 years ago

[OS X] [OpenGL] Window resizer not drawn

Categories

(Core :: Graphics, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- beta9+

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Keywords: regression, Whiteboard: [tb33needed][hardblocker])

Attachments

(5 files, 8 obsolete files)

34.16 KB, image/png
faaborg
: ui-review-
Details
22.16 KB, image/png
faaborg
: ui-review+
Details
80.61 KB, image/png
faaborg
: ui-review+
Details
2.25 KB, patch
joe
: review+
Details | Diff | Splinter Review
10.61 KB, patch
joe
: review+
jaas
: review+
Details | Diff | Splinter Review
The window resizer in the bottom right corner is missing when using the OpenGL layers backend. NSWindow used to draw it on top of everything after our NSViews were finished rendering, but apparently you can't draw anything on top of OpenGL-enabled NSViews.

The only simple fix I can think of at the moment is adding this to xul.css:
:root::after {
  content: '';
  position: fixed;
  bottom: 0;
  right: 0;
  -moz-appearance: resizer;
  z-index: some large value;
}

... but I don't like it.
That wouldn't do the right thing anyway, would it?  It'd put that in for non-toplevel XUL documents, for example.  And nonresizable XUL documents, etc...
Right you are.
Can't we just draw the resizer in nsChildView.mm (perhaps via a callback from LayerManagerOGL when after compositing the layers)?

Maybe that's the way to fix bug 595156 as well ... add nsIWidget::DrawUnder and nsIWidget::DrawOver callbacks that the layer managers use when drawing toplevel windows? I guess for non-BasicLayers we also need to get the size and position of the area drawn so we can create temporary buffers.

We could also hook something up from layout, having FrameLayerBuilder create a couple of extra layers.
Duplicate of this bug: 602740
confirmed in xulrunner-based apps
This is different from bug 489303, right?
blocking2.0: --- → beta8+
Markus, ideas?
Assignee: nobody → mstange
(In reply to comment #6)
> This is different from bug 489303, right?

Right. Bug 489303 will only restore the resizer on the main browser window, but on Mac we need it on all resizable windows. And bug 489303 won't have any effect on Mac, otherwise its main window resizer would conflict with the all-window one we're trying to restore in this bug. :)

(In reply to comment #7)
> Markus, ideas?

Roc's plan sounds good to me, but I don't have time to implement it at the moment. Somebody else will have to do it.
I don't have any opinion on whether the additional layer should be managed by the layer manager or by FrameLayerBuilder.
Assignee: mstange → nobody
Scott, can you look into this?
Assignee: nobody → sgreenlay
Duplicate of this bug: 606916
Duplicate of this bug: 602007
Whiteboard: [tbtrunkneeds]
Assignee: sgreenlay → joe
Attached patch WIP Mac Resize Handle Patch (obsolete) — Splinter Review
This patch adds nsIWidget::DrawOver and DrawUnder, and has an override in nsCocoaWindow that can be used to draw the resize handle (the drawing code in there right now is temporary and probably wrong).

The problem is that nsCocoaWindow::DrawOver is not called because mWidget in LayerManagerOGL::Render is an nsIWidget found through GetNearestWidget() and not the nsCocoaWindow.
Yeah, we'll be calling DrawOver on the nsChildView child of nsCocoaWindow. So, override DrawOver in nsChildView instead, or have it forward to nsCocoaWindow.
blocking2.0: beta8+ → betaN+
Joe asked me to state that I don't think this blocks a time-based beta 8. We'd certainly like to get it in, but it isn't a regression since beta7 and we're trying to stick to schedules for these betas, not features.
Whiteboard: [tbtrunkneeds] → [tb33needs]
Attached patch WIP Mac Resize Handle Patch (obsolete) — Splinter Review
Attachment #485833 - Attachment is obsolete: true
The above patch tries to draw a red square for the resize handle (but fails).
Attached patch Add UploadSurfaceToTexture (obsolete) — Splinter Review
Add an interface to easily upload a gfxASurface to a texture. 

I've tried to keep this as generic as possible so it can be used in CairoImageOGL and CanvasLayerOGL (might need a few more changes) to take advantage of the GetAsImageSurface optimization. I'll file a separate bug for that if this scrapes it's way through review.
Attachment #496052 - Flags: review?(joe)
Attached patch WIP drawing of the resize handle (obsolete) — Splinter Review
This uses UploadSurfaceToTexture and our normal rendering code to draw the resize handle.

Currently only draws a green rectangle, the cairo drawing code needs to be actually written.

We definitely don't need to be repeating this every frame, we can hang on to the texture and just re-use it.
Attached patch WIP native resizer drawing (obsolete) — Splinter Review
Here's a patch on top of your patches that uses a private method to draw the native resizer. Instead of using private stuff it would probably be better to draw the resizer manually, though.
Marking 604101 as blocking this bug as it should resolve the issue of uploading surfaces to textures efficiently. We may not need the UploadSurfaceToTexture patch depending on how this is resolved.
Depends on: 604101
This uses the native cocoa theme drawing code to draw a window resizer.

It does however draw an outlined square as well as the diagonal lines, not sure if this is an issue.
Attachment #492439 - Attachment is obsolete: true
Attachment #496052 - Attachment is obsolete: true
Attachment #496053 - Attachment is obsolete: true
Attachment #496128 - Attachment is obsolete: true
Attachment #497831 - Flags: review?(joe)
Attachment #496052 - Flags: review?(joe)
The square was the reason why I didn't go for that approach, but I guess we can still change that later.

Should we really add DrawUnder? I can't foresee a use for it.

The new nsIWidget methods need to be added to the nsIWidget_MOZILLA_2_0_BRANCH interface instead.
Attached image Resizer, no scroll bars
Let's get UI feedback for the resizer-with-box.
Attachment #498067 - Flags: ui-review?(faaborg)
Attachment #498068 - Flags: ui-review?(faaborg)
Attachment #498069 - Flags: ui-review?(faaborg)
Attached patch without box (obsolete) — Splinter Review
Here's a patch that draws the resizer using CGContextStrokeLineSegments. I've also gotten rid of DrawUnder, moved DrawOver to the nsIWidget_MOZILLA_2_0_BRANCH interface and made it compile in non-libxul builds.
Attachment #498341 - Flags: review?(joe)
There's another change that's necessary to make it compile in non-libxul builds. Widget doesn't pick up the definition of LayerManagerOGLProgram::sCurrentProgramKey in gfx/layers/opengl/LayerManagerOGL.cpp, so it can't call some methods of LayerManagerOGLProgram which use ASSERT_THIS_PROGRAM.

This patch just disables the assert in non-libxul builds.
Attachment #498342 - Flags: review?(joe)
>It does however draw an outlined square as well as the diagonal lines, not sure
>if this is an issue.

Yeah, the outline on the square draws too much attention to the re sizer (and isn't native in the sense that Safari doesn't do this).  Let's just use a pixel perfect image so that we can have direct control.
Attachment #498069 - Flags: ui-review?(faaborg) → ui-review+
Attachment #498068 - Flags: ui-review?(faaborg) → ui-review+
Attachment #498067 - Flags: ui-review?(faaborg) → ui-review-
Attachment #497831 - Attachment is obsolete: true
Attachment #497831 - Flags: review?(joe)
Attached patch without box, v2 (obsolete) — Splinter Review
Attachment 498341 [details] [diff] was missing a static_cast<nsIWidget_MOZILLA_2_0_BRANCH*>. This one actually compiles.
Attachment #498341 - Attachment is obsolete: true
Attachment #500837 - Flags: review?(joe)
Attachment #498341 - Flags: review?(joe)
Whiteboard: [tb33needs] → [tb33needs][hard blocker]
Whiteboard: [tb33needs][hard blocker] → [tb33needs][hardblocker]
Attachment #498342 - Flags: review?(joe) → review+
Comment on attachment 500837 [details] [diff] [review]
without box, v2

Won't this draw on all windows, not just resizable ones?

Also, I think we should create one texture per window, and reuse it.
Attachment #500837 - Flags: review?(joe) → review-
Attached patch draw resizer, v3Splinter Review
This fixes drawing the resizer for non-resizable windows. Nice catch Joe. Hope I'm not stepping on your toes jumping in here Markus.

I'm not sure what the best way to save off one texture per window is, I'll leave that for Markus. Why wouldn't we just save off one texture for all windows, btw?

Can we take this patch for now (so it'll make beta 9) and implement saving the texture in another bug later?
Attachment #500837 - Attachment is obsolete: true
Attachment #501977 - Flags: review?(joe)
Thanks, Josh!

Joe, would the nsCocoaWindow destructor be the only place where we'd have to delete the retained texture, or is it possible that the layer manager goes away before the widget is destroyed?
Comment on attachment 501977 [details] [diff] [review]
draw resizer, v3


>diff --git a/widget/src/cocoa/nsChildView.mm b/widget/src/cocoa/nsChildView.mm

>+void
>+nsChildView::DrawOver(LayerManager* aManager, nsIntRect aRect) {

Put that { on a new line.

Looks good; please file a blocker followup to only allocate one texture per window.

(We need one texture per window because we have GL context per window; we might be able to share, but it's a bit tricksy.)
Attachment #501977 - Flags: review?(joe) → review+
blocking2.0: betaN+ → beta9+
Assignee: joe → mstange
blocking2.0: beta9+ → betaN+
Attachment #501977 - Flags: review+
blocking2.0: betaN+ → beta9+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/8d005c3a6685
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre ID:20110109030350
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla2.0b9
Confirming all windows and dialogs of BlueGriffon who have a resizer now draw
it correctly again. Thanks a lot guys.
Depends on: 627656
Whiteboard: [tb33needs][hardblocker] → [tb33needed][hardblocker]
Depends on: 641357
No longer depends on: 641357
You need to log in before you can comment on or make changes to this bug.