544088 - Xt plugins broken by switch to OOPP (not implemented)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Karl Tomlinson (:karlt)]

STR:
1) load data:text/html,<embed type="application/x-print-unix-nsplugin">
2) if the plugin process is still running, cause a repaint,
   e.g. by covering the plugin with another window or switching tabs.

AR: plugin process exits with error message

nsPluginNativeWindowGtk2: NPPVpluginNeedsXEmbed=0
About to create new xtbin of 240 X 200 from 0x44001cd...
About to show xtbin(0x7f939b390360)...
completed gtk_widget_show(0x7f939b390360)
nsPluginNativeWindowGtk2: call SetWindow with xid=0x4e00001
-1159444656[7f93af410040]: NPError mozilla::plugins::PluginInstanceParent::NPP_SetWindow(const NPWindow*) (aWindow=7f939b3cbc88)
-1534514864[f5fa90]: virtual bool mozilla::plugins::PluginInstanceChild::AnswerNPP_SetWindow(const mozilla::plugins::NPRemoteWindow&, NPError*) (aWindow=<window: 0x4e00001, x: 8, y: 8, width: 240, height: 200>)
Error: Couldn't find per display information

Comment 2

[Johnny Stenback (:jst)]

Karl, is this something that breaks plugins other than our test plugin as well?

Comment 3

[Benjamin Smedberg]

Yes, this breaks a few known plugins in the wild, including Acrobat and VLC.

Comment 4

[Karl Tomlinson (:karlt)]

VLC and Acroread's plugins are Xt plugins.  For those plugins we currently workaround this with an OOPP-blacklist.

I don't know how many other plugins there might be that need Xt support.
The original plugin model was based on Xt, so any plugins that follow our demo plugins will depend on Xt.

Picking which plugins to run OOPP based on whether they need XEmbed might be a possible but tricky option.
If a plugin *instance* needs XEmbed then it does not need Xt support.
(Some plugins that do not say they need XEmbed might also work even without Xt support.)
The tricky part is that we need an instance to query for XEmbed before we decide which process will create the instance.

I wonder whether there are similar issues with deciding which Mac plugins to run OOP.

The ideal solution would be to run all plugins OOP, including Xt plugins.
This is possible but will take something like a week's work.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

I volunteer to help with it.

Comment 6

[Johnny Stenback (:jst)]

Ok, sounds like we should deal with this for 1.9.3 then, and thanks for your offer to help Martin! I suggest you find karlt on irc.mozilla.org (in #developers) and you guys can work out the details on how to approach this.

Comment 7

[Karl Tomlinson (:karlt)]

Thank you, Martin.

For in-process plugins, GtkXtBin (which is not a GTK Type) currently uses a
GtkSocket for the XEmbed embedder implementation and provides an "XtClient" (which is not an Xt type) XEmbed client implementation.

The GtkSocket and XtClient can be in different processes (though XtClient may
need some modification for this to work), so my idea was to expose sufficient
XtClient API so that the plugin process could use the XtClient without a
GtkXtBin.

The browser process would then treat the plugin process as an XEmbed plugin,
the plugin process would use XtClient to behave like an XEmbed client, and for
NPP_SetWindow, the plugin process would present
XtWindow(XtClient::child_widget) to the plugin in NPWindow.window.

The plugin process will also need to provide the Xt Display (not the GDK
display) to the plugin in NPSetWindowCallbackStruct::display for
NPP_SetWindow and in response to NPN_GetValue NPNVxDisplay.

I'm not clear on what Set/LoseNonXEmbedPluginFocus in nsWindow.cpp do, but I'm
hoping that there is no need to worry about that.

BTW, the timezone here is +1200, so I may be difficult to find on irc.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Thanks. One question - can the XtClient::xtwindow (derived from XtClient::child_widget by gtk_xtbin_new()) be created in browser process and passed to the plugin one? (it's the current state) Or do I have to create it in the plugin process and connect it to browser by XEmbed?

Comment 9

[Karl Tomlinson (:karlt)]

I haven't tested, but I suspected that Xt plugins would assume that the Window belonged to an Xt Widget (in the plugin process).  If the Window were created in the browser process then the plugin wouldn't know that that Xt was involved, and so there would be no reason to use Xt to create the window.

I also suspect that merely using a Window to communicate between processes would lose the inter-process focus handling features provided by XEmbed and xt_client_event_handler.

So AFAIK the Window needs to be created in the plugin process using Xt and connected to the browser by XEmbed.  Even if using a Window from the browser process can be made to work somehow, the Xt Display and event loop connections would still need to be in the plugin process.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Is there any config I can use to distinguish that the plugin is not running as oopp? I think we still want to support it...

Comment 11

[Karl Tomlinson (:karlt)]

There is this code to determine whether to run plugins OOP
http://hg.mozilla.org/mozilla-central/annotate/d4e5f5f19777/modules/plugin/base/src/nsNPAPIPlugin.cpp#l310
but that may not be what you want.

From where are you wanting to work out whether the plugin is OOP?
(So far our nsObjectFrame/nsNPAPIPlugin code hasn't needed to behave differently.)

Comment 12

[Martin Stránský [:stransky] (ni? me)]

In nsPluginNativeWindowGtk2::CallSetWindow(). From my tests the Xt window can't be shared/transferred between browser/plugin processes so it's pointless to create it as xtbin widget and propagate it to the plugin.

Comment 13

[Karl Tomlinson (:karlt)]

Can the plugin wrapper PluginInstanceParent::NPP_GetValue always reply true to NPPVpluginNeedsXEmbed (even when the plugin is an Xt plugin)?

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Sure. But I expect we still want to keep the ability to run plug-ins w/o oopp, right? May it be configured by some pref?

Comment 15

[Karl Tomlinson (:karlt)]

If the plugin is not OOP then PluginInstanceParent::NPP_GetValue won't be used; instead the plugins own NPP_GetValue will return the appropriate value.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Attachment: wip1

What about this? I think it's the most simple solution where GtkXtBin() is just moved from browser to plugin process.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 448363 [details] [diff] [review]
wip1

(In reply to comment #16)
> What about this? I think it's the most simple solution where GtkXtBin() is just
> moved from browser to plugin process.

Adding another XEmbed layer seems unnecessary and so not ideal to me, but I
haven't tried using XtClient directly and so don't know how complex that would
be.

I had a play with some Xt plugins and noticed that the focus support of our
in-process implementation is very poor: changing focus when mousing over the
plugin, but not changing UI to indicate the change (i.e. continuing to blink
the caret in unfocused textboxes).  Given that, there doesn't seem to be a
good implementation to compare this new implementation against, and so I have
less motivation to ensure things are well polished here.

If you think that using GtkXtBin directly is going to be the tidiest solution
then that will be good enough here.

One thing to note is that XEmbed embedders usually resize their clients so I
would guess that not all of the resizing in this patch is necessary.

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Attachment: WIP v.2 - Xt only

A basic Xt version, the code is derived from gtk2xtbin and nspluginwrapper project, some code from nspluginwrapper is still commented out.

Comment 19

[Martin Stránský [:stransky] (ni? me)]

I wonder how to support the windowless plug-ins. What about to map a shared memory segment (via. mmap) and put the cairo drawable there? It could be transferred then between browser/plugin process then.

Comment 20

[Karl Tomlinson (:karlt)]

We don't need to worry about Xt windowless plug-ins.
Windowless plugins are neither XEmbed nor Xt (and we don't provide an Xt display for windowless plugins even in-process).

Comment 21

[Martin Stránský [:stransky] (ni? me)]

Really? There are at least two windowless plug-ins that does not work (on Fedora 12). The one from modules/plugin/sdk/samples/basic/unix and http://multimedia.cx/diamondx/ in windowless/transparent mode. I did the same tests with nspluginwrapper and the results are the same (i.e. they're broken).

And how can be XEmbed mixed with windowless mode anyway? AFAIK for windowless plugin mozilla creates an offscreen cairo surface and passes it to plugin to draw itself into it, for XEmbed mode it passes X window handle.

Or am I missing something?

Comment 22

[Martin Stránský [:stransky] (ni? me)]

Attachment: wip v.3

Fixed display/xt loop setup. It works fine with adobe reader plugin, but keyboard input is still unsupported. I'll fix it next week + clean it for review.

Comment 23

[Karl Tomlinson (:karlt)]

(In reply to comment #21)
> Really? There are at least two windowless plug-ins that does not work (on
> Fedora 12). The one from modules/plugin/sdk/samples/basic/unix and

It's unfortunate that that is our sample plugin because this gdk_window_foreign_new will fail (OOP) because expose->drawable is a Pixmap.
(It's just lucky that it mostly works in-process.)

http://hg.mozilla.org/mozilla-central/annotate/adf19efa718f/modules/plugin/sdk/samples/basic/unix/BasicPlugin.c#l247

This is better, 
http://hg.mozilla.org/mozilla-central/annotate/634e1f2cf46c/modules/plugin/test/testplugin/nptest_gtk2.cpp#l340

though even that is assuming that the browser is using GDK.

> http://multimedia.cx/diamondx/ in windowless/transparent mode.

That was written before the X11 windowless API was settled, and was just intended to be extended for windowless when the API was ready.

There are some patches to make it work here, though I'm not sure it's appropriate for an example plugin
https://bugzilla.mozilla.org/show_bug.cgi?id=386144

> And how can be XEmbed mixed with windowless mode anyway? AFAIK for windowless
> plugin mozilla creates an offscreen cairo surface and passes it to plugin to
> draw itself into it, for XEmbed mode it passes X window handle.

Yes, windowless and XEmbed don't mix.   XEmbed gets an X Window and windowless gets an X Pixmap (which, inside mozilla, is used as a cairo surface).

Comment 24

[Martin Stránský [:stransky] (ni? me)]

Attachment: xt plugins support

t focus handler is derived from nspluginwrapper project so we may note it somewhere...maybe in "Contributors"?

Comment 26

[Karl Tomlinson (:karlt)]

Sorry I'm slow here, Martin.  I'll try to look more at this soon.

(In reply to comment #24)
> Xt focus handler is derived from nspluginwrapper project so we may note it
> somewhere...maybe in "Contributors"?

Yes, adding the author(s) to Contributors makes sense, but most of nspluginwrapper is under GPL2 (or later), so if the code is derived from files with that header, then we can only distribute it under MPL1.1 GPL2 and LGPL2.1 with the author's permission.

Would you be able to see if the author can give permission, please?

Comment 27

[Martin Stránský [:stransky] (ni? me)]

The code comes from nspluginwrapper/src/npw-viewer.c which is under GPL2 (or later). I'll ask Gwenole for permission but he was unreachable when I tried to talk to him last time. 

Anyway, why is the author permission requested, because of MPL? If so I can put the parts from nspluginwrapper to a separate file which would be under GPL2 only.

Comment 28

[Benjamin Smedberg]

GPL-only code is not allowed in the Mozilla tree. All of our code must be compatible with the tri-license.

Comment 29

[Martin Stránský [:stransky] (ni? me)]

Okay, I'll provide a patch derived from GtkXtBin, without nspluginwrapper code. I have to merge it with latest trunk anyway.

Comment 30

[Karl Tomlinson (:karlt)]

If it's possible to share code with GtkXtbin, by exporting XtClient (or something), that would be great.  That could also help avoid adding too much more platform-specific code to the dom/plugins files.

Comment 31

[Martin Stránský [:stransky] (ni? me)]

Attachment: xt plugins support, linked to GtkXtbin

That's a great idea! There's the patch attached, it uses XtClient and runs GtkXtbin focus handlers directly.

Comment 32

[Gwenole Beauchesne]

Hi,

(In reply to comment #26)
> Yes, adding the author(s) to Contributors makes sense, but most of
> nspluginwrapper is under GPL2 (or later), so if the code is derived from files
> with that header, then we can only distribute it under MPL1.1 GPL2 and LGPL2.1
> with the author's permission.
> 
> Would you be able to see if the author can give permission, please?

Yes, of course. But what form do you want? What do I need to do? Sorry for slowness, I missed Martin's mail from the weekend.

Comment 33

[Karl Tomlinson (:karlt)]

Thank you very much, Gwenole.

I understand that Martin's latest patch no longer uses code from nspluginwrapper.  (At least some has been replaced by similar code that was already in Gecko.)
If there are any parts remaining, I'll ask Martin to identify, so you can confirm here that you are the author and happy that the code is used under the new licences.

Comment 34

[Martin Stránský [:stransky] (ni? me)]

(In reply to comment #33)
> Thank you very much, Gwenole.
> 
> I understand that Martin's latest patch no longer uses code from
> nspluginwrapper.  (At least some has been replaced by similar code that was
> already in Gecko.)
> If there are any parts remaining, I'll ask Martin to identify, so you can
> confirm here that you are the author and happy that the code is used under the
> new licences.

I reviewed the patch again and I believe there are not any parts from nspluginwrapper in the new patch. XEmbed focus handlers are called from GtkXtbin widget and X event handlers are derived from it too.

Comment 35

[Karl Tomlinson (:karlt)]

Comment on attachment 468296 [details] [diff] [review]
xt plugins support, linked to GtkXtbin

>+        XtSetArg(args[n], XtNoverrideRedirect, True);n++;

Can you explain the purpose of override-redirect, please?
Override-redirect seems counter to the XEmbed protocol.

>+        XtSetArg(args[n], XtNborderWidth, 0);n++;
>+        XtSetArg(args[n], XtNbackgroundPixmap, None);n++;

>+        /* Attach Xt shell to browser socket */
>+        XtRealizeWidget(mTookitData.top_widget);
>+        XReparentWindow(mWsInfo.display, XtWindow(mTookitData.top_widget), 
>+                        browserSocket, 0, 0);
>+        XtRealizeWidget(mTookitData.child_widget);

It looks like this is using a slightly different technique to what
xt_client_create is using.  This looks more appealing than tricking Xt to
believe that the top_widget has been realized, but can you explain the pros
and cons of the two methods, please?

I'd like this code to be shared between IP and OOP plugins, and I think it
should be possible to have the same code working in both cases.
The code in PluginInstanceChild::CreateWindow should be able to be replaced by
a call to xt_client_create.

I assume that there are improvements that can be made to xt_client_create,
but are changes actually necessary for OOPP?  (Or can they be moved to a
separate patch?)

Is the realize before reparent the reason for override-redirect?
It looks like it would be more appropriate to set XtNmappedWhenManaged to
False and then use XEMBED_MAPPED to request that the embedder (socket) maps
the client.

> bool
> PluginModuleChild::InitGraphics()
> {
> #if defined(MOZ_WIDGET_GTK2)
>     // Work around plugins that don't interact well with GDK
>     // client-side windows.
>     PR_SetEnv("GDK_NATIVE_WINDOWS=1");
> 
>+#if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)
>+    char *mArgv[1];
>+    int   mArgc = 0; 
>+    XtToolkitInitialize(); 
>+    XtAppContext app_context = XtCreateApplicationContext();
>+    mXtDisplay = XtOpenDisplay(app_context, NULL, "Wrapper", "Wrapper", NULL, 0,
>+                              &mArgc, mArgv);
>+#endif

I'd prefer not to open this display connection from all plugin containers, but
only from those that need Xt.  The X server has a limit on the number of
clients, so we should not use that resource unnecessarily.  (I guess
the same argument would apply to the GTK connection, but don't worry about
that here - at least most plugins use that connection.)

>+    // Sync mTookitData for XtClient
>+    if(!XEmbed) {
>+      mTookitData.xtdisplay = mWsInfo.display;
>+      mTookitData.xtcolormap = mWsInfo.colormap;
>+      mTookitData.xtvisual = mWsInfo.visual;
>+      mTookitData.xtdepth = mWsInfo.depth;
>+    }

This is what xt_client_init does, only xt_client_init also handles opening the
display on demand.  I expect xt_client_init could be called just before
xt_client_create.  I don't know why xt_client_init and xt_client_create are
separate functions, but exporting both would be fine.

PluginModuleChild may sometimes need the Xt display before a
client has been created.  One way to handle that would be to call
xt_client_init on a dummy client.  This would be similar to our IPP code and,
with the g_source changes below, would hook up into the event loop.
I don't know whether the event loop is needed without a client window;
if not, the other option is adding an xt_client_get_display method to
gtk2xtbin.

>+PluginModuleChild::InitXtLoop()

Whenever code is being copied from one place to another it is usually an
indication that the code needs to be refactored so that it can be reused.

What seems necessary here is that the num_widgets and g_source code in
gtk_xtbin_new and gtk_xtbin_destroy is moved to xt_client_init and
xt_client_destroy.

>@@ -936,17 +1110,24 @@ _getvalue(NPP aNPP,

>+        case NPNVxDisplay:
>+          *(void **)aValue = (void *)PluginModuleChild::current()->mXtDisplay;
>+          return NPERR_NO_ERROR;

I think this should return the default (GDK) display for non-Xt plugins.
(Might want to forward this onto the instance if aNPP is non-NULL.)

>+        case NPNVxtAppContext:
>+          *(void **)aValue = (void *)XtDisplayToApplicationContext(PluginModuleChild::current()->mXtDisplay);
>+          return NPERR_NO_ERROR;

This is probably correct, but currently Mozilla doesn't support this, even for
in-process plugins.  I don't think we want to actually _extend_ the support
offered to Xt plugins (unless there's a good reason), and this looks like
something the plugin could discover from the display (using
XtDisplayToApplicationContext).

http://hg.mozilla.org/mozilla-central/annotate/8d87fd383e60/modules/plugin/base/src/nsNPAPIPlugin.cpp#l1930

>@@ -1753,25 +1934,43 @@ PluginModuleChild::AnswerPPluginInstance

>+    // FIXME/cjones: use SAFE_CALL stuff too?
>+    if(mFunctions.getvalue) {
>+      *rv = mFunctions.getvalue(npp, 
>+                                NPPVpluginNeedsXEmbed,
>+                                (void *)&mXEmbed);
>+      if (NPERR_NO_ERROR != *rv) {
>+          mXEmbed = FALSE;

It's not quite right to ask each instance whether it is XEmbed but only keep
one result on the ModuleChild.  I guess we can make the assumption that one
plugin won't have XEmbed and Xt instances, if necessary, but it would be
better if mXEmbed was on the instance.  The NPPVpluginNeedsXEmbed call is also
unnecessary for windowless plugins, and I'm not sure that we can assume that
the result from windowless plugins would be meaningful.

>+        nsIPluginInstance *inst = aPluginInstance;
>+        PRBool aOOPPlugin = ((nsNPAPIPluginInstance *)inst)->mOOPPlugin;

No need for the intermediate variable |inst| here.
isOOPPlugin is a better name than aOOPPlugin.  a* is used for parameters.

>                 else if(GTK_IS_SOCKET(widget)) {
>+                    if(!g_object_get_data(G_OBJECT(widget), "enable-xt-focus")) {
>                     nswindow->SetPluginType(nsWindow::PluginType_XEMBED);
>                     break;
>                 }
>             }
>+            }

Indentation.

Josh should probably review the nsNPAPIPlugin(Instance) changes.

Comment 36

[Martin Stránský [:stransky] (ni? me)]

Attachment: requested changes, wip

There is the patch with requested changes here. But it seems to have buggy expose events or something, the plugin redraw is slower than with the previous one and sometimes it fails completely.

Comment 37

[Karl Tomlinson (:karlt)]

Is there a reason why xt_client_xloop_create() can't be called (automatically) from xt_client_init() instead of from PluginModuleChild?

Comment 38

[Martin Stránský [:stransky] (ni? me)]

(In reply to comment #37)
> Is there a reason why xt_client_xloop_create() can't be called (automatically)
> from xt_client_init() instead of from PluginModuleChild?

I think It could be called from xt_client_init(). The reason I have it here is that I was getting crashes from Adobe reader plugin when the X loop was started from NPP_SetWindow but it could be caused by wrong display or something else.

But I think it's more clear to launch the X loop when first plugin instance is issued and stop it when the last instance is removed and have those two places near by.

Comment 39

[Martin Stránský [:stransky] (ni? me)]

Attachment: v7, merged with latest trunk

This one should address it. 

As for the XtNoverrideRedirect flag, I'm not sure what the option does in this specific case, but without it the plugin window is sometimes blank after pdf reader load.

Comment 40

[Karl Tomlinson (:karlt)]

I don't seem to be able to reproduce any redraw failure with redirect forced to zero, full page documents, and Acroread 9.4.0.
I see window resizing is often slow, but not detectably any slower than in-process.

Can you describe some steps to reproduce, please?

Comment 41

[Martin Stránský [:stransky] (ni? me)]

I reproduced it on Fedora 13/x86_64/32-bit ff build. 

Try to load quickly more different pdf documents to different tabs. Sometimes some of the documents is loaded (window title is set) but the main plug-in window is not shown.

Another testcase is to grab a tab with pdf plugin and drop it on window area. The tab is reopened as a new window but only as a blank one.

Comment 42

[Karl Tomlinson (:karlt)]

Thanks.  I can reproduce now.

I'm also getting a crash due to null xtdisplay when loading
data:text/html,<embed%20width="98%"%20height="98%"%20type="application/x-vlc-plugin">

#5  0x00007f3d9ec52c8b in xt_client_xloop_create ()
    at /home/karl/moz/dev/widget/src/gtkxtbin/gtk2xtbin.c:514
#6  0x00007f3d9e93801b in mozilla::plugins::PluginModuleChild::AnswerPPluginInstanceConstructor (this=0x7f3d91c0d030, aActor=0x7f3d91c918c0, aMimeType=..., 
    aMode=@0x7fffc97c5f0e, aNames=..., aValues=..., rv=0x7fffc97c5eee)
    at /home/karl/moz/dev/dom/plugins/PluginModuleChild.cpp:1825
#7  0x00007f3d9ea3b10e in mozilla::plugins::PPluginModuleChild::OnCallReceived
    (this=0x7f3d91c0d030, __msg=..., __reply=@0x7fffc97c6068)
    at PPluginModuleChild.cpp:636

Comment 43

[Martin Stránský [:stransky] (ni? me)]

Attachment: v8

Added X display check to xt_client_xloop_create().

Comment 44

[Karl Tomlinson (:karlt)]

(In reply to comment #39)
> As for the XtNoverrideRedirect flag, I'm not sure what the option does in this
> specific case, but without it the plugin window is sometimes blank after pdf
> reader load.

(In reply to comment #41)
> Sometimes
> some of the documents is loaded (window title is set) but the main plug-in
> window is not shown.

Yes, the plugin window is mapped and sized correctly, but the window from the acroread process (an inferior of the plugin-container) is not initialized.
It is not mapped, not sized correctly, has no child windows (which is normally would have), and the WM_NAME is "acroread" instead of the usual document title.

Once this starts happening, existing plugins continue to draw, but subsequent plugins show the same problem.

I'll see if I can work out what's different here.
I can't think of a good reason why OOP should be different, given that even in-process the plugin was already on a separate display.
override-redirect is not appropriate for XEmbed as the window is meant to be managed by the embedder, so i'd like to find a different solution.

Comment 45

[Benjamin Smedberg]

I think at this point we should just keep the blacklist prefs for FF4, I think... any reason we *need* to take this as long as acrobat/VLC are kept in-process?

Comment 46

[Martin Stránský [:stransky] (ni? me)]

The only plug-in which causes such troubles are acrobat, right? If there's some with open sources with such symptoms it will be much easier to debug it.

Comment 47

[Karl Tomlinson (:karlt)]

(In reply to comment #38)
> (In reply to comment #37)
> > Is there a reason why xt_client_xloop_create() can't be called (automatically)
> > from xt_client_init() instead of from PluginModuleChild?
> 
> I think It could be called from xt_client_init(). The reason I have it here is
> that I was getting crashes from Adobe reader plugin when the X loop was started
> from NPP_SetWindow but it could be caused by wrong display or something else.

nppdf.so calls XtAppAddInput during _NPPluginFuncs::newp(), so it should be
set up before SetWindow.

I was thinking it might be necessary to hook into the event loop when the
plugin first requested the Display (during NP_Initialize for nppdf.so) ...

> 
> But I think it's more clear to launch the X loop when first plugin instance is
> issued and stop it when the last instance is removed

... However, I like this idea of only doing this while there are plugin instances
active, as then we don't need to poll every 25 ms while there are no
existing instances, and I expect this should work.

Comment 48

[Karl Tomlinson (:karlt)]

Comment on attachment 488709 [details] [diff] [review]
v8

The symptoms that I described in comment 44 are unrelated to
XtNoverrideRedirect, but happen after destroying a plugin instance
due to problems reference counting the Xt event source.

>+    if (!mXEmbedConfigured) {      
>+      // FIXME/cjones: use SAFE_CALL stuff too?
>+      if(mFunctions.getvalue) {
>+        *rv = mFunctions.getvalue(npp, 
>+                                  NPPVpluginWindowBool,
>+                                  (void *)&mNeedWindow); 
>+        if (NPERR_NO_ERROR != *rv) {               
>+            mNeedWindow = TRUE;
>+        }
>+        if (mNeedWindow) {
>+          *rv = mFunctions.getvalue(npp, 
>+                                    NPPVpluginNeedsXEmbed,
>+                                    (void *)&mXEmbed);
>+          if (NPERR_NO_ERROR != *rv) {
>+              mXEmbed = FALSE;
>+          }
>+        }
>+      }
>+                
>+      if(mNeedWindow && !mXEmbed) {
>+        xt_client_xloop_create(); 
>+      }    
>+    
>+      mXEmbedConfigured = TRUE;
>+    }

xt_client_xloop_create is only called once (when !mXEmbedConfigured), but ...

> PluginModuleChild::DeallocPPluginInstance(PPluginInstanceChild* aActor)
> {
>     PLUGIN_LOG_DEBUG_METHOD;
>     AssertPluginThread();
> 
>+#if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)
>+    if(!mXEmbed) {
>+      xt_client_xloop_destroy();

... is called for each instance.

Also, _NPPluginFuncs::newp() may fail, in which case
PluginModuleChild::AnswerPPluginInstanceConstructor() will return early
without hooking Xt into the event loop, but DeallocPPluginInstance() will
still decrement the Xt event source reference count.

mXEmbed should and mNeedWindow must be an instance (not module) variable

I'd be inclined to do this handling on the PluginInstanceChild (rather than
the module).  PluginInstanceChild::Initialize() currently does nothing, so its
call from PluginModuleChild::AllocPPluginInstance() can be moved to
AnswerPPluginInstanceConstructor(), and so Initialize() and can manage these
variables as well as adding the Xt event source.

It looks like PluginInstanceChild::AnswerNPP_Destroy() is a suitable place to
decrement the reference count on the Xt event source, after
AnswerNPP_Destroy() arranges for _NPPluginFuncs::destroy() to be called.

Comment 49

[Karl Tomlinson (:karlt)]

(In reply to comment #45)
> any reason we *need* to take this as long as acrobat/VLC are kept in-process?

The main reason would be for the sake of unknown other Xt plugins, which would end up exiting early unless the user added blacklist prefs (comment 4).

Comment 50

[Karl Tomlinson (:karlt)]

Comment on attachment 488709 [details] [diff] [review]
v8

>+        *rv = mFunctions.getvalue(npp, 
>+                                  NPPVpluginWindowBool,
>+                                  (void *)&mNeedWindow); 

Instead of the browser querying the plugin, NPPVpluginWindowBool is actually used by the plugin calling NPN_SetValue() during newp().

PluginInstanceChild::NPN_SetValue could record this in mWindow.type (which would also need to be initialized to NPWindowTypeWindow).

Comment 51

[Karl Tomlinson (:karlt)]

(In reply to comment #50)
> PluginInstanceChild::NPN_SetValue could record this in mWindow.type (which
> would also need to be initialized to NPWindowTypeWindow).

Oh, you are already setting mWindowless there, so disregard this sentence.

Comment 52

[Benjamin Smedberg]

The PDF/VLC prefs are already in place, so this doesn't need to block.

Comment 53

[Martin Stránský [:stransky] (ni? me)]

Attachment: v9 with requested changes

Thanks for the review!

There's a patch with requested changes attached, without the XtNoverrideRedirect, seems to work fine.

Comment 54

[mmichalm6]

I have a plugin based on mozilla examples and it uses Xt. This plugin runs normally on FF3.6.4 but on FF4 plugin crashes when calling XtAddEventHandler with the following error:

Error: Couldn't find per display information
###!!! [Parent][RPCChannel] Error : Channel error: cannot send/recv
###!!! [Parent][RPCChannel] Error : Channel error: cannot send/recv
(firefox-bin:3529): Glib-GObject-CRITICAL **: g_object_get_data: 
assertion 'G_IS_OBJECT (object)' failed

So I assume it's same problem as problem discussed in this bug. I found that I can force FF4 to run all plugins in-process by setting dom.ipc.plugins.enabled in about:config to false. With these settings my plugin works fine.

How can I tell FF4 to run my plugin in-process? Is there another way how to run my plugin with FF4?

Thanks for you answers

Michal Maca

Comment 55

[Karl Tomlinson (:karlt)]

(In reply to comment #54)
> I have a plugin based on mozilla examples and it uses Xt. This plugin runs
> normally on FF3.6.4 but on FF4 plugin crashes when calling XtAddEventHandler
> with the following error:
> 
> Error: Couldn't find per display information

If this is a different plugin from the one in bug 638670, can you add a comment there with the name of the dynamic library loaded by the browser, and an indication of who uses the plugin, please?

> How can I tell FF4 to run my plugin in-process?

Users can add a specific boolean preference in about:config with a name like "dom.ipc.plugins.enabled.libmyplugin.so" and set the value to "false".

> Is there another way how to run my plugin with FF4?

If the plugin needs an X window, more plugins are using XEmbed now.
https://developer.mozilla.org/en/XEmbed_Extension_for_Mozilla_Plugins

Better still is if the plugin can use the windowless plugin API.
https://developer.mozilla.org/en/Gecko_Plugin_API_Reference/Drawing_and_Event_Handling#Windowless_Plug-ins

Comment 56

[William Bardwell]

So is there still no actual solution for this problem?  I have a plugin that breaks when run oopp due to using XtWindowToWidget, and I don't think that I can ask users to use about:config and expect that to both succeed and not **** them off.

Is there any way for a plugin that needs Xt to work right without weird user intervention under Firefox 4.0 on Linux?  It sounds like oopp is not really ready to go but it has been plastered over with the black list, so just less popular plugins get hosed.

So I guess I file a bug asking for my plugin to be added to the black list and hope that it gets turned around quickly.

Comment 57

[Karl Tomlinson (:karlt)]

Yes, please file a bug with a patch like that in bug 638670.

An alternative solution might be to package the plugin in an extension that sets the preference.  The idea being that the user installs the extension (which includes the plugin).

Comment 58

[Scott Talbert]

Is there some reason this patch never made it into trunk?  Were there any problems with the latest version of the patch?

Comment 59

[Karl Tomlinson (:karlt)]

The patch needs to be reviewed.  The main reason why this hasn't happened is that there have been more important things to review.

It turned out that there weren't that many Xt plugins around and we have workarounds for the known plugins that aren't being rewritten to work in Chromium also.

If there are other Xt plugins that we should know about then please make a case for them.

One thing I'm considering here is taking just enough of this patch so that Xt plugins render (and don't crash) but dropping the focus code at this stage because, even in process, that works poorly right now.

Comment 60

[Scott Talbert]

I wanted to try the patch in a more recent version of Firefox, so I went ahead and updated it so it applies to the current trunk.  (Hopefully this may be useful to someone.)

Comment 61

[Scott Talbert]

Attachment: patch updated to apply to current trunk

Comment 62

[Karl Tomlinson (:karlt)]

Attachment: export XtClient methods for OOP plugin support

Thanks, Scott.  This is the gtkxtbin part of the changes.

Comment 63

[Karl Tomlinson (:karlt)]

Attachment: remainder of changes

And this is the remainder of the changes, rebased for some needsXEmbed changes on m-c.

Comment 64

[Karl Tomlinson (:karlt)]

Comment on attachment 610406 [details] [diff] [review]
export XtClient methods for OOP plugin support

>+void xt_client_event_handler (Widget w, XtPointer client_data, XEvent *event);
>+void xt_client_focus_listener (Widget w, XtPointer user_data, XEvent *event);
>+void xt_client_set_info (Widget xtplug, unsigned long flags);

These methods no longer need to be exported, I assume.
Please don't export them unnecessarily as it is helpful to know that these
methods won't be called from outside this file.

Otherwise this part should be fine.

Comment 65

[Karl Tomlinson (:karlt)]

Attachment: support Xt plugins OOP

I corrected a bad merge that I did in attachment 610408 [details] [diff] [review].

This is now merged with cfd6bf0fe1e9.  The landing of bug 740217 will conflict
with changes to nsNPAPIPlugin.cpp in this patch.  However,

>+        bool isOOPPlugin = ((nsNPAPIPluginInstance *)aPluginInstance.get())->mOOPPlugin;

I would suggest renaming PluginLibrary::UseAsyncPainting() to IsOOP(), Then
this can be aPluginInstance.GetPlugin().GetLibrary().IsOOP() and the
nsNPAPIPlugin(Instance) changes in this patch are not necessary.

Probably best to make that API change in a separate patch so that Josh can
review that part.

>+        if (needsXEmbed || isOOPPlugin) {        
>+          bool enableXtFocus = isOOPPlugin && !needsXEmbed;
>+          rv = CreateXEmbedWindow(enableXtFocus);

"bool enableXtFocus = !needsXEmbed" is enough.
No need to check isOOPPlugin again.

> PluginInstanceChild::Initialize()
> {
>+#if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)
>+    // Missing NPP_GetValue?
>+    if (mPluginIface->getvalue) {
>+        NPError rv;
>+        bool windowed;
>+    
>+        rv = mPluginIface->getvalue(GetNPP(), NPPVpluginWindowBool,
>+                                   (void *)&windowed);
>+        mWindowless = (NPERR_NO_ERROR == rv) ? !windowed : FALSE;

The browser can't query the plugin to find out if it is windowless.
Instead, the plugin requests windowless mode during newp (Comment 50).

That means that the initialization here can't happen until after (or perhaps
sometimes during newp).  Can the Initialize call can be moved to after the
newp call and success check?

Then mXtLoopCreated won't be needed.

>+        
>+        if (!mWindowless) {
>+            rv = mPluginIface->getvalue(GetNPP(), NPPVpluginNeedsXEmbed,
>+                                       (void *)&mXEmbed);
>+            if (NPERR_NO_ERROR != rv) {
>+                mXEmbed = FALSE;
>+            }
>+        }    
>+    }

The type of the data that the plugin will set here is a bit odd.
See the comment in AnswerNPP_GetValue_NPPVpluginNeedsXEmbed.
This needs to share code with that somehow.

>+    // Set up Xt loop for windowed plugins 
>+    // without XEmbed support
>+    mWsInfo.display = DisplayGet();
>+
>+    NS_ASSERTION(!mXtLoopCreated, "mXtLoopCreated should be NULL!");
>+    xt_client_xloop_create();
>+    mXtLoopCreated = TRUE;

The comment indicates the right thing to do here, but
xt_client_xloop_destroy() is being called even for windowless and XEmbed
plugins.

>@@ -332,16 +341,23 @@ PluginInstanceChild::NPN_GetValue(NPNVar

>+    case NPNVxDisplay:
>+        *(void **)aValue = mWsInfo.display;
>+        return NPERR_NO_ERROR;

With Initialized() not called until after newp(), this will need to call
DisplayGet(), at least if mWsInfo.display is not yet set.

>+    case NPNVxtAppContext:
>+        return NPERR_GENERIC_ERROR;
>+    

This shouldn't be reached so no need for this case.

>+Display *
>+PluginInstanceChild::DisplayGet()
>+{
>+    if (mXEmbed) {
>+        return DefaultXDisplay();
>+    }
>+    else {
>+        return xt_client_get_display();
>+    }    
>+}

Gecko convention would call this GetDisplay().

Again with Initialize() after newp(), mXEmbed may not yet be set.
I think that will require querying the plugin instance with
mPluginIface->getvalue.
Again share code with AnswerNPP_GetValue_NPPVpluginNeedsXEmbed.

>+      XtSetArg(args[0], XtNheight, height);
>+      XtSetArg(args[1], XtNwidth,  width);
>+      if (mTookitData.top_widget)
>+          XtSetValues(mTookitData.top_widget, args, 2);
>+      if (mTookitData.child_widget)
>+          XtSetValues(mTookitData.child_widget, args, 2);      

Are these calls necessary?
I don't see the child_widget call in the in-process implementation.
Embedders usually resize their children, so is the top_widget call even
unnecessary?

>+    if (!mWindow.window) {
>+        DeleteWindow();
>+    }   

We shouldn't need to bother with this case.  (It shouldn't happen.)
nsPluginNativeWindowGtk2 doesn't.

>@@ -207,16 +213,19 @@ PluginInstanceChild::~PluginInstanceChil

>+#if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)    
>+    DeleteWindow();
>+#endif

AnswerNPP_Destroy would be a more natural place to have this.

> #if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)
>+public:
>     NPSetWindowCallbackStruct mWsInfo;
>+    bool mWindowless;
>+    bool mXEmbed;
>+    bool mWindowCreated;
>+    XtClient mTookitData;
>+    bool mXtLoopCreated;
> #elif defined(OS_WIN)
>     HWND mPluginWindowHWND;
>     WNDPROC mPluginWndProc;

The "public:" keyword here doesn't look right.  At least the following
cross-platform variables should not be public only on MOZ_X11.
Preferably, no variables should be public.

mWindow.type can be used instead of adding the mWindowless variable.

I expect mTookitData or mWindow.window can be initialized and used in such a
way that mWindowCreated is unnecessary.

mTookitData is Xt-specific, so it should be named to indicate that.
(It is not general data for the PluginInstanceChild.)
I would just call it mXtClient.

>+#include <X11/X.h>
>+#include <X11/Xlib.h>
>+#include <X11/Xutil.h>
>+#include <X11/Intrinsic.h>
>+#include <X11/Shell.h>
>+#include <X11/StringDefs.h>

I expect that all or most of these don't need to be in the header file.
Are they all for the XtSetValues calls?

>+        case NPNVxDisplay: {
>+          if (aNPP) {
>+            *(void **)aValue = InstCast(aNPP)->DisplayGet();
>+          } 
>+          else {
>+            *(void **)aValue = xt_client_get_display();
>+          }          
>+          return NPERR_NO_ERROR;
>+        }

Rather than making DisplayGet() public for this single call, I think it would
be tidier to break from the switch block when aNPP is set and move the
InstCast(aNPP)->NPN_GetValue() call to after the switch block.

>                 if(GTK_IS_SOCKET(widget)) {
>+                    if (!g_object_get_data(G_OBJECT(widget), "enable-xt-focus")) {
>                     nswindow->SetPluginType(nsWindow::PluginType_XEMBED);
>                     break;
>                 }
>             }
>+            }

Indentation needs fixing up here.

Comment 66

[Martin Stránský [:stransky] (ni? me)]

Thanks, I'll update the patch with your comments.

Comment 67

[Martin Stránský [:stransky] (ni? me)]

Attachment: export XtClient methods for OOP plugin support, v.2

The XtClient patch, removed exports

Comment 68

[Martin Stránský [:stransky] (ni? me)]

Attachment: UseAsyncPainting() -> IsOOP()

The UseAsyncPainting() -> IsOOP() API change. Josh, can you check it please?

Comment 69

[Martin Stránský [:stransky] (ni? me)]

Attachment: support Xt plugins OOP, v.2

Here we come, it should address all your comments.

Comment 71

[Karl Tomlinson (:karlt)]

Comment on attachment 620235 [details] [diff] [review]
support Xt plugins OOP, v.2

>diff --git a/dom/plugins/base/nsPluginNativeWindowGtk2.cpp b/dom/plugins/base/nsPluginNativeWindowGtk2.cpp

>+#include "nsNPAPIPluginInstance.h"

This file already uses methods from this interface, so I assume this include
is not necessary here.

>+    case NPNVxDisplay:
>+        if (!mWsInfo.display) {
>+            // We are called before Initialize() so we have to call it now.
>+           Initialize();
>+           NS_ASSERTION(mWsInfo.display, "We should have a valid display!");
>+        }        

There is a small change in behavior here from the in-process implementation.
If the plugin requests the NPNVxDisplay during newp before is has set
NPPVpluginWindowBool, then Initialize will be called before we know it is
windowless and mWsInfo.display will get set to the wrong display.

>+#if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)
>+Display *
>+PluginInstanceChild::GetDisplay()
>+{
>+    if (mXEmbed) {
>+        return DefaultXDisplay();
>+    }
>+    else {
>+        return xt_client_get_display();
>+    }    
>+}

If this is only called from CreateWindow, then this can be moved to there.

(If you end up keeping this function for other purposes, then it should return
the default display for windowless plugins, and document that GetDisplay must
only be used after newp, if you don't want to make it safe to call before
then.)

>+  if (!mWindow.window)
>+      return;
>+
>+  if (!mXEmbed) {
>+      if (mXtClient.top_widget) {     
>+          xt_client_unrealize(&mXtClient);
>+          xt_client_destroy(&mXtClient); 
>+          mXtClient.top_widget = None;
>+      }
>+  }

I guess mWindow.window is NULL for windowless plugins where
mXtClient.top_widget would not be initialized, but I think it would be
clearest to initialize top_widget in the constructor.  Then the !mXEmbed check
can be removed.

top_widget is a pointer and so should be set to NULL or nsnull.

++  // We don't have to keep a reference to plug-in window any more

Perhaps "We don't have to keep the plug-in window ID any longer."
to avoid any suggestion of reference counting.

>-    mWindow.window = reinterpret_cast<void*>(aWindow.window);
>-    mWindow.x = aWindow.x;
>-    mWindow.y = aWindow.y;
>-    mWindow.width = aWindow.width;
>-    mWindow.height = aWindow.height;
>-    mWindow.clipRect = aWindow.clipRect;
>-    mWindow.type = aWindow.type;

>+    // Don't call for windowed plugins when we don't have valid window
>+    if (aWindow.window || mWindow.type != NPWindowTypeWindow) {
>+        if (!mWindow.window) {
>+            CreateWindow(aWindow);
>+        }  
>+        else {
>+            UpdateWindow(aWindow);
>+        }
>+    }      

The XtClient should not be created for windowless plugins and so its probably
best not to call CreateWindow for windowless plugins.

I don't expect this to be called with an empty aWindow.window -
nsPluginNativeWindowGtk2 was not designed to handle that so that check is
probably redundant.

I preferred the mWindow updates before the GTK bug workarounds and the
PLUGIN_LOG_DEBUG.  With these changes moving the mWindow updates, the
PLUGIN_LOG_DEBUG is showing old data.

The MOZ_WIDGET_GTK2 block for old GTK bug workarounds should probably be
skipped for Xt windowed instances.  By adding the test to the existing gtk_check_version "if" statement, the indentation will not need to change.

UpdateWindow now merely calls WindowDataSync, so there is no need to keep
UpdateWindow.  I prefer the mWindow updates inline in SetWindow rather than
having the separate WindowDataSync, partly because WindowDataSync is not
syncing all data and it is tricky to keep track of which bits are updated.

>+        // Already initilized?

Remove the '?' here, as there is no doubt that is has been already
initialized if display is set.  "initialized" typo.

>+    // Request for windowless plugins is set in newp(), before this call.
>+    if (mWindow.type == NPWindowTypeWindow) {
>+        AnswerNPP_GetValue_NPPVpluginNeedsXEmbed(&mXEmbed, &rv);
>+
>+        // Set up Xt loop for windowed plugins 
>+        // without XEmbed support
>+        if (mWindow.type == NPWindowTypeWindow && !mXEmbed) {
>+           mWsInfo.display = GetDisplay();

No need to check mWindow.type a second time.
(I guess the plugin could request windowless during the browser's NeedsXEmbed
query, but that seems unlikely.)

mWsInfo.display should be set even for mXEmbed and for windowless.

> #endif
>-
>     class FlashThrottleAsyncMsg : public ChildAsyncCall

Remove this whitespace change.

>             *(NPBool*)aValue = value ? true : false;
>             return result;
>         }
>-
>+#if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)
>+        case NPNVxDisplay: {
>+          if (aNPP) {
>+              return InstCast(aNPP)->NPN_GetValue(aVariable, aValue);
>+          } 
>+          else {
>+              *(void **)aValue = xt_client_get_display();
>+          }          
>+          return NPERR_NO_ERROR;
>+        }
>+        case NPNVxtAppContext:
>+          return NPERR_GENERIC_ERROR;
>+#endif
>         default: {
>             if (aNPP) {
>                 return InstCast(aNPP)->NPN_GetValue(aVariable, aValue);

Something is inconsistent about the indentation here.

Comment 72

[Martin Stránský [:stransky] (ni? me)]

Attachment: UseAsyncPainting() -> IsOOP(), for check-in

patch for check-in

Comment 73

[Martin Stránský [:stransky] (ni? me)]

Attachment: export XtClient methods for OOP plugin support, v.2 for check-in

a patch for check-in

Comment 74

[Martin Stránský [:stransky] (ni? me)]

Attachment: support Xt plugins OOP, v.3

What about this one? if NPPVpluginWindowBool is called after NPNVxDisplay, the internal display is updated accordingly. 

All other remarks should be addressed.

Comment 75

[Karl Tomlinson (:karlt)]

Comment on attachment 637856 [details] [diff] [review]
support Xt plugins OOP, v.3

>+    , mXEmbed(FALSE)

Use C++ "false" for C++ "bool" types, as here.

>+        NPWindowType newWindowType = windowed ? NPWindowTypeWindow : NPWindowTypeDrawable;
>+        if (mWindow.type != newWindowType && mWsInfo.display) {
>+           // plugin type has been changed but we already have a valid display
>+           // so update it for the recent plugin mode
>+           if (mXEmbed || mWindow.type != NPWindowTypeWindow) {

I think you want to test the newWindowType on the last line here, and
using |windowed| instead of |type != NPWindowTypeWindow| makes that a little
simpler.

>+        Window browserSocket = (Window)reinterpret_cast<void*>(aWindow.window); 

aWindow is a uint64_t (I don't know why it has 64-bits) so the
reinterpret_cast<void*> is unnecessary.

r+ with those changes.

Comment 76

[Martin Stránský [:stransky] (ni? me)]

Attachment: support Xt plugins OOP, v.3 for check-in

Thanks for the review, there's a patch for check-in here.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de697e323b39
https://hg.mozilla.org/integration/mozilla-inbound/rev/b778d551c9e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9414cba94d

Comment 78

[Ryan VanderMeulen [:RyanVM]]

http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

Of course, compiling is a nice feature for any patches to have. Backed out due to widespread build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/23c4021f728c

Logs:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=0091488ce2f1

Comment 79

[Martin Stránský [:stransky] (ni? me)]

Yes, I need to create the hg account for that. 

The windows failures are just missing #ifdefs but the qt build is the more complicated. Karl, are the Xt plugins supported on Qt? Because the gtk2bin is built for GTK only (by default, in the original code), but the original condition:

#if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)

applies to Qt too. The mWindow.window is set on Qt but is it even working?

Comment 80

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #79)
> Yes, I need to create the hg account for that. 

You need only level 1 access and I can vouch for that.
http://www.mozilla.org/hacking/commit-access-policy/

Or there is now https://wiki.mozilla.org/ReleaseEngineering:Autoland

> Karl, are the Xt plugins supported on Qt? Because the gtk2bin
> is built for GTK only (by default, in the original code), but the original
> condition:
> 
> #if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)
> 
> applies to Qt too. The mWindow.window is set on Qt but is it even working?

There is some level of plugin support with Qt (it may require special plugins - I don't know), but Xt plugins were only supported with GTK2.

http://hg.mozilla.org/mozilla-central/annotate/ba8463beab13/dom/plugins/base/nsNPAPIPlugin.cpp#l1971

Comment 81

[Martin Stránský [:stransky] (ni? me)]

Attachment: UseAsyncPainting() -> IsOOP(), for check-in, v2

Updated for MOZ_OVERRIDE change.

Comment 82

[Martin Stránský [:stransky] (ni? me)]

Attachment: support Xt plugins OOP, v.4

Comment 83

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 647098 [details] [diff] [review]
support Xt plugins OOP, v.4

Fixed patch for MAC OS/Windows/Android. 

There were missing #ifdefs for Windows. Plus I used MOZ_WIDGET_GTK for OOP Gtk Xt plugins and "#if defined(MOZ_X11) && defined(XP_UNIX) && !defined(XP_MACOSX)" is used for all unix systems (Qt, MacOS).

Comment 84

[Karl Tomlinson (:karlt)]

Comment on attachment 647098 [details] [diff] [review]
support Xt plugins OOP, v.4

>     mWsInfo.display = DefaultXDisplay();
>+#if defined(MOZ_WIDGET_GTK)
>+    mWsInfo.display = NULL;
>+    mXtClient.top_widget = NULL;
>+#endif

Put the non-GTK DefaultXDisplay() in an #else block, or move it to
Initialize() and move display = NULL out of this block.

r+ with that change.

Comment 85

[Karl Tomlinson (:karlt)]

Bug 626472 means these patches now need s/\bnsnull\b/nullptr/g to apply/land.

Comment 86

[Martin Stránský [:stransky] (ni? me)]

Attachment: support Xt plugins OOP, v.4, for check-in

Comment 87

[Ryan VanderMeulen [:RyanVM]]

Green on Try:
https://tbpl.mozilla.org/?tree=Try&rev=c8eb0ae123a2
https://tbpl.mozilla.org/?tree=Try&rev=9f5ffc81ed94

https://hg.mozilla.org/integration/mozilla-inbound/rev/a624042b95ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/099b35c03672
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a29b0cb50cc

Comment 88

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a624042b95ef
https://hg.mozilla.org/mozilla-central/rev/099b35c03672
https://hg.mozilla.org/mozilla-central/rev/1a29b0cb50cc

Comment 89

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

This broke clang (and therefore asan) builds:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-32ea5314fa2d/try-linux64-debug/try-linux64-debug-bm14-try1-build10950.txt.gz

/builds/slave/try-lnx64-dbg/build/widget/gtkxtbin/gtk2xtbin.c:480:8: error: void function 'xt_client_xloop_create' should not return a value [-Wreturn-type]
       return NULL;