For backwards compatibility and to allow the greatest flexibility we should be returning the native XID for plugin ports. Gtk plugins get get the associated GdkWindow from that port and from there get the GtkWidget if they care. Other toolkits ( Xt for example ) can use that as a parent and create their own application context.
nominating for nsbeta3. I think blizzard's comment says enough.
Status: NEW → ASSIGNED
Keywords: correctness, nsbeta3
nsbeta3-, don't see any reason to hold NS6 for this
Target Milestone: --- → Future
if we ship with this, we're bound to a broken api and plugin developers aren't going to want to write plugins for both apis.
Pav, please take a look at this. Embedders are stuck getting Flash plugin support because of this, because it adds a dependency on GTK, which they remove to reduce footprint. Marking embed+, will reconsider for rtm+. cc jrgm
Priority: P3 → P2
Whiteboard: [nsbeta3-] → [nsbeta3-] [embed+]
Target Milestone: Future → M19
this has nothing to do with plugin developers. this is a mozilla specific issue that requires either this bug to be fixed or people such as the xlib guys to add a patch to our plugin code that makes plugins work with their toolkit. The xlib guys have a bug and a patch that makes the xlib port work with plugins again (55278).
I'm not following how an xlib patch would solve this problem. the new plugin api requires a gdksuperwin obj, which some environments might not have. We need to nail *this* bug to reduce dependencies on plugin writers *and* to maintain platform partity w/ windows and mac (which provide lower level window object access).
this has *NOTHING* to do with plugin *WRITERS*
Pav, are you sure about that? From just a cursory examination of the interfaces it looks like we pass information about the window back to a plugin instance in at least one place nsIPluginInstance::SetWindow(): http://lxr.mozilla.org/seamonkey/source/modules/plugin/public/nsIPluginInstance.h#142 Have a look at SimplePluginInstance::PlatformSetWindow() in the sample code which is exactly what people writing plugins have to do: http://lxr.mozilla.org/seamonkey/source/modules/plugin/test/npsimple.cpp#934 Although I think the size issue that keeps getting brought up is pretty bogus, asking plugin writers to know or care about the superwin code is a big problem. I want to get away from it and will with gtk 2.0 but we need to get away from this for plugin writers.
if plugin writers had to care about it, then flash, etc wouldn't work.
ok.. um. I think that there are some issues here.. (mainly I'm really confused about how plugins work now). I don't understand how we manage to get the correct window from old plugins. Somewhere (which I can't seem to find) we are creating a superwin that wraps the native XWindow (I assume) with either a superwin or an nsIWidget.. i'm going to guess an nsIWidget since I don't see any calls to gdk_superwin_new outside of widget... Blizzard, can you create an nsIWidget with a XID with the gtk implimentation? I think now that we should definatly fix this, but I've got to get a clear understanding of what is going on with the plugin code and why things are working now before I can do this.
Uhh...you're thinking of the 4.x plugin compatibility stuff which takes care of all of this for you. I'm talking about the new plugin apis. We obviously can't change the 4.x api at all.
we pass out a GdkSuperWin in the old 4.x code... who added the superwin stuff to the new plugin apis? I don't know anything about that code... the only plugin code I have worked on is the 4.x compat code.
No, it isn't. What is passed to the _plugin_ is the X11 window is in the ws_info member for 4.x compatibility. What is passed to the _4.x plugin instance code_ is the superwin. You create the X11 window from that. This means that the plugin instance code for 4.x has to be changed as well, but not in a way that breaks any compatibility with 4.x plugins at all. I say again, what does change and what the embedding people are whining about is the fact that the new style plugins are passed the superwin and not the raw X window of the inner window.
this seems to work for me.. i've tested it with flash and the simple plugin... it probably needs some more error checking though.
See, easy as pie. Yeah, you might want to do some more error checking. For example, in GetNativeData() make sure that mSuperWin is non-null before dereferencing it. I've seen some bad users in the past that try to use it before Init() or after Destroy().
I don't see any more room for error checking (based on the new diffs). blizzard, the mSuperWin null-check is already made (few lines above it's deref). Can the #include "gdksuperwin.h" line be removed from the 4xplugininstance file?
OK, I didn't look at the source code around it just the diff. I wondered if that check was already there. And to answer your question, I think that you can remove the include from the ns4x plugin file. However, it still requires gtk as you can tell, just not the superwin structure.
IIUC you've got a patch for this, it's already super-reviewed, and it should be marked rtm+ for PDT review and approval shortly. If this bug is *not* going to make RTM for any reason, please email email@example.com and firstname.lastname@example.org. Thanks!
Whiteboard: [nsbeta3-] [embed+] → [nsbeta3-] [embed+] rtm+
checked into the embedding branch.
PDT: If you're considering minusing this, call ekrock so I can rope in engineering and explain why we have to accept this for RTM. The issue is that we *have* to accept this for the embedding branch as this bug totally horks plug-ins in the embedded context. If we don't accept it on the branch and trunk as well, we've just forked the plug-in API between Netscape 6 and the trunk/embedding branch, and plug-in developers won't be willing to create, distrtibute, and maintain two versions of their plug-in. In a nutshell, this is one of those situations where if we don't get this right in the first release, we've given ourselves an irreparable self-inflicted wound forever. So please accept the patch into the branch.
Whiteboard: [nsbeta3-] [embed+] rtm+ → [nsbeta3-] [embed+] [rtm+]
Whiteboard: [nsbeta3-] [embed+] [rtm+] → [nsbeta3-] [embed+] [rtm++]
its on the netscape branch. now for the trunk
checked in on the trunk, branch, embedding branch.. hope thats it. marking fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
pavlov (just reading this bug traffic after some time off): Java Plug-In is a new-style plugin, and it broke as a result of this bug fix. So, to address your comment of 2000-10-05 22:48, plugin writers *do* care about this bug; at least, new-style plugin writers. I don't know why the Flash plugin continued to work; perhaps it's just an old-style plugin. ekrock: Sun agrees that this change should be made, but for the record, this broke Java Plug-In working in recent builds. I learned today that while I was gone, Ed Burns brought some Java bits to Netscape RE last week. Those bits no longer work with current builds. The fix is minor and easily understood, I believe, but still requires a re-spin of Java Plugin. I hope we get accepted! For more info, please see bug 57046 about the breakage on Linux/Solaris.
The flash plugin was an older 4.x plugin so was unaffected by this change. However, I thought that the Java plugin was also a 4.x style plugin as well otherwise I would have brought the java people into this. Sorry about that.
verified that the patch is in the branch (version 1.34). Adding vtrunk keyword.
Well, maybe I don't understand smth, but why Blizzard's fix should work on big endian 32 bits platforms (really on all, but on little endian it occasionally works). Let's see /usr/openwin/include/X11/X.h:typedef unsigned long XID; sizeof(void*) = 4 nsWindow.cpp: return (void *)GDK_WINDOW_XWINDOW(mSuperWin->bin_window); So we just lost 4 bytes (on little endian it's upper, on big endian lower). I think this bug must be reopened as it prevents all new style plugins from working on big endian platforms. Dixi.
blizzard: that's okay, unless you're Mr. Guardian Of All New-Style Plugins, I don't see how you could really know how all of us could be affected. I don't know of anybody keeping track of all the new-style plugins on the Unix platforms. Nikolay: I don't think the problem you're seeing is a byte truncation problem. It could be byte-ordering, but not truncation. I don't know what's causing that problem. Sounds like you have some new info to post to this bug report, though, that happens when you're running on a big-endian CPU but displaying on an X server that is running on a little-endian CPU.
Yeah, George you are right. Sorry Blizzard :). Really I've got strange problem - it's OK for big-endian prog on big-endian display, but broken when big-endian prog on little-endian display. I just don't understand why it happens.
Aww, crap. This is a very serious problem. I hadn't considered that the X window ID might be an unsigned long. This means that it isn't going to fit into a pointer on some platforms. We pass it through a pointer size in two places - as a result from GetNativeData() and through nsPluginWindow.window. We can't change this since I think that the 4.x compatibility stuff on all platforms requires this alignment of variables in the struct.
Blizzard, I don't think it's serious problem, as X headers takes care that size of XID == 32 (_XSERVER64 define). But I yet don't understand why I have a problem with endiannes mixing. Really it seems XReparentWindow hack doesn't work and window isn't reparented on Linux display (althoug XID is correct).
so long as an XID is 32bits, we should be ok. I don't know why you would be having endianness problems. the size problem would only kick in on 64bit machines if it was just an unsigned long on them.
Maybe it's what's being discussed in bug #57046?
Yes blizzard, it's true. XSync() really helps. I think it happens as JDK and Mozilla uses different X server connections, so until you'll XSync() parent window XID is invalid for another X connection(JDK).
Does this help with the problem that you describe in 57046 ( Applets crash mozilla )?
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.