Plugins no longer get resize events

VERIFIED FIXED in mozilla0.9

Status

()

Core
Plug-ins
P3
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: markh, Assigned: Peter Lubczynski)

Tracking

Trunk
mozilla0.9
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

18 years ago
Plugins no longer have nsIPluginInstance::SetWindow() called when the plugin is 
resized.  This can be confirmed by performing the following steps:
* Build a debug version of the npsimple plugin sample (modules/plugin/test)
* Open the SimpleScriptableTest.html file
* Note that the console correctly prints the 
message "SimplePluginInstance::SetWindow" as the sample is loaded.
* Reresize the main frame.
* Check the console - no "SimplePluginInstance::SetWindow" message is printed 
from the resize.

I am fairly sure this is a regression introduced in revision 1.194 of 
nsObjectFrame.cpp - http://bonsai.mozilla.org/cvslog.cgi?
file=mozilla/layout/html/base/src/nsObjectFrame.cpp

Note that an "ignore whitespace" diff is recommended as there are a number of 
indentation fixes included in 1.194.

In this checkin, the diff chunk starting at line 1214 has removed the call to 
SetWindow().  For the Mac only, it also added a call to FixUpPluginWindow(), 
but this does not call SetWindow() either.

Adding suggested patch which reinstates the SetWindow() call.  This means that 
SetWindow will again be called twice on object instantiation - once to set the 
window and again after the first reflow.

Adding bnesse to the CC as the submitter of Rev 1.194, and Chris Blizzard as he 
is kind enough to be helping us here, and confirms that this size event is 
needed for Linux.
(Reporter)

Comment 1

18 years ago
Created attachment 26382 [details] [diff] [review]
Suggested patch that reinstates the SetWindow() call on resize.

Comment 2

18 years ago
Unfortunately I lost the Mac version of the simple plugin sample that I had
created when upgrading my CVS required pulling a new tree from scratch. Thus
testing this on the Mac is gonna be difficult :(.

As I recall the SetWindow call was removed because DidReflow was being called
before the window coordinates were properly set. This was causing the plugin to
be drawn on the screen somewhere, and then moved on the next update cycle after
the coordinates were updated with the correct information.
(Reporter)

Comment 3

18 years ago
Occasionally we do see 2 resize events.  However, I'm not sure the solution is 
to prevent _any_ resize events at all.  I appreciate the problem with a repro 
on the Mac, but how can we progress this?  The way things stand, there is no 
way for Windows or Unix plugins to get resize events at all.

Would you be happier if we only did this for Unix and Windows?  Any other 
suggestions on how we can progress this and stop it rotting?

Comment 4

18 years ago
It's not about preventing multiple resize events as much as it is about
preventing resize events before the plugin has been properly placed.

Sorry, but I'm a little confused here. What exactly is the bug you are trying to
fix? In the case you describe the plugin doesn't "resize", it gets pushed around
the screen and the encompassing widget handles that. Unless your plugin is
"windowless" (full page) I don't understand why you would need SetWindow() to be
called on a reflow.

If this patch is the only way to fix your problem then, yes, #ifndef XP_MAC the
code. Even if it turns out that this is necessary on the Mac, it will do more
harm than good in the near term (i.e. break something that is fixed as opposed
to fixing something which is broken) on the Mac.
(Assignee)

Comment 5

18 years ago
Please, if this is needed to get plugins to work better on Linux, #ifndef XP_MAC 
it out because it fixes bugs on Mac.

Comment 6

17 years ago
This is particularly noticeable when going to pages with Flash content and using
either the Flash 4 or Flash 5 plugin (I'm using Linux build 2001031408).  E.g.
http://www.quantumlight.com/

When the frame is resized, the Flash movie does not get resized, it merely gets
cropped.  This is a big issue for us -- we use a fair amount of Flash in our UI.
 Flash movies got resized just fine in Mozilla0.7.
(Assignee)

Comment 7

17 years ago
What if for NS_SIZE events NPP_SetWindow was called?

Can someone attach a GOOD testcase?

Comment 8

17 years ago
A good test case lives here:
http://lxr.mozilla.org/seamonkey/source/modules/plugin/testevents/

It should be in the main trunk.  We created this recently to provide a more 
solid plugin testcase.  This is a native editor based on scintilla and an HTML 
testcase comes with it.  We wanted to highlight some of the problems we were 
having with plugins, and sizing on linux is very high on that list.

Comment 9

17 years ago
It seems that the cases involved here are either a) full page plugin
(quantumlight) or b) in a frame sized by percent (trunk test case). It would
seem that, in either case, Peter's suggestion should work.
(Reporter)

Comment 10

17 years ago
The previously attached patch does ensure that size events get to the plugin, 
but as a result of reflows rather than specifically using the size events.

It seems clear to me that this was a regression, so the simplest solution is to 
undo that regression, rather than changing the way it is done.

If I move the patch to be inside #ifdef's that exclude the Mac, will I have a 
hope of r=&sr=?
(Assignee)

Comment 11

17 years ago
Mark, have you tried your patch on the Mac? What's it's behavior. An XP solution 
would be the best.
Assignee: av → peterlubczynski
(Reporter)

Comment 12

17 years ago
I havent tested the Mac.  However, earlier comments in this bug imply that the 
Mac is perfect without any such patch - indeed, the SetWindow() call I 
reinstated was purposely removed from the code for the Mac to fix a bug - 
unfortunately its removal from non-mac platforms was the problem.

I don't have a Mac and am unlikely to get one soon.  It appears the Mac Moz 
guys themselves will have trouble testing this, judging by the second comment 
in this bug.  Ideally, a port of the testevents sample plugin for the Mac would 
be done, giving us a permanent test case.
(Assignee)

Comment 13

17 years ago
Created attachment 27833 [details] [diff] [review]
flash embeded testcase with %% width
(Assignee)

Comment 14

17 years ago
Please let me know if you see the cropping with the testcase in Linux. I do not 
see it in Windows. I DO see it on Mac. Whatever fix you have for Linux, the same 
should be done for Mac. I tried my idea of calling SetWindow if a NS_SIZE event 
comes through but the frame needs to be told to reflow itself. Not quite sure 
how to do this as nsIFrame::Reflow() takes a bunch of arguments.

Marc, is it possible to tell a frame to reflow itself or post that kind of 
event?
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
(Reporter)

Comment 15

17 years ago
Hrm.  One problem is that there are many ways to skin a cat.  On Windows, you 
can use the WM_SIZE message to hack into resize notifies if you need.  However, 
it is difficult and ugly, but may be how flash is working on Windows. I can see 
no way to make resize work on Windows without either my patch or WM_SIZE 
notification hacks.

Bug 59620 gave me the impression that the Real guys were using native Mac 
notifications for sizing, which then made me surprised that extra SetWindow() 
calls for sizing would be a problem.

Linux has no such hacks readily available, so AFAIK there is _no_ solution for 
Linux other than my patch.

I'm not sure why you want to trigger a reflow.  My patch allows SetWindow to be 
called on Reflow, and Moz itself takes care of when and how to reflow.  
Everything seems in place once my patch is applied.

What am I missing?  As far as I know, everything except flash on the Mac was 
working perfectly before bug 59620 was fixed, so I don't understand why that 
same solution isn't OK now.  If bug 59620 fixed Flash on the Mac, but broke 
Real on the mac and all plugins on all other platforms, my impression is simply 
that the fix to 59620 is invalid.
(Assignee)

Comment 16

17 years ago
The fix to bug 59620 is valid because if fixes positioning on the Mac.

If my memory suits me, I think the problem was that sometimes we would Reflow 
(initial) and not have the proper port set for the Mac. Hence calling SetWindow 
in Reflow() caused plugins to be offset and jitter when laying out. Brian, I 
think there was also a problem with scrolling as well. 

I'm fine with the fix as long as it's #IFDEF but we really need a solution that 
works for ALL platforms. CC:ing some other plugin vendors.

Comment 17

17 years ago
Adding the patch outside of the ifdef code will most likely regress Bug 59620,
but applying in the patch in the #ifdef code won't fix this bug on the Mac. I've
put the patch into my tree to test this, but have been really busy this
afternoon. The patch in 59620 was more along the lines of "what worked" than it
was a "correct fix".

The real problem is that DidReflow gets called too many times and never with
enough information. At one point I tried to not call SetWindow while the plugin
was being instantiated, but DidReflow can't distinguish between the
instantiation call and a Resize call.

Comment 18

17 years ago
Using www.quantumlight.com as a test case, I see no ill effects on the Mac. I
would really like to have Nick test this patch on Real Player before giving it
the nod though.
(Assignee)

Comment 19

17 years ago
Created attachment 28916 [details] [diff] [review]
patch with #ifdef XP_UNIX
(Assignee)

Comment 20

17 years ago
Mark,

Your patch seems to not effect Windows and breaks Mac, so I have wrapped it in 
#ifdef XP_UNIX because I guess this is Linux only.

Please try this patch out. Thanks.
(Assignee)

Updated

17 years ago
OS: All → Linux
Priority: -- → P3
Hardware: All → PC

Comment 21

17 years ago
Actually Peter, as per my previous comment, it seems to be fine on the Mac. And 
it fixes the resize problem described. Are you sure it has no effect on windows? 
It seems like the problem should exist there as well.
(Reporter)

Comment 22

17 years ago
Windows also has the exact same problem.  Without a similar patch, the only way 
to get resize in Windows is to hack your own WNDPROC together and catch WM_SIZE 
messages.

If any #ifdef is needed it should be excluding _just_ the mac.  However it is 
not at all clear to me what the status of the mac side is.
(Assignee)

Comment 23

17 years ago
After testing on all platforms, Mark's first patch should go in. 

I'm seeking reviews.
Keywords: patch
Whiteboard: [seeking review]

Comment 24

17 years ago
[s]r=waterson
(Assignee)

Comment 25

17 years ago
Patch checked in.

new revision: 1.205; previous revision: 1.204
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [seeking review]

Comment 26

17 years ago
Lots of verifications.

If I had to test stuff, it was on Win2K, build id 2001052404.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.