501618 - Follow up child widget removal on OS/2

Status: RESOLVED FIXED

(Core Graveyard :: Widget: OS/2, defect)

Description

[Peter Weilbacher]

Bug 352093 implements widget removal with the necessary changes for the other platorms. This bug is about doing the necessary changes on OS/2.

Platform specific bits that affect OS/2 I currently find in attachment 386202 [details] [diff] [review] (where a new implementation of nsIWidget::Scroll is needed).

Comment 1

[Rich Walsh]

Since I'm almost done with the necessary patches, I'll take ownership of this.

Comment 2

[Rich Walsh]

Attachment: OS/2 widget removal v1

This implements the OS/2 code to support both Bug 352093 (Remove child widgets from content area) and Bug 339548 (Move plugin widgets to the top of the widget hierarchy).

OS/2 bug 500933 (plugin widgets) should probably be closed out since it is just a component of the larger widget removal project.  If plugins lands before widget removal, the current patch can easily be broken into two parts.

Comment 4

[Peter Weilbacher]

The cross-platform patches have landed, so we should get this in as quickly as possible. I'll try to look at the patch again tonight and then push it, if I don't find a problem.

Comment 5

[Peter Weilbacher]

I'm sorry that this "as quickly as possible" took so long. Eeven though I haven't really finished my review and don't fully understand the new code. But this now has the status of a build break fix and didn't seem to cause any new problems with some very superficial testing, I now pushed the change:
   http://hg.mozilla.org/mozilla-central/rev/031780660399

I still leave the bug open for the r+ and some testing with the MPlayer plugin that I want to do.

Comment 6

[Peter Weilbacher]

Comment on attachment 388617 [details] [diff] [review]
OS/2 widget removal v1

OK, I think I understand the code now, and it seems correct for what it does. But why did you take out the NotifyForeignChildWindows() call? I think this will again break the MPlayer plugin as before bug 418645.

Comment 7

[Peter Weilbacher]

Hmm, would be great, if someone could test the MPlayer plugin. (I have tried for one hour to do that here, but it always hangs the app until I kill mplayer.exe.) The latest "nightly" (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/contrib/latest-mozilla-central/firefox-3.6a1pre.en-US.os2.zip) contains the fix from this bug.

Comment 8

[KO Myung-Hun]

I've tested.

As you thought, this patch breaks the MPlayer plugin as before.

Comment 9

[Rich Walsh]

(In reply to comment #6)
> why did you take out the NotifyForeignChildWindows() call?

Because:
 - it had been removed from the Win32 code that was my template
 - its removal had no effect on any of the plugins I tested with
 - its real purpose (to support video overlay plugins) and/or
   its intended beneficiary (npmp) was wholly undocumented.

Even if I had known what it's for, I probably still couldn't have tested because I can't get npmp to work when invoked from an 'object' tag.  If I drop, e.g., a Quicktime movie (or a URL to it) on a blank page, it fills the page and plays.  But if I embed it on a page where I can force scrollbars and do some testing, it displays "media not found".

If someone can provide me with either a test page or a plugin that works as expected, I'll be happy to fix whatever needs fixin'.

Comment 10

[Peter Weilbacher]

OK, I just wanted to know if you had a specific problems with that function, that it didn't compile any more or something. I got the plugin working again and can reproduce the original problem. I'll try to resurrect the code with an improved comment.

Probably you know this, but anyway as a tip: for any not optimally documented code (it's difficult to know that in this case the word "video" would have triggered something), one can search in mxr.mozilla.org for it and access the "blame". In this case one has to use the "Mozilla CVS" database, because the code is older than the Hg repo. Then one can access <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/os2/nsWindow.cpp&rev=1.234&mark=2211#2211>. Hovering the mouse over the checkin names in the left column one can find the bug number and the checkin comment. In this case that would already have given the MPlayer plugin as the full reason. Otherwise digging into the comments on the bug might have helped.

Comment 11

[Rich Walsh]

(In reply to comment #10)
> I got the plugin working again and can reproduce the original problem.
> I'll try to resurrect the code with an improved comment.

I already have the code & can restore it as needed.  However, I can't test my patch properly, either for the obvious problem that you can reproduce or for other less obvious issues that I encountered while testing other plugins.

Could somebody please identify or create a page where this plugin works as expected?

Comment 12

[Peter Weilbacher]

See bug 418645 comment 28.

Comment 13

[Peter Weilbacher]

Attachment: reinstate the notification

Same function as before, just with two lines more comment.

I checked that we indeed still need to recurse, even though the widget hierarchy is now a bit flatter than before. Doing the notification from within ConfigureChildren is unfortunately too late.

I would like to get this in until Monday, i.e. before the freeze for branching and FF 3.6a1...

Comment 14

[Rich Walsh]

(In reply to comment #13)
> Created an attachment (id=392066) [details]
> reinstate the notification

I already have a patch which addresses the smear issue (without recursion) plus a couple of other plugin-related bugs.  I will post it later today after I've updated my tree & rebuilt.

Comment 15

[Rich Walsh]

Attachment: fix plugin scrolling & positioning

This sends WM_VRNDISABLED messages without the need for recursion.  Previously, the code drilled down through an indeterminate number of child windows to find the first non-mozilla window, then sent the msg to it.  This is no longer necessary.  The only children of the scrolling window are mozilla plugin windows, and their children are the non-mozilla windows we're targeting.  Thus, we only have to enumerate the top-level children and send a msg to their first (and only) child.

This patch also fixes two other problems:
- the plugin's frame would be mispositioned when it was partially scrolled off the bottom of a page - the visible portion of the window would display the bottom of the image rather than the top;
- after resizing a mozilla plugin window, it would be invalidated twice, causing the WM_VRNDISABLED msg to be ineffective.

Comment 16

[Peter Weilbacher]

Comment on attachment 392116 [details] [diff] [review]
fix plugin scrolling & positioning

Yes, this works, too, to remove the spilling effect with npmp (still with a recursion of 1 :-)) and to better position the plugin content.
If you don't prefer otherwise, I think hGrandChild should move into the while loop (to put the braces to some use and restrict the scope of the variable, I was recently told, that's how it should be done now, both in C and C++) and the extra space with WinQueryWindow should be removed. Thanks for removing the "::". :-)

Comment 17

[Peter Weilbacher]

I'll push the fix soon, but comparing to the behavior on the 1.9.1 branch, though, scrolling with MPlayer (both in DIVE and SNAP modes) is still awfully slow. And it is now repainting more often than before, especially when the video is partially outside the displayed area. It then seems to be told a different size, as I see the video shrink for an instance so that it fully fits into the viewport. Then it rescales to the correct size (part of which is invisible). The "Click here to play" message now also tends to display close to the center of the _visible_ plugin area and not in the middle of the full plugin area as before.

I think this deserves more investigation, I'll open a new bug for that.

Comment 18

[Peter Weilbacher]

http://hg.mozilla.org/mozilla-central/rev/86b666ca54d0