Sylvain Pasche Reporter
	Description • 17 years ago

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.

	Jesse Ruderman
	Comment 1 • 16 years ago

Boris's patch in bug 113934 will make it possible to move content from one window to another.  Yay!

	Markus Stange [:mstange] Assignee
	Comment 2 • 15 years ago

The patches I'll attach here depend on the patch in bug 495710.

	Markus Stange [:mstange] Assignee
	Comment 3 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 4 • 15 years ago

Oops, somehow I had lost a null check.

	Markus Stange [:mstange] Assignee
	Comment 5 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 8 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 9 • 15 years ago

Comment on attachment 381035 [details] [diff] [review]
part 2, v1: Refactor native window creation

New patches coming.

	Markus Stange [:mstange] Assignee
	Comment 10 • 15 years ago

(on top of the patch in bug 495710)

	Steven Michaud [:smichaud] (Retired)
	Comment 13 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 14 • 15 years ago

(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.

	Josh Aas
	Comment 15 • 15 years ago

pushed part 1, v2 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/d159361bf624

	Josh Aas
	Comment 16 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 17 • 15 years ago

(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.

	Markus Stange [:mstange] Assignee
	Comment 19 • 15 years ago

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.

	Steven Michaud [:smichaud] (Retired)
	Comment 20 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 21 • 15 years ago

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

	Josh Aas
	Comment 22 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 23 • 15 years ago

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

	Markus Stange [:mstange] Assignee
	Comment 24 • 15 years ago

Reparenting child windows was really easy after all.

	Josh Aas
	Comment 25 • 15 years ago

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...

	Markus Stange [:mstange] Assignee
	Comment 26 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 27 • 15 years ago

The URL where I tested RealPlayer is http://www.film.com/movies/mediaplayback/transformers-revenge-of-the-fallen/26214263

	Jesse Ruderman
	Comment 28 • 15 years ago

Out of curiosity, does RealPlayer suffer the same fate when you detach a tab from a Firefox window?

	Markus Stange [:mstange] Assignee
	Comment 29 • 15 years ago

Interestingly, it doesn't. There are some painting errors, but no crash.

	Steven Michaud [:smichaud] (Retired)
	Comment 30 • 15 years ago

(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.

	Markus Stange [:mstange] Assignee
	Comment 31 • 15 years ago

Comment on attachment 386955 [details] [diff] [review]
part 4, v4

OK, then let's give this a chance.

	Steven Michaud [:smichaud] (Retired)
	Comment 32 • 15 years ago

> OK, then let's give this a chance.

I don't understand.

	Markus Stange [:mstange] Assignee
	Comment 33 • 15 years ago

I meant "Let's give this patch the chance to get reviewed and landed, even if it will make RealPlayer crash".

	Steven Michaud [:smichaud] (Retired)
	Comment 34 • 15 years ago

That doesn't sound like a good idea to me :-(

	Steven Michaud [:smichaud] (Retired)
	Comment 35 • 15 years ago

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.)

	Steven Michaud [:smichaud] (Retired)
	Comment 36 • 15 years ago

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.)

	Chris Lawson (gone)
	Comment 37 • 15 years ago

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?

	Steven Michaud [:smichaud] (Retired)
	Comment 38 • 15 years ago

(In reply to comment #37)

Can you give us some bug numbers?

	Josh Aas
	Comment 39 • 15 years ago

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.

	Smokey Ardisson (offline for a while; not following bugs - do not email)
	Comment 40 • 15 years ago

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).

	Josh Aas
	Comment 41 • 15 years ago

I'll take care of getting in touch with Real if it becomes necessary.

	Markus Stange [:mstange] Assignee
	Comment 42 • 15 years ago

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?

	Matthew Gregan [:kinetik]
	Comment 43 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 44 • 15 years ago

Oops, sorry Matthew, I totally misread your patch.

	Markus Stange [:mstange] Assignee
	Comment 45 • 15 years ago

> or maybe "UpdatePluginPort" could just be implemented
> such that "GetNativeData" can use it.

That's what I implemented now.

	Josh Aas
	Comment 46 • 15 years ago

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.

	Markus Stange [:mstange] Assignee
	Comment 48 • 15 years ago

Landed part 4: http://hg.mozilla.org/mozilla-central/rev/afe42cf52913