1.01 KB, patch
|Details | Diff | Splinter Review|
408 bytes, patch
|Details | Diff | Splinter Review|
1.04 KB, patch
|Details | Diff | Splinter Review|
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.
Created attachment 26382 [details] [diff] [review] Suggested patch that reinstates the SetWindow() call on resize.
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.
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?
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.
Please, if this is needed to get plugins to work better on Linux, #ifndef XP_MAC it out because it fixes bugs on Mac.
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.
What if for NS_SIZE events NPP_SetWindow was called? Can someone attach a GOOD testcase?
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.
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.
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=?
Mark, have you tried your patch on the Mac? What's it's behavior. An XP solution would be the best.
Assignee: av → peterlubczynski
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.
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
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.
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.
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.
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.
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.
OS: All → Linux
Priority: -- → P3
Hardware: All → PC
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.
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.
After testing on all platforms, Mark's first patch should go in. I'm seeking reviews.
Whiteboard: [seeking review]
Patch checked in. new revision: 1.205; previous revision: 1.204
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [seeking review]
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.