Closed Bug 420491 Opened 12 years ago Closed 10 years ago

Ability to reparent the contentview to hide the titlebar in fullscreen

Categories

(Core :: Widget: Cocoa, enhancement)

All
macOS
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sylvain.pasche, Assigned: mstange)

References

Details

Attachments

(4 files, 11 obsolete files)

4.41 KB, patch
jaas
: review+
Details | Diff | Splinter Review
4.76 KB, patch
jaas
: review+
Details | Diff | Splinter Review
23.29 KB, patch
jaas
: review+
Details | Diff | Splinter Review
20.45 KB, patch
jaas
: review+
Details | Diff | Splinter Review
From bug 370857 comment 23:

"[...] since we cannot hide the titlebar on the fly for a window (without
recreating the window or reparenting the content view), I think we should spin
off a separate bug for implementing this. Reparenting the contentview is IMO
the "right" way to do this, but it's stability-wise it's a risky change (what
with widgets caching weak pointers to native windows, etc) so maybe that's even
a post-1.9 thing."

The approach in bug 370857 is to move the window offscreen to hide the titlebar. That's a kind of hack and can be bad if you have multiple monitors vertically aligned.
Hardware: PC → All
Boris's patch in bug 113934 will make it possible to move content from one window to another.  Yay!
Depends on: 113934
Assignee: nobody → mstange
Status: NEW → ASSIGNED
The patches I'll attach here depend on the patch in bug 495710.
Depends on: 495710
EnsureWindowData is the place where the window map gets notified about the existence of a window that it should monitor. Currently it's only called when we create a ChildView, which is a little strange IMO. This setup makes the assumption that we only care about a window after we create a ChildView in it.
However, when we reparent ChildViews into a new window, this assumption doesn't hold. So we should call ensureWindowData after the creation of the window itself, too, and therefore I'm moving this code into the window map where it belongs.
Attachment #381024 - Flags: review?(smichaud)
Depends on: 495920
Oops, somehow I had lost a null check.
Attachment #381024 - Attachment is obsolete: true
Attachment #381034 - Flags: review?(smichaud)
Attachment #381024 - Flags: review?(smichaud)
This patch applies on top of the patch in bug 495710 and the "part 1" patch.
It factors out the creation and destruction of the native window into several helper functions.
I'll attach a diff -w for ease of reviewing.
Attachment #381035 - Flags: review?(joshmoz)
This is the main patch that does the view reparenting. I think it's relatively straightforward.
You will need the patch in bug 495920 if you don't want to crash after the reparenting.
Attachment #381040 - Flags: review?(joshmoz)
Attachment #381040 - Flags: review?(smichaud)
Attachment #381034 - Flags: review?(smichaud) → review+
Attachment #381035 - Attachment is obsolete: true
Attachment #381035 - Flags: review?(joshmoz)
Comment on attachment 381035 [details] [diff] [review]
part 2, v1: Refactor native window creation

New patches coming.
Attachment #381037 - Attachment is obsolete: true
Attachment #381037 - Flags: review?(joshmoz)
Attachment #381040 - Attachment is obsolete: true
Attachment #381040 - Flags: review?(smichaud)
Attachment #381040 - Flags: review?(joshmoz)
(on top of the patch in bug 495710)
Attachment #383644 - Flags: review?(joshmoz)
Attachment #383646 - Flags: review?(joshmoz)
Attachment #383646 - Flags: review?(smichaud)
Comment on attachment 383646 [details] [diff] [review]
part 4, v1: Implement HideWindowChrome

I didn't test this, but it basically looks fine to me.

I did find a few mistakes, though.  And I have one question.

> -  virtual nsresult   CreateNativeWindow(const nsIntRect &aRect,
> -                                        nsBorderStyle aBorderStyle);
> +  virtual nsresult   CreateNativeWindow(const NSRect &aRect,
> +                                        nsBorderStyle aBorderStyle,
> +                                        PRBool aRectIsFrameRect);

The last line should be:

  +                                        PRBool aRectIsFrameRect = PR_FALSE);

> +// Otherwise, aRect.x/y specify the position of the window's frame relative to
> +// the menubar and aRect.width/height specify the size of the content rect.

"the menubar" -> "the bottom of the menubar"

> +  NSView* contentView = [mWindow contentView];
> +  [contentView removeFromSuperviewWithoutNeedingDisplay];
> +  NSRect frameRect = [mWindow frame];
> ...

Apple's docs (on the removeFromSuperview and
removeFromSuperviewWithoutNeedingDisplay methods) say that contentView
should be retained before either of these calls, and only released
after it's been added to another view hierarchy.

> +  if (!mWindowMadeHere ||
> +      (mWindowType != eWindowType_toplevel && mWindowType != eWindowType_dialog) ||
> +      [[mWindow childWindows] count])
> +    return NS_ERROR_FAILURE;

Why do you need to stop nsCocoaWindow::HideWindowChrome() from being
called when the native window has child windows?  What specific
cases/problems are you trying to avoid?  You might want to comment on
that here.
(In reply to comment #13)
Thanks for the review!

> The last line should be:
>  +                                        PRBool aRectIsFrameRect = PR_FALSE);

Oh, it doesn't even compile because of that mistake... sorry.

> Apple's docs (on the removeFromSuperview and
> removeFromSuperviewWithoutNeedingDisplay methods) say that contentView
> should be retained before either of these calls, and only released
> after it's been added to another view hierarchy.

Oops. I'll do that.

> Why do you need to stop nsCocoaWindow::HideWindowChrome() from being
> called when the native window has child windows?  What specific
> cases/problems are you trying to avoid?

I noticed that going into fulllscreen mode while the toolbar customization palette is open will make it disappear. I tried reparenting the child windows using setParentWindow, but on first try it didn't seem to work. I'm not sure what exactly happened, though; until I figure that out I just wanted to disallow it.
pushed part 1, v2 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/d159361bf624
Whiteboard: [needs review josh]
Comment on attachment 383644 [details] [diff] [review]
part 2, v2: Refactor native window construction

Why is "StandardCreate" declared to be virtual, and why did you make the new functions (CreateNativeWindow, CreatePopupContentView, DestroyNativeWindow) virtual? AFAICT they are just helper functions specific to nsCocoaWindow.

+  if (mWindowType == eWindowType_popup ? !UseNativePopupWindows() : !aNativeWindow) {

Add parenthesis around the left "==" operation for clarity.

+  // Note that we need to check the window type because we mark sheets sheets as 

Typo: "sheets sheets"

In the future, separate bugs marked as dependencies would be a good idea. This is a lot for one bug, particularly since you have it broken down into nice steps already.
Attachment #381036 - Attachment is obsolete: true
(In reply to comment #16)
> (From update of attachment 383644 [details] [diff] [review])
> Why is "StandardCreate" declared to be virtual,

I don't know, it has been virtual since its inception in 1998. I've changed it to be protected and non-virtual.

> and why did you make the new
> functions (CreateNativeWindow, CreatePopupContentView, DestroyNativeWindow)
> virtual?

Because I copied the declaration of StandardCreate when I created them ;)

I've now made them protected and non-virtual, too.

> +  if (mWindowType == eWindowType_popup ? !UseNativePopupWindows() :
> !aNativeWindow) {
> 
> Add parenthesis around the left "==" operation for clarity.

Done.

> +  // Note that we need to check the window type because we mark sheets sheets
> as 
> 
> Typo: "sheets sheets"

Fixed.

> In the future, separate bugs marked as dependencies would be a good idea.

OK, I'll do that in the future.

Part 3 v2 still applies.
Attachment #383644 - Attachment is obsolete: true
Attachment #384699 - Flags: review?(joshmoz)
Attachment #383644 - Flags: review?(joshmoz)
Attached patch part 4, v2 (obsolete) — Splinter Review
Attachment #383646 - Attachment is obsolete: true
Attachment #384700 - Flags: review?(smichaud)
Attachment #383646 - Flags: review?(smichaud)
Attachment #383646 - Flags: review?(joshmoz)
Comment on attachment 384699 [details] [diff] [review]
part 2, v3 [checked in]

+   // Utility method for implementing both Create(nsIWidget ...) and
+   // Create(nsNativeWidget...)
+  nsresult             StandardCreate(nsIWidget *aParent,

Oops, the comment is indented incorrectly.
Attachment #384699 - Flags: review?(joshmoz) → review+
Whiteboard: [needs review josh] → [needs review josh][needs review smichaud]
Attachment #384700 - Flags: review?(smichaud) → review+
Comment on attachment 384700 [details] [diff] [review]
part 4, v2

Looks fine to me.

Though I still think you should add a comment about the following
(maybe an XXX comment).  It may be a while before you come back and
fix this.

>> Why do you need to stop nsCocoaWindow::HideWindowChrome() from
>> being called when the native window has child windows?  What
>> specific cases/problems are you trying to avoid?
>
> I noticed that going into fulllscreen mode while the toolbar
> customization palette is open will make it disappear. I tried
> reparenting the child windows using setParentWindow, but on first
> try it didn't seem to work. I'm not sure what exactly happened,
> though; until I figure that out I just wanted to disallow it.
Whiteboard: [needs review josh][needs review smichaud] → [needs review josh]
Attachment #381034 - Attachment description: part 1, v2: restore window null check → part 1, v2: restore window null check [checked in]
Comment on attachment 384699 [details] [diff] [review]
part 2, v3 [checked in]

Checked in part 2, v3: http://hg.mozilla.org/mozilla-central/rev/47d3f32817f8
Attachment #384699 - Attachment description: part 2, v3 → part 2, v3 [checked in]
Comment on attachment 383645 [details] [diff] [review]
part 3, v2: Factor out window mask identification [checked in]

+// Helper method for window masks of toplevel and dialog windows

Probably don't need this comment, the name of the function is descriptive enough.

-  /* Apple's docs on NSWindow styles say that "a window's style mask should

Please preserve this whole comment somehow.
Attachment #383645 - Flags: review?(joshmoz) → review+
Whiteboard: [needs review josh] → [needs new patch (part 4) mstange]
Comment on attachment 383645 [details] [diff] [review]
part 3, v2: Factor out window mask identification [checked in]

Checked in part 3, v2: http://hg.mozilla.org/mozilla-central/rev/385e96c9370c
Attachment #383645 - Attachment description: part 3, v2: Factor out window mask identification, unify toplevel and dialog → part 3, v2: Factor out window mask identification [checked in]
Attached patch part 4, v3 (obsolete) — Splinter Review
Reparenting child windows was really easy after all.
Attachment #384700 - Attachment is obsolete: true
Attachment #385634 - Flags: review?(joshmoz)
Whiteboard: [needs new patch (part 4) mstange] → [needs review josh]
Comment on attachment 385634 [details] [diff] [review]
part 4, v3

+  // Show the new window.
+  rv = Show(PR_TRUE);
+  NS_ENSURE_SUCCESS(rv, rv);

Are you sure you want to show every time? Can this be called on a hidden window?

We need to make sure this doesn't mess up plugins, which on Mac OS X hold references to their top-level window's WindowRef and its per-window Quickdraw graphics world. We need to make sure NPP_SetWindow is called after re-parenting to give plugins their new parent WindowRef and Quickdraw graphics world. Please makes sure that is happening and that this works with Flash, Flip4Mac, Real, Quicktime...
Attachment #385634 - Flags: review?(joshmoz)
Whiteboard: [needs review josh] → [needs plugin testing mstange]
Depends on: 501407
Attached patch part 4, v4 (obsolete) — Splinter Review
First testing revealed:
 - Flash didn't have any problems.
 - Flip4Mac and QuickTime sometimes crashed because in StartDrawPlugin called
   ::SetGWorld with an outdated port. It doesn't any more since I've added
   UpdatePluginPort (see the changes to nsChildView.mm).
 - RealPlayer takes the cake. First it fails to draw its controls; on 10.5 it
   will crash subsequently, while on 10.4 it starts drawing the video outside
   of the window.

I haven't been able to fix the RealPlayer problems. It seems to me that SetWindow is called often enough, since every call into the plugin in nsObjectFrame.cpp is in some way preceded by a call to FixupPluginWindow... and mPluginPortChanged is apparently set correctly, too.

I'd appreciate any help here.
Attachment #385634 - Attachment is obsolete: true
Out of curiosity, does RealPlayer suffer the same fate when you detach a tab from a Firefox window?
Interestingly, it doesn't. There are some painting errors, but no crash.
(In reply to comment #26)

> I haven't been able to fix the RealPlayer problems. It seems to me
> that SetWindow is called often enough, since every call into the
> plugin in nsObjectFrame.cpp is in some way preceded by a call to
> FixupPluginWindow... and mPluginPortChanged is apparently set
> correctly, too.
>
> I'd appreciate any help here.

I'd guess the Real plugin is caching information passed to it by
NPP_SetWindow() (possibly including NP_Port.port), and (re)using it in
methods that aren't directly called from any NPAPI method (e.g on a
timer).

If so, there's not a lot you can do about it.  Especially given that
we don't have the Real plugin's source, and don't know exactly which
information is cached, and what the plugin might be doing with it.
Comment on attachment 386955 [details] [diff] [review]
part 4, v4

OK, then let's give this a chance.
Attachment #386955 - Attachment description: part 4, v4, wip → part 4, v4
Attachment #386955 - Flags: review?(joshmoz)
Whiteboard: [needs plugin testing mstange] → [needs review josh]
> OK, then let's give this a chance.

I don't understand.
I meant "Let's give this patch the chance to get reviewed and landed, even if it will make RealPlayer crash".
That doesn't sound like a good idea to me :-(
By the way, make sure you're testing with the latest version of
RealPlayer.  The download has been called "RealPlayer11GOLD.dmg" for
more than a year now.  But when I downloaded a copy last week I found
that it's now at version 11.0.1.  (The copy I downloaded last June was
version 11.0.0.)
If the problems persist, I'd suggest taking up this issue either with Real directly or on the plugin-futures list.

That's not a public list.  If you're not already on it, Josh should add you :-)

(No, I don't have any Real contacts, but Josh might.)
I think we had one or two Real contacts when we were working on some of the sillier Camino-RealPlayer bugs. Maybe check the cc lists for those?
(In reply to comment #37)

Can you give us some bug numbers?
Comment on attachment 386955 [details] [diff] [review]
part 4, v4

>+void nsChildView::UpdatePluginPort()
>+{
>+  NS_ASSERTION(mIsPluginView, "UpdatePluginPort called on non-plugin view");
>+
>+  // Call GetNativeData in order to update mPluginPort, which is used in
>+  // StartDrawPlugin.
>+  if (!mPluginIsCG)
>+    GetNativeData(NS_NATIVE_PLUGIN_PORT);
>+}

I don't like the hidden logic of cache updating here. Ideally "nsChildView::StartDrawPlugin" should obtain the correctly updated port on its own, otherwise we should have an explicit method for updating the cache. It doesn't make sense to set cache data with a call that starts with "Get". An explicit call, if necessary, can be used in your new "UpdatePluginPort" method and in "GetNativeData", or maybe "UpdatePluginPort" could just be implemented such that "GetNativeData" can use it.
Attachment #386955 - Flags: review?(joshmoz) → review-
I assume Josh knows who our current Real contacts are, but Bob Clark worked on and fixed the very frustrating bug 336211 (which was the actual Real bug among the bugs Chris mentioned in comment 37).
I'll take care of getting in touch with Real if it becomes necessary.
Whiteboard: [needs review josh] → [needs new patch mstange]
Matthew, in bug 420527 you made StartDrawPlugin use the cached plugin port. I haven't really understood the reason for that yet. Do you have any suggestions how to address comment 39?
If I understand what you're asking, I'm not sure what the answer is, sorry.

We were using the port cached in mPluginPort before bug 420527, that bug just altered the logic to use the top-level window's port for the CG plugins (as there's no particular port associated with the plugin in this case) and execute the same logic we used to set up Carbon plugins.
Oops, sorry Matthew, I totally misread your patch.
Attached patch part 4, v5 (obsolete) — Splinter Review
> or maybe "UpdatePluginPort" could just be implemented
> such that "GetNativeData" can use it.

That's what I implemented now.
Attachment #386955 - Attachment is obsolete: true
Attachment #388543 - Flags: review?(joshmoz)
Whiteboard: [needs new patch mstange] → [needs review josh]
Comment on attachment 388543 [details] [diff] [review]
part 4, v5

>+void nsChildView::UpdatePluginPort()
>+{
>+  NS_ASSERTION(mIsPluginView, "UpdatePluginPort called on non-plugin view");
>+
>+  NSWindow* window = [mView nativeWindow];
>+  if (!mPluginIsCG) {

Switch the if blocks around so we don't need to negate mPluginIsCG.

>+    if (window) {
>+      WindowRef topLevelWindow = (WindowRef)[window windowRef];
>+      if (topLevelWindow) {
>+        mPluginPort.qdPort.port = ::GetWindowPort(topLevelWindow);
>+
>+        NSPoint viewOrigin = [mView convertPoint:NSZeroPoint toView:nil];
>+        NSRect frame = [[window contentView] frame];
>+        viewOrigin.y = frame.size.height - viewOrigin.y;
>+
>+        // need to convert view's origin to window coordinates.
>+        // then, encode as "SetOrigin" ready values.
>+        mPluginPort.qdPort.portx = (PRInt32)-viewOrigin.x;
>+        mPluginPort.qdPort.porty = (PRInt32)-viewOrigin.y;
>+      }
>+    }

We should probably null out the qdPort if there is no window, like we do in the CG case.

>+  nsresult             CreateNativeWindow(const NSRect &aRect,
>+                                          nsBorderStyle aBorderStyle,
>+                                          PRBool aRectIsFrameRect = PR_FALSE);

I dislike implicit arguments like that, I prefer they only be used when it makes a big difference in the number of args. Just pass PR_FALSE from any caller that wants PR_FALSE.

>-  // compensate for difference between frame and content area height (e.g. title bar)
>-  NSRect newWindowFrame = [NSWindow frameRectForContentRect:rect styleMask:features];
>+  if (!aRectIsFrameRect) {

Please switch the blocks here so we don't have to negate aRectIsFrame and the simple case is first.
Attachment #388543 - Flags: review?(joshmoz)
Whiteboard: [needs review josh]
Attached patch part 4, v6Splinter Review
Attachment #388543 - Attachment is obsolete: true
Attachment #389808 - Flags: review?(joshmoz)
Attachment #389808 - Flags: review?(joshmoz) → review+
Landed part 4: http://hg.mozilla.org/mozilla-central/rev/afe42cf52913
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Depends on: 515446
Depends on: 526318
Depends on: 537044
You need to log in before you can comment on or make changes to this bug.