[webkit-reviews] review granted: [Bug 42084] Add a PluginController class, use it for invalidation and getting the user agent : [Attachment 61236] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 42084: Add a PluginController class, use it for invalidation and getting
the user agent
https://bugs.webkit.org/show_bug.cgi?id=42084

Attachment 61236: Patch
https://bugs.webkit.org/attachment.cgi?id=61236&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
>  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