Negotiate Mac event and drawing model support

Negotiate Mac event and drawing model support

This will help performance on our main thread, and will future proof us.

BUG=NONE
TEST=BUILD

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86478

[dmac, 2011-05-24 20:15:53]

PTAL

[Lambros (do not use), 2011-05-24 20:20:19]

Just a drive-by question, what is "Bozo" here?  Anything to do with this:
http://en.wikipedia.org/wiki/Bozo_bit ?
:)
Lambros


On Tue, May 24, 2011 at 1:15 PM, <dmaclach@chromium.org> wrote:

> Reviewers: awong, Wez,
>
> Message:
> PTAL
>
> Description:
> Negotiate Mac event and drawing model support
>
> This will help performance on our main thread, and will future proof us.
>
> BUG=NONE
> TEST=BUILD
>
>
> Please review this at http://codereview.chromium.org/6992033/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M remoting/host/host_plugin.cc
>
>
> Index: remoting/host/host_plugin.cc
> diff --git a/remoting/host/host_plugin.cc b/remoting/host/host_plugin.cc
> index
>
371d91f7e7e71786dfa0946e9508679a386a1435..894d1423b34e9f8c34d105ae6295ecd6fb190072
> 100644
> --- a/remoting/host/host_plugin.cc
> +++ b/remoting/host/host_plugin.cc
> @@ -595,6 +595,44 @@ class HostNPPlugin {
>   }
>
>   bool Init(int16 argc, char** argn, char** argv, NPSavedData* saved) {
> +#if defined(OS_MACOSX)
> +    // Use the modern CoreGraphics and Cocoa models when available, since
> +    // QuickDraw and Carbon are deprecated.
> +    // The drawing and event models don't change anything for this plugin,
> since
> +    // none of the functions affected by the models actually do anything.
> +    // This does however keep the plugin from breaking when Chromium
> eventually
> +    // drops support for QuickDraw and Carbon, and it also keeps the
> browser
> +    // from sending Null Events once a second to support old Carbon based
> +    // timers.
> +    // Chromium should always be supporting the newer models.
> +
> +    // Bozo check to see if Chromium supports the CoreGraphics drawing
> model.
> +    NPBool supports_core_graphics = false;
> +    NPError err = g_npnetscape_funcs->getvalue(instance_,
> +
> NPNVsupportsCoreGraphicsBool,
> +                                               &supports_core_graphics);
> +    if (err == NPERR_NO_ERROR && supports_core_graphics) {
> +      // Switch to CoreGraphics drawing model.
> +      g_npnetscape_funcs->setvalue(instance_, NPPVpluginDrawingModel,
> +          reinterpret_cast<void*>(NPDrawingModelCoreGraphics));
> +    } else {
> +      LOG(ERROR) << "No Core Graphics support";
> +      return false;
> +    }
> +
> +    // Bozo check to see if Chromium supports the Cocoa event model.
> +    NPBool supports_cocoa = false;
> +    err = g_npnetscape_funcs->getvalue(instance_, NPNVsupportsCocoaBool,
> +                                      &supports_cocoa);
> +    if (err == NPERR_NO_ERROR && supports_cocoa) {
> +      // Switch to Cocoa event model.
> +      g_npnetscape_funcs->setvalue(instance_, NPPVpluginEventModel,
> +          reinterpret_cast<void*>(NPEventModelCocoa));
> +    } else {
> +      LOG(ERROR) << "No Cocoa Event Model support";
> +      return false;
> +    }
> +#endif  // OS_MACOSX
>     return true;
>   }
>
>
>
>

[awong, 2011-05-24 20:22:36]

LGTM

bozos included :)

http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc
File remoting/host/host_plugin.cc (right):

http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:626: &supports_cocoa);
nit: needs one more space.

[Wez, 2011-05-25 03:21:22]

Sorry, was OOO this afternoon & only just got to this.

http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc
File remoting/host/host_plugin.cc (right):

http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc#ne...
remoting/host/host_plugin.cc:614: if (err == NPERR_NO_ERROR &&
supports_core_graphics) {
No need to do separate check for support prior to setting support; setvalue()
should return != NPERR_NO_ERROR if the option isn't supported.

[Wez, 2011-05-25 03:23:25]

On 2011/05/25 03:21:22, Wez wrote:
> Sorry, was OOO this afternoon & only just got to this.
> 
> http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc
> File remoting/host/host_plugin.cc (right):
> 
>
http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc#ne...
> remoting/host/host_plugin.cc:614: if (err == NPERR_NO_ERROR &&
> supports_core_graphics) {
> No need to do separate check for support prior to setting support; setvalue()
> should return != NPERR_NO_ERROR if the option isn't supported.

Also, does it actually make sense to mix old/new drawing/event models, in the
case where the browser claimed to support one but not the other (I realise it
won't in practice, but...)?

[dmaclach1, 2011-05-25 15:03:54]

On Tue, May 24, 2011 at 20:21,  <wez@chromium.org> wrote:
> Sorry, was OOO this afternoon & only just got to this.
>
>
> http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc
> File remoting/host/host_plugin.cc (right):
>
>
http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc#ne...
> remoting/host/host_plugin.cc:614: if (err == NPERR_NO_ERROR &&
> supports_core_graphics) {
> No need to do separate check for support prior to setting support;
> setvalue() should return != NPERR_NO_ERROR if the option isn't
> supported.

Out of curiousity, where did you see that specified? I couldn't find a
good spec on it, so I decided to go with safe.

[dmaclach1, 2011-05-25 15:05:19]

On Tue, May 24, 2011 at 20:23,  <wez@chromium.org> wrote:
> On 2011/05/25 03:21:22, Wez wrote:
>>
>> Sorry, was OOO this afternoon & only just got to this.
>
>> http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc
>> File remoting/host/host_plugin.cc (right):
>
>
>
http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc#ne...
>>
>> remoting/host/host_plugin.cc:614: if (err == NPERR_NO_ERROR &&
>> supports_core_graphics) {
>> No need to do separate check for support prior to setting support;
>> setvalue()
>> should return != NPERR_NO_ERROR if the option isn't supported.
>
> Also, does it actually make sense to mix old/new drawing/event models, in
> the
> case where the browser claimed to support one but not the other (I realise
> it
> won't in practice, but...)?

I'd rather just not load on an untested configuration like that.

[Wez, 2011-05-25 17:15:40]

On 2011/05/25 15:03:54, dmaclach1 wrote:
> On Tue, May 24, 2011 at 20:21,  <mailto:wez@chromium.org> wrote:
> > Sorry, was OOO this afternoon & only just got to this.
> >
> >
> > http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc
> > File remoting/host/host_plugin.cc (right):
> >
> >
>
http://codereview.chromium.org/6992033/diff/1/remoting/host/host_plugin.cc#ne...
> > remoting/host/host_plugin.cc:614: if (err == NPERR_NO_ERROR &&
> > supports_core_graphics) {
> > No need to do separate check for support prior to setting support;
> > setvalue() should return != NPERR_NO_ERROR if the option isn't
> > supported.
> 
> Out of curiousity, where did you see that specified? I couldn't find a
> good spec on it, so I decided to go with safe.

It's not well-specified anywhere I can find, unfortunately.  I checked a couple
of plugin implementations, including the Firebreath generic plugin framework,
and found that they just called setvalue() and checked the result.

I get the impression that the way the Mozilla example is written is more to show
that you can use getvalue() and setvalue() to negotiate a combination that
works.

And yes, I agree that we shouldn't load if the drawing & event models are
inconsistent.  I just wondered if we should load if they are consistent but old?

[Wez, 2011-05-25 18:20:43]

Forgot to mention; I checked Chromium's NPAPI plugin host-side, which has the
behaviour you'd expect for setvalue().