555274 - [OOPP] allow i386 Mac OS X test plugin to negotiate Cocoa event model when run out-of-process

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

We should allow the i386 Mac OS X test plugin to negotiate the Cocoa drawing model when run out-of-process. Otherwise we won't be able to use a standard test plugin out-of-process. This will allow us to continue testing the Carbon event model by default when the test plugin is run in-process.

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

[Josh Aas]

Attachment: fix v1.1

Comment 3

[Josh Aas]

Attachment: fix v1.2

Fix dumb typo, now this works.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Josh, one issue with this patch occurs to me: why should we resort to choosing the model in nptest based on OOP/IP if we don't expect/want plugins to do the same?  My intuition is that we should have a preferred model (one that works OOP ;) ), and if we want nptest to negotiate an older model to test that code path, then set an environment variable saying "MOZ_TEST_MODEL=FOO" or some other hack.

Does that seem reasonable?

Comment 5

[Josh Aas]

I went with this choice because it allows us to keep the event model logic in the test plugin. Plugins will be able to figure out if they are in process or out of process if they want to anyway.

If we do end up with an OOPP environment variable we should add a comment where it is checked so people don't see it as sample code.

I'm torn on whether to stick with my fix or change to making Cocoa default with a Carbon override env variable.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #5)
> I went with this choice because it allows us to keep the event model logic in
> the test plugin.

I don't understand what you mean by this.

> If we do end up with an OOPP environment variable we should add a comment where
> it is checked so people don't see it as sample code.
> 

I don't see a need for an OOPP environment variable in any case.

Comment 7

[Josh Aas]

(In reply to comment #6)
> (In reply to comment #5)
> > I went with this choice because it allows us to keep the event model logic in
> > the test plugin.
> 
> I don't understand what you mean by this.

We're making a decision based on whether the test plugin is OOP or not. If we simply expose that data point we can keep the logic for choosing Cocoa or Carbon in the test plugin itself. The alternative is to specify a testing event model in XPCOM init. That is a logically strange thing to do.

Comment 8

[Josh Aas]

Sorry, I confused this patch with something else I was working on. We're putting the logic in "dom/plugins/PluginModuleChild.cpp", which is less strange than XPCOM init.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Wait, just to be clear, did we agree on IRC to the following proposal?

(In reply to comment #5)
> change to making Cocoa default with
> a Carbon override env variable.

Our test harness or tests themselves would set the Carbon override when we want to test that, right?

Comment 10

[Josh Aas]

Attachment: fix v1.3

Comment 11

[Josh Aas]

I'm aware that my patch includes the following change for clarity:

-OOPPluginsEnabled(const char* aFilePath, const nsPluginTag *aPluginTag)
+RunPluginOOP(const char* aFilePath, const nsPluginTag *aPluginTag)

Comment 12

[Josh Aas]

Attachment: fix v1.4

Get rid of an unnecessary negation.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 438620 [details] [diff] [review]
fix v1.4

>diff --git a/modules/plugin/base/src/nsPluginHost.cpp b/modules/plugin/base/src/nsPluginHost.cpp
>--- a/modules/plugin/base/src/nsPluginHost.cpp
>+++ b/modules/plugin/base/src/nsPluginHost.cpp
>@@ -1548,16 +1548,20 @@ nsPluginHost::nsPluginHost()
> 
>   PLUGIN_LOG(PLUGIN_LOG_ALWAYS,("nsPluginHost::ctor\n"));
>   PR_LogFlush();
> #endif
> 
> #ifdef MAC_CARBON_PLUGINS
>   mVisiblePluginTimer = do_CreateInstance("@mozilla.org/timer;1");
>   mHiddenPluginTimer = do_CreateInstance("@mozilla.org/timer;1");
>+
>+  // If Carbon is an available drawing model we want to test it
>+  // instead of Cocoa. Cocoa is the default for the test plugin.
>+  PR_SetEnv("TEST_CARBON_NPAPI=1");

I think it'd be better to set this in a test harness, because we'd
also want to test Cocoa on 10.5, right? (And perhaps test different
instances of the same module with different event models.).  But TBH I
don't know in which harness or where this would be set.  Maybe file a
QA followup?

>diff --git a/modules/plugin/test/testplugin/nptest_macosx.mm b/modules/plugin/test/testplugin/nptest_macosx.mm
>--- a/modules/plugin/test/testplugin/nptest_macosx.mm
>+++ b/modules/plugin/test/testplugin/nptest_macosx.mm
>@@ -62,24 +59,31 @@ pluginInstanceInit(InstanceData* instanc
>   if ((NPN_GetValue(npp, NPNVsupportsCoreGraphicsBool, &supportsCoreGraphics) == NPERR_NO_ERROR) &&
>       supportsCoreGraphics) {
>     NPN_SetValue(npp, NPPVpluginDrawingModel, (void*)NPDrawingModelCoreGraphics);
>   } else {
>     printf("CoreGraphics drawing model not supported, can't create a plugin instance.\n");
>     return NPERR_INCOMPATIBLE_VERSION_ERROR;
>   }
> 
>-#ifdef USE_COCOA_NPAPI
>-  NPBool supportsCocoaEvents = false;
>-  if ((NPN_GetValue(npp, NPNVsupportsCocoaBool, &supportsCocoaEvents) == NPERR_NO_ERROR) &&
>-      supportsCocoaEvents) {
>-    NPN_SetValue(npp, NPPVpluginEventModel, (void*)NPEventModelCocoa);
>+#ifndef NP_NO_CARBON
>+  if (getenv("TEST_CARBON_NPAPI")) {
>+    sCurrentEventModel = NPEventModelCarbon;
>   } else {
>-    printf("Cocoa event model not supported, can't create a plugin instance.\n");
>-    return NPERR_INCOMPATIBLE_VERSION_ERROR;

It appears to me from a quick scan of npapi.h that NP_NO_CARBON
doesn't imply no Cocoa.  Is that right?  If so, I think this check
would prevent us from testing Cocoa on 10.5.

>+#endif
>+    NPBool supportsCocoaEvents = false;
>+    if ((NPN_GetValue(npp, NPNVsupportsCocoaBool, &supportsCocoaEvents) == NPERR_NO_ERROR) &&
>+        supportsCocoaEvents) {
>+      NPN_SetValue(npp, NPPVpluginEventModel, (void*)NPEventModelCocoa);
>+      sCurrentEventModel = NPEventModelCocoa;
>+    } else {
>+      printf("Cocoa event model not supported, can't create a plugin instance.\n");
>+      return NPERR_INCOMPATIBLE_VERSION_ERROR;
>+    }
>+#ifndef NP_NO_CARBON
>   }
> #endif
> 

So I think the logic would be

#ifndef NP_NO_CARBON
  if (getenv("TEST_CARBON_NPAPI"))
    sCurrentEventModel = NPEventModelCarbon;
#endif

  if (sCurrentEventModel == NPEventModelCocoa) {
    NPBool supportsCocoaEvents = false;
    [...]
  }

> int16_t
> pluginHandleEvent(InstanceData* instanceData, void* event)
> {
>-#ifdef USE_COCOA_NPAPI
>+#ifndef NP_NO_CARBON
>+  if (sCurrentEventModel == NPEventModelCarbon) {
>+    EventRecord* carbonEvent = (EventRecord*)event;
>+    if (!carbonEvent)
>+      return 1;
>+    
>+    NPWindow* w = &instanceData->window;
>+    switch (carbonEvent->what) {
>+      case updateEvt:
>+        pluginDraw(instanceData, NULL);
>+        return 1;
>+      case mouseDown:
>+      case mouseUp:
>+      case osEvt:
>+      {
>+        Rect globalBounds = {0};
>+        WindowRef nativeWindow = static_cast<WindowRef>(static_cast<NP_CGContext*>(w->window)->window);
>+        if (nativeWindow)
>+          ::GetWindowBounds(nativeWindow, kWindowStructureRgn, &globalBounds);
>+        instanceData->lastMouseX = carbonEvent->where.h - w->x - globalBounds.left;
>+        instanceData->lastMouseY = carbonEvent->where.v - w->y - globalBounds.top;
>+        return 1;
>+      }
>+      default:
>+        return 1;
>+    }
>+    return 1;
>+  }
>+#endif
>+
>   NPCocoaEvent* cocoaEvent = (NPCocoaEvent*)event;
>   if (!cocoaEvent)
>-    return 0;
>+    return 1;
> 
>   switch (cocoaEvent->type) {
>     case NPCocoaEventDrawRect:
>       pluginDraw(instanceData, cocoaEvent);
>       return 1;
>     case NPCocoaEventMouseDown:
>     case NPCocoaEventMouseUp:
>     case NPCocoaEventMouseMoved:
>       instanceData->lastMouseX = (int32_t)cocoaEvent->data.mouse.pluginX;
>       instanceData->lastMouseY = (int32_t)cocoaEvent->data.mouse.pluginY;
>       return 1;
>     default:
>-      return 0;
>+      return 1;
>   }
>-#else
>-  EventRecord* carbonEvent = (EventRecord*)event;
>-  if (!carbonEvent)
>-    return 0;
> 
>-  NPWindow* w = &instanceData->window;
>-  switch (carbonEvent->what) {
>-  case updateEvt:
>-    pluginDraw(instanceData);
>-    return 1;
>-  case mouseDown:
>-  case mouseUp:
>-  case osEvt:
>-    {
>-    Rect globalBounds = {0};
>-    WindowRef nativeWindow = static_cast<WindowRef>(static_cast<NP_CGContext*>(w->window)->window);
>-    if (nativeWindow)
>-      ::GetWindowBounds(nativeWindow, kWindowStructureRgn, &globalBounds);
>-    instanceData->lastMouseX = carbonEvent->where.h - w->x - globalBounds.left;
>-    instanceData->lastMouseY = carbonEvent->where.v - w->y - globalBounds.top;
>-    return 1;
>-    }
>-  default:
>-    return 0;
>-  }
>-#endif
>+  return 1;
> }
>

Why the s/return 0/return 1/ here?

Comment 14

[Josh Aas]

(In reply to comment #13)
> (From update of attachment 438620 [details] [diff] [review])

> I think it'd be better to set this in a test harness, because we'd
> also want to test Cocoa on 10.5, right? (And perhaps test different
> instances of the same module with different event models.).  But TBH I
> don't know in which harness or where this would be set.  Maybe file a
> QA followup?

The 32-bit test plugin runs in-process by default on 10.5 and 10.6. When it is in-process, I want it to run as a Carbon plugin. If someone configures the browser to run it out-of-process, I want it to switch to Cocoa. Our 10.6 tinderboxes produce a 64-bit plugin, which runs Cocoa all the time, so this doesn't really have anything to do with 10.5 vs. 10.6 when it comes to the 32-bit plugin.

I don't know of a place to put this in the test harness either.

> It appears to me from a quick scan of npapi.h that NP_NO_CARBON
> doesn't imply no Cocoa.  Is that right?  If so, I think this check
> would prevent us from testing Cocoa on 10.5.

I don't know what this has to do with 10.5 or Cocoa. NP_NO_CARBON just means what it says - if it is true, then Carbon is not available on the architecture. It is true for 64-bit, false for 32-bit. Both Cocoa and Carbon are available on 10.5 and 10.6 in 32-bit Gecko builds. The check here just disallows the possibility of the TEST_CARBON_NPAPI variable being used when Carbon is not available (64-bit builds).

> So I think the logic would be
> 
> #ifndef NP_NO_CARBON
>   if (getenv("TEST_CARBON_NPAPI"))
>     sCurrentEventModel = NPEventModelCarbon;
> #endif
> 
>   if (sCurrentEventModel == NPEventModelCocoa) {
>     NPBool supportsCocoaEvents = false;
>     [...]
>   }

This logic is the same as what I wrote but it does have fewer macros. I'll switch to it.

> Why the s/return 0/return 1/ here?

Normally return values don't matter here - NPAPI ignored them entirely until recently. Now there is one single use for them - if you return 0 for a Cocoa NPAPI key down event it'll start out-of-line IME. This was slipped into Cocoa NPAPI by WebKit and is a strange quirk, but it makes 1 the more normal return value.

Comment 15

[Benjamin Smedberg]

The 32-bit test plugin runs in-process by default on 10.5 and 10.6. When it is
in-process, I want it to run as a Carbon plugin. If someone configures the

Don't we want/need testing of both Carbon and Cocoa in-process? What cjones is saying, I think, is that it doesn't make sense for the OOPP/IPP switch to be the same as the Carbon/Cocoa switch. If we need to build a second version of the testplugin (application/x-testplugin-cocoa) we should do that, in order to get the right testing matrix. Otherwise we should use environment variables or some other mechanism so that we can test both systems IPP, and only cocoa OOPP.

Comment 16

[Josh Aas]

I think maybe you guys are talking about a bigger problem than what I'm trying to solve here. This is not a bug about allowing for testing multiple models per architecture. This is about allowing different models to be negotiated at runtime so that what would normally be a Carbon plugin is capable of running out-of-process as a Cocoa plugin, for manual testing. See the bug summary.

Right now we can only test one event model per architecture, and within that constraint we are testing Carbon on i386 and Cocoa on x86_64. At the moment, Carbon is hard-coded at compile time on 32-bit, which is fine for running our automated tests, but it means that the 32-bit test plugin can't run out-of-process (it can't make that runtime switch).

I do agree that for maximum flexibility it would be nice to set the force-carbon env variable in the test harness.

Comment 17

[Josh Aas]

Attachment: fix v1.5

We sorted out the confusion on irc and I found a new approach that maximizes flexibility and doesn't require us to set env variables from gecko code.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 439054 [details] [diff] [review]
fix v1.5

:D

Comment 19

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/490fba6cd22a

Comment 20

[Josh Aas]

64-bit bustage fix:

http://hg.mozilla.org/mozilla-central/rev/901155284571