Bug 42084 – Add a PluginController class, use it for invalidation and getting the user agent

• Component: New Bugs
• Status: RESOLVED FIXED

[Anders Carlsson]

Add a PluginController class, use it for invalidation and getting the user agent

[Anders Carlsson]

Created attachment 61236 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 61236 [details]
Patch

>  void NPN_MemFree(void* ptr)
>  {
> +    // We could use fastFree here, but there might be plug-ins that mix NPN_MemAlloc/NPN_MemFree with malloc and free,
> +    // so having them be equivalent seems like a good idea.
> +    free(ptr);
>      notImplemented();
>  }

This doesn't seem "not implemented" anymore.

> -void NPN_InvalidateRect(NPP instance, NPRect *invalidRect)
> +void NPN_InvalidateRect(NPP npp, NPRect *invalidRect)

Funny * placement here.

> +void NetscapePlugin::invalidate(NPRect* invalidRect)

I think the parameter type could be const NPRect*.

> -bool NetscapePlugin::initialize(const Parameters& parameters)
> +bool NetscapePlugin::initialize(PluginController* pluginController, const Parameters& parameters)
>  {
> +    ASSERT(!m_pluginController);
> +    m_pluginController = pluginController;

Other code seems to assume that m_pluginController is non-null after this point. So maybe you should add an assertion about that.

> +class PluginController {
> +public:
> +    virtual ~PluginController() { }

I think it's better to declare the destructor as protected, if no one is supposed to be allowed to delete a PluginController.

> +String PluginView::userAgent(const KURL& url)
> +{
> +    if (Frame* frame = m_pluginElement->document()->frame())
> +        return frame->loader()->client()->userAgent(url);
> +
> +    return String();
> +}

I think an early return when frame is null would be nicer.

PluginController seems more like a client-type class than a controller-type class. It isn't really controlling a plugin at all; it's just performing some actions on behalf of the plugin. Maybe PluginClient would be a better name?

You should make sure this doesn't break the Windows build. You could add PluginController.h to WebKit2.vcproj.

r=me

[Anders Carlsson]

(In reply to comment #2)
> (From update of attachment 61236 [details])
> >  void NPN_MemFree(void* ptr)
> >  {
> > +    // We could use fastFree here, but there might be plug-ins that mix NPN_MemAlloc/NPN_MemFree with malloc and free,
> > +    // so having them be equivalent seems like a good idea.
> > +    free(ptr);
> >      notImplemented();
> >  }
> 
> This doesn't seem "not implemented" anymore.
> 

Removed.

> > -void NPN_InvalidateRect(NPP instance, NPRect *invalidRect)
> > +void NPN_InvalidateRect(NPP npp, NPRect *invalidRect)
> 
> Funny * placement here.
>

Fixed.
 
> > +void NetscapePlugin::invalidate(NPRect* invalidRect)
> 
> I think the parameter type could be const NPRect*.

Fixed.

> 
> > -bool NetscapePlugin::initialize(const Parameters& parameters)
> > +bool NetscapePlugin::initialize(PluginController* pluginController, const Parameters& parameters)
> >  {
> > +    ASSERT(!m_pluginController);
> > +    m_pluginController = pluginController;
> 
> Other code seems to assume that m_pluginController is non-null after this point. So maybe you should add an assertion about that.
>

Added.
 
> > +class PluginController {
> > +public:
> > +    virtual ~PluginController() { }
> 
> I think it's better to declare the destructor as protected, if no one is supposed to be allowed to delete a PluginController.
>

Fixed.
 
> > +String PluginView::userAgent(const KURL& url)
> > +{
> > +    if (Frame* frame = m_pluginElement->document()->frame())
> > +        return frame->loader()->client()->userAgent(url);
> > +
> > +    return String();
> > +}
> 
> I think an early return when frame is null would be nicer.

Fixed.

> 
> PluginController seems more like a client-type class than a controller-type class. It isn't really controlling a plugin at all; it's just performing some actions on behalf of the plugin. Maybe PluginClient would be a better name?
> 

I don't like PluginClient in this case - how about PluginOwner?

> You should make sure this doesn't break the Windows build. You could add PluginController.h to WebKit2.vcproj.

Will do.

> 
> r=me

Thanks!

[Adam Roben (:aroben)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 61236 [details] [details])
> > PluginController seems more like a client-type class than a controller-type class. It isn't really controlling a plugin at all; it's just performing some actions on behalf of the plugin. Maybe PluginClient would be a better name?
> > 
> 
> I don't like PluginClient in this case - how about PluginOwner?

There's nothing in the PluginController interface that indicates that this object owns a plugin. It's just an object that knows how to invalidate a plugin and return a user-agent string. So PluginOwner also seems a little misleading.

[Anders Carlsson]

Committed r63094: <http://trac.webkit.org/changeset/63094>