nsIWidget::GetNativeData(NS_NATIVE_PLUGIN_PORT) should return native XID on unix

VERIFIED FIXED

Status

()

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

People

(Reporter: blizzard, Assigned: Stuart Parmenter)

Tracking

({arch, embed})

Trunk
x86
Linux
arch, embed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta3-] [embed+] [rtm++])

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
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.
(Assignee)

Comment 1

18 years ago
nominating for nsbeta3.  I think blizzard's comment says enough.
Status: NEW → ASSIGNED
Keywords: correctness, nsbeta3

Comment 2

18 years ago
nsbeta3-, don't see any reason to hold NS6 for this
Whiteboard: [nsbeta3-]
Target Milestone: --- → Future

Updated

18 years ago
Keywords: arch

Comment 3

18 years ago
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.
Keywords: rtm

Comment 4

18 years ago
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
Keywords: embed
Priority: P3 → P2
Whiteboard: [nsbeta3-] → [nsbeta3-] [embed+]
Target Milestone: Future → M19
(Assignee)

Comment 5

18 years ago
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).

Comment 6

18 years ago
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).
(Assignee)

Comment 7

18 years ago
this has *NOTHING* to do with plugin *WRITERS*
(Reporter)

Comment 8

18 years ago
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.
(Assignee)

Comment 9

18 years ago
if plugin writers had to care about it, then flash, etc wouldn't work.
(Assignee)

Comment 10

18 years ago
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.
(Reporter)

Comment 11

18 years ago
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.
(Assignee)

Comment 12

18 years ago
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.
(Reporter)

Comment 13

18 years ago
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.
(Assignee)

Comment 14

18 years ago
Created attachment 16549 [details] [diff] [review]
something like this
(Assignee)

Comment 15

18 years ago
this seems to work for me.. i've tested it with flash and the simple plugin...
it probably needs some more error checking though.
(Reporter)

Comment 16

18 years ago
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().

Comment 17

18 years ago
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?
(Reporter)

Comment 18

18 years ago
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.
(Reporter)

Comment 19

18 years ago
sr=blizzard
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 ekrock@netscape.com and
petitta@netscape.com. Thanks!

Comment 21

18 years ago
marking rtm+.
Whiteboard: [nsbeta3-] [embed+] → [nsbeta3-] [embed+] rtm+

Comment 22

18 years ago
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+]

Comment 24

18 years ago
rtm++
Whiteboard: [nsbeta3-] [embed+] [rtm+] → [nsbeta3-] [embed+] [rtm++]
(Assignee)

Comment 25

18 years ago
its on the netscape branch.  now for the trunk
(Assignee)

Comment 26

18 years ago
checked in on the trunk, branch, embedding branch.. hope thats it.  marking fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 27

18 years ago
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.
(Reporter)

Comment 28

18 years ago
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.

Comment 29

18 years ago
verified that the patch is in the branch (version 1.34). Adding vtrunk keyword.
Keywords: vtrunk

Comment 30

18 years ago
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.

Comment 31

18 years ago
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.

Comment 32

18 years ago
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.
(Reporter)

Comment 33

18 years ago
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.

Comment 34

18 years ago
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).
(Assignee)

Comment 35

18 years ago
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.
(Reporter)

Comment 36

18 years ago
Maybe it's what's being discussed in bug #57046?

Comment 37

18 years ago
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).
(Reporter)

Comment 38

18 years ago
Does this help with the problem that you describe in 57046 ( Applets crash
mozilla )?

Comment 39

17 years ago
v
Status: RESOLVED → VERIFIED
Keywords: vtrunk
You need to log in before you can comment on or make changes to this bug.