Closed Bug 298961 Opened 16 years ago Closed 16 years ago
Plugins get no notification through NPP
_Set Window when switching between tabs
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3 Generally I would expect that plugins are always aware of their window position and state. This does not seem to be case when a new tab is created and switched to. Reproducible: Always Steps to Reproduce: 1. Open FireFox 2. Go to www.macromedia.com which has two .SWF files on the page 3. Create a new tab and switch to it Actual Results: FireFox will not notify the Flash plugin about the fact that the user just switched to a new tab. Expected Results: Ideally NPP_SetWindow should be called and the CGrafPtr pointer in NP_Port::port should become NULL. Why is this important? Macromedia will be transitioning the Flash Player to use OpenGL for final blitting in the future, achieving a 20-30% performance increase for all content and tear free drawing. This will require to setup a OpenGL drawing region using the AGL/NGL frameworks. If we do not know when a switch to a tab happens we can not hide the OpenGL region and it will remain in place. Safari handles this correctly, we get notification through NPP_SetWindow and are able to hide the OpenGL region every time tabs are switched. You can contact me in case you need a test plugin binary.
over to josh or jst?
Assignee: nobody → joshmoz
Flags: blocking1.8b4? → blocking1.8b4+
I'm pretty sure we do call SetWindow when switching tabs (maybe not quite at the right time, see bug 277067). For a tab that's being hidden, the cliprect will be set to an empty rect (although I think the port is still non-null).
I verified that both Firefox and Camino notify plugins of tab switches by calling SetWindow(). For a plugin in a hidden tab, the port is not null, but the clipRect is empty (height and width are zero). I understand that this does not make it possible for the plugin to tell if it is simply scrolled offscreen in the current tab, or in a background tab. I'm loath to change it to pass a null port, because I have no idea which other plugins would be adversely affected by this change. We should float this on the macplugin-dev mailing list.
This can be taken of the 1.5b5 blocker list I think. Apparently Macromedia solved their problem because Flash 8.0.22 does't exhibit it.
http://www.kaourantin.net/2005/07/why-does-flash-player-8-on-mac-perform.html This indicates that 8.0.22 behavior might not be optimal because of this bug. Tinic - did you work around this bug successfully in 8.0.22?
(In reply to comment #5) > http://www.kaourantin.net/2005/07/why-does-flash-player-8-on-mac-perform.html > > This indicates that 8.0.22 behavior might not be optimal because of this bug. > Tinic - did you work around this bug successfully in 8.0.22? No we did not. I would not have been wise to break Flash in older FireFox versions ;-) 8.0.22 which we just released to the public disables OpenGL support for FireFox, we have a specific check in there. We check the user agent string for the occurence of "AppleWebKit" to see if the browser is Safari. So if you change the user agent string to include AppleWebKit somewhere OpenGL should be turned on. I also sent a custom build to Josh which always enables OpenGL. I can also rebuild 8.0.22 with OpenGL enabled if you need it.
Tinic: could you explain why what we provide now (SetWindow() with empty clipRect) is insufficient for your needs?
I think we might want to expand the plugin API to add tab switching calls, and then add code to call them appropriately (see bug 277067). It would be nice to do this for 1.5.
Noticed the change in state for this bug, and would like to comment on why Macromedia feels the bug is essential. Flash rendering performance on Firefox is currently much slower than Safari. The degraded performance is particularly perceptible when viewing video and graphics intensive media. The Flash Player team can’t enable the rendering optimizations for Firefox until the API is in place. If the API change is pushed beyond the 1.5 release, this will result in us having to live without the benefit of some really compelling optimizations for quite a while due to our release cycle.
Agreed. This is the kind of API change that has to get in earlier, rather than later. Requesting reconsideration.
Flags: blocking1.8b5- → blocking1.8b5?
I've retested this, and found that, indeed, we don't call SetWindow() on the plugin when a tab is being hidden. This should block 1.5b5, since it's known to cause issues with Flash, and RealPlayer. The resulting bugs (leaving stray frames on the new tab) have previously been construed as security issues.
Need to get traction on this ASAP, plussing for now. Josh/Simon, this needs to happen this week or we'll drop it again.
Flags: blocking1.8b5? → blocking1.8b5+
This patch tidies up some code in nsObjectFrame to ensure that we call SetWindow() on the plugin when the clip rect changes. This causes SetWindow() to get called for plugins in a tab which is being hidden. However, it does not fix the _timing_ of that call (it may still be too late, see bug 277067). I also removed some XP_MAC #ifdefs.
Comment on attachment 197583 [details] [diff] [review] Patch Tested with a version of the Flash plugin that has OpenGL enabled for Gecko browsers. This patch stops the plugin from drawing over tabs it shouldn't. Don't we have some utility function for comparing rects without testing every field? Fix that if you can, but otherwise this looks fine.
Attachment #197583 - Flags: review?(joshmoz) → review+
BTW - I only tested on Mac OS X 10.4.2. Somebody should take this patch for a spin on Windows, testing various plugins.
The changes in the patch are all in #ifdef (XP_MACOSX) sections.
Comment on attachment 197583 [details] [diff] [review] Patch sr=jst
Attachment #197583 - Flags: superreview?(jst) → superreview+
Comment on attachment 197583 [details] [diff] [review] Patch Josh is going to land this on the trunk so we can get some trunk baking / plugin testing going on there before this goes into the branch.
landed on trunk
Assignee: joshmoz → sfraser_bugs
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Excellent. My sincere thanks to the dev, qa, and code reviewers for making this happen so quickly. The Flash Player team will run some tests early next week and follow-up if we encounter any issues.
We wanted QA to give plugins a spin in the trunk builds.
Be sure to test Java, QuickTime, RealPlayer (e.g. weather.com) and Flash with this change.
Simon: Can we get a test case to use to verify this?
(In reply to comment #23) > Simon: Can we get a test case to use to verify this? Josh should be able to help you out here.
Tested using today's trunk build - Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050930 Firefox/1.6a1 using a version of the Flash plugin that has OpenGL enabled for Gecko browsers. Did not see any bleedthrough - I tested a number of sites including weather.com, ran numerous Quicktime trailers, a few of the flash top animation sites including http://www.ikea.com/ms/sv_SE/kampanj/fy06_dromkok/dromkok.html. I did see an issue with scrolling on the Macromedia site (when you scroll down you get flickering in the flash part of the site. If Macromedia doesn't address the bug on their side then we should get one on file.
Comment on attachment 197583 [details] [diff] [review] Patch approving for the branch now that we've gotten our QA feedback from this.
Attachment #197583 - Flags: approval1.8b5? → approval1.8b5+
landed on branch
As a result (I think) of this change, nsIPluginInstance::SetWindow() is now _never_ called with pluginWindow == NULL. That used to be a signal that the plugin should be made invisible. Also, pluginWindow->window->port (i.e. the GrafPort) is never NULL ... though there was some discussion (I think) of using this as a signal that the user had switched away from a tab. Am I right? And if so, was this deliberate? (By the way, both Firefox and Camino now seem to behave this way.)
(In reply to comment #28) > As a result (I think) of this change, nsIPluginInstance::SetWindow() is now > _never_ called with pluginWindow == NULL. That used to be a signal that the > plugin should be made invisible. If so, that wasn't deliberate. The signal that the plugin should be made invisible is that the clipRect is empty. > Also, pluginWindow->window->port (i.e. the GrafPort) is never NULL ... though > there was some discussion (I think) of using this as a signal that the user > had switched away from a tab. That was discussed, but never implemented. > Am I right? And if so, was this deliberate? What the patch in this bug did was to ensure that we always call SetWindow() when the clipRect changes. > (By the way, both Firefox and Camino now seem to behave this way.) Yes, they should behave the same way now (particularly that bug 312563 was checked in for Camino).
>> As a result (I think) of this change, nsIPluginInstance::SetWindow() is now >> _never_ called with pluginWindow == NULL. That used to be a signal that >> the plugin should be made invisible. > >If so, that wasn't deliberate. The signal that the plugin should be made >invisible is that the clipRect is empty. The MRJ Plugin has been using pluginWindow == NULL in this way for years (since well before I started playing with it). So, whether or not this was legitimate, I'll bet other plugins are doing the same thing. Also, as far as I can tell, stopping the use of pluginWindow == NULL on tab switching wasn't required to fix this bug (298961). (Tell me if I'm wrong, though.) Whatever the rights or wrongs of this particular issue, it's now moot for the Java Embedding Plugin -- I spent a couple of extra days banging my head against this, and found workarounds for all the issues I could identify. It helped that I long ago decided to treat the zero-clipping and pluginWindow == NULL signals identically -- and interpret both as signals that the plugin should (in effect) be made invisible. Your change showed up places where I hadn't quite achieved this ... but 20-odd hours of head-banging was enough to set things right. Also, the problems were subtle enough that I didn't notice them right away, even in Firefox 1.5 Beta 2 (which contained the fix for bug 298961). I think it's too bad, though, that you made a pretty fundamental change in the de-facto plugin interface without warning anyone about it ... at least outside the circle of those discussing this particular bug. (Side note: When you scroll the browser window, the plugin window switches back and forth several times a second between being zero-clipped and being "normally" clipped. Calling SetWndow() with pluginWindow == NULL only happened on switching tabs or creating a new tab (actually it didn't always happen even then, just most of the time). Because the pluginWindow == NULL signal was used only when it was actually needed, when I received it I used a relatively expensive way of making the plugin invisible -- something far too expensive to use several times a second (I actually tried it yesterday). It turns out I don't really need this "expensive" set-invisible method in Carbon browsers. But I _did_ need it in Cocoa browsers ... at least until I found a workaround. Without the "expensive" set-invisible method or some kind of workaround, parts of applets with dynamic displays (like the NOAA weather applets) would leak out from a background tab into the foreground tab. For those who are curious, my workaround (the one that made it unnecessary for me to use the "expensive" set-invisible method even in Cocoa browsers) is contained in the MyNSView _propagateDirtyRectsToOpaqueAncestors method in Controller.m.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, I didn't mean to reopen this bug.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.