Bug 29302 - NPAPI plugin support feature on Webkit for S60 platform
: NPAPI plugin support feature on Webkit for S60 platform
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: S60 Emulator S60 3rd edition
: P2 Normal
Assigned To:
:
: Qt
:
: 27065 29799
  Show dependency treegraph
 
Reported: 2009-09-16 09:24 PST by
Modified: 2009-11-25 01:33 PST (History)


Attachments
Enabling NPAPI plugin support for S60 (38.48 KB, patch)
2009-09-16 09:24 PST, Rohini Ananth
vestbo: review-
Review Patch | Details | Formatted Diff | Diff
Patch with review comments incorporated (38.35 KB, patch)
2009-09-17 08:35 PST, Rohini Ananth
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Patch with review comments incorporated (37.16 KB, patch)
2009-09-23 09:51 PST, Rohini Ananth
no flags Review Patch | Details | Formatted Diff | Diff
Patch with changes incorporated - Compilation issues solved (39.36 KB, patch)
2009-10-01 02:46 PST, Rohini Ananth
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Patch (41.71 KB, patch)
2009-10-11 16:27 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Patch (41.37 KB, patch)
2009-10-12 17:53 PST, Yael
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Patch (39.61 KB, patch)
2009-10-13 14:41 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch after hash changes landed (39.65 KB, patch)
2009-10-14 07:52 PST, Yael
hausmann: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-09-16 09:24:06 PST
Created an attachment (id=39649) [details]
Enabling NPAPI plugin support for S60 

Enabling NPAPI plugin support on Qt Webkit for S60 platform. Added a folder 'symbian' under WebCore/plugins/ for handling platform specific functionality and other required changes to enable NPAPI plugin support.The design uses Qt plugin framework for discovering/loading plugins.
------- Comment #1 From 2009-09-16 09:46:29 PST -------
First of all, some of the code seems sharable across platforms. Have you looked
into that?

+#elif !defined(XP_SYMBIAN) && defined(XP_WIN)

You can have XP_WIN and XP_SYMBIAN enabled as the same time?

+PluginContainerSymbian::PluginContainerSymbian(PluginView* view, QWidget*
parent)
+    : m_parent(parent)
+    , m_pluginView(view)
+    , m_hasPendingGeometryChange(false)
+{
+setParent(m_parent);
+}

^ missing some indent here!

+void PluginContainerSymbian::adjustGeometry()
+{
+    if (m_hasPendingGeometryChange) {
+        setGeometry(m_windowRect);
+        setMask(m_clipRegion);

You dont have the problem we had to setMask with an empty region showing
everything? thus having to hide in that case?


+void PluginContainerSymbian::focusInEvent(QFocusEvent* event)
+{
+    // we got focus, stop redirecting the wheel events
+    //redirectWheelEventsToParent(false);

Dont commit outcommented code like this


+#ifndef PluginContainerQt_H
+#define PluginContainerQt_H

I beleive that should be _h and not _H but might be wrong.

+    PluginContainerSymbian* container =
static_cast<PluginContainerSymbian*>(platformPluginWidget());

Do you really need a special Symbian container? or could we just make a simple
ifdef in the PluginContainerQt?

All the event conversion makes it seem like we need some special convertion
classes. We also need this for windowless linux plugins.


+void PluginView::invalidateRect(NPRect* rect)
+{
+    if (m_isWindowed)
+        return;
+    if (rect) {
+        IntRect r(rect->left, rect->top, rect->right - rect->left,
rect->bottom - rect->top);
+        m_invalidRects.append(r);
+        if (!m_invalidateTimer.isActive())
+            m_invalidateTimer.startOneShot(0.001);
+    }
+}

maybe just do 'if (m_isWindowed || !rect) return' ?

+void PluginView::invalidateRegion(NPRegion region)
+{
+    if (m_isWindowed)
+        return;
+    QRegion* r = static_cast<QRegion*>(region);
+    if (r) {
+        QVector<QRect> rects = r->rects();
+
+        for (int i = 1; i < rects.size(); ++i) {
+            const QRect& qRect = rects.at(i);
+            IntRect r(qRect);
+            m_invalidRects.append(r);
+        if (!m_invalidateTimer.isActive())
+            m_invalidateTimer.startOneShot(0.001);
+        }
+    }
+}

^ here I would also do 'if (!r) return;'
------- Comment #2 From 2009-09-16 09:48:06 PST -------
Yael, does this mean that you will be back working on windowless plugins? :-)
------- Comment #3 From 2009-09-17 08:11:38 PST -------
(From update of attachment 39649 [details])
r- for now

We need to discuss the design some more, and there's style issues such as the ones Kenneth mentioned.
------- Comment #4 From 2009-09-17 08:16:15 PST -------
Discussed with Yael on IRC about the dependency on Qt and agreed that since Qt will be a system library on S60 in the future, the dependency is analogous to the Carbon/Cocoa and CoreGraphics/QuickDraw dependencies of the XP_MACOSX implementation. 

But, we should make this clear and explicit through similar mechanism as the XP_MACOSX implementation, for example being able to query for the toolkit suport etc.
------- Comment #5 From 2009-09-17 08:35:49 PST -------
Created an attachment (id=39695) [details]
Patch with review comments incorporated

Incorporating most of review comments and few others answered here...

Q: You can have XP_WIN and XP_SYMBIAN enabled as the same time?
A: Yes for winscw [debug version of s60], both get defined

Q:Do you really need a special Symbian container? or could we just make a simple ifdef in the PluginContainerQt?
A: yes we would need a container class for handling scroll/zoom [gesture] events later

Q:does this mean that you will be back working on windowless plugins ?
A: Current patch contains support for both windowed & windowless plugin :-)
------- Comment #6 From 2009-09-17 10:46:50 PST -------
> Q:Do you really need a special Symbian container? or could we just make a
> simple ifdef in the PluginContainerQt?
> A: yes we would need a container class for handling scroll/zoom [gesture]
> events later

Why should that be limited to the Symbian port, it seems generally useful. That would make a lot of sense on Maemo for instance.
------- Comment #7 From 2009-09-21 13:39:49 PST -------
(From update of attachment 39695 [details])
> Index: WebCore/bridge/npapi.h
> ===================================================================
> --- WebCore/bridge/npapi.h	(revision 48458)
> +++ WebCore/bridge/npapi.h	(working copy)
> @@ -56,6 +56,10 @@
>  #    endif /* XP_WIN */
>  #endif /* _WIN32 */
>  
> +#ifdef __SYMBIAN32__
> +#    define XP_SYMBIAN
> +#endif /* __SYMBIAN32__ */
> +

This does not appear to be necessary as npapi.h already contains this snippet. See http://trac.webkit.org/changeset/48174

>  #ifdef __MWERKS__
>  #    define _declspec __declspec
>  #    ifdef macintosh
> @@ -106,9 +110,11 @@
>      #include <stdio.h>
>  #endif
>  
> +#if !defined(XP_SYMBIAN)
>  #ifdef XP_WIN
>      #include <windows.h>
>  #endif
> +#endif

This hunk is wrong. XP_WIN should not be defined, and in fact after r48174 it won't be
defined when compiling with WINSCW (or Symbian in general).

The same applies to the other hunks that add !SYMBIAN to #ifdef XP_WIN.

> +        m_npWindow.window = (void*)platformPluginWidget();
> +
> +        if (!(m_plugin->quirks().contains(PluginQuirkDeferFirstSetWindowCall)))
> +            setNPWindowRect(frameRect());
> +    } else {
> +        setPlatformWidget(0);
> +        show();
> +        m_npWindow.type = NPWindowTypeDrawable;
> +        if (!(m_plugin->quirks().contains(PluginQuirkDeferFirstSetWindowCall)))
> +            setNPWindowRect(frameRect());

For an obviously new platform/ABI, do we really need these quirks?

> +
> +} // namespace WebCore
> Index: WebCore/plugins/symbian/npinterface.h
> ===================================================================
> --- WebCore/plugins/symbian/npinterface.h	(revision 0)
> +++ WebCore/plugins/symbian/npinterface.h	(revision 0)
> @@ -0,0 +1,71 @@
> +/*
> +    Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies)
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public
> +    License as published by the Free Software Foundation; either
> +    version 2 of the License, or (at your option) any later version.
> +
> +    This library is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +    Library General Public License for more details.
> +
> +    You should have received a copy of the GNU Library General Public License
> +    along with this library; see the file COPYING.LIB.  If not, write to
> +    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +    Boston, MA 02110-1301, USA.
> +*/
> +#ifndef npinterface_H
> +#define npinterface_H
> +
> +#include "npfunctions.h"
> +#include <QtPlugin>
> +class NPInterface {
> +public:
> +    virtual NPError NP_Initialize(NPNetscapeFuncs *aNPNFuncs, NPPluginFuncs *aNPPFuncs) = 0;
> +    virtual void NP_Shutdown() = 0;
> +    virtual char* NP_GetMIMEDescription() = 0;
> +};
> +
> +enum NppEventType {
> +    NppEventKey,
> +    NppEventMouse,
> +    NppEventDraw,
> +    NppEventVisibility,
> +    NppCustomValue
> +};
> +
> +typedef struct _NPPEvent {
> +    NppEventType type;
> +    void* data;
> +} NPPEvent;
> +
> +typedef struct _NPPEventKey {
> +    void* keyEvent; // QKeyEvent
> +    void* reserved;
> +} NPPEventKey;
> +
> +typedef struct _NPPEventMouse {
> +    void* mouseEvent; // QMouseEvent
> +    void* reserved;
> +} NPPEventMouse;
> +
> +typedef struct _NPPEventVisibility {
    bool visible;
> +    void* reserved;
> +} NPPEventVisibility;
> +
> +typedef struct _NPPEventDraw {
> +    void* painter; // QPainter
> +    void* reserved;
> +} NPPEventDraw;

I admit that this file is the biggest part that I don't like.

I understand that Qt is becoming a system library on Symbian, it's becoming the toolkit that plugins have to use. That's good.

But then it should still be possible to use the existing NPAPI interface for that, no?

The above interface _mimics_ npapi, so why not use it directly?
------- Comment #8 From 2009-09-22 06:28:17 PST -------
I tried to apply the patch (from 17.9.) to latest webkit code (r48572) and ran into the following problems:

- the patch updates the webcore.pro, but I believe that you also need to add the export configuration (see below) to WebCore.pro (in the symbian {} block, line 23ff). This is needed to make API available to client code in symbian:

    BLD_INF_RULES.prj_exports += \
          "plugins/symbian/npinterface.h $$MW_LAYER_DOMAIN_EXPORT_PATH(cwrt/npinterface.h)" \
          "plugins/npfunctions.h  $$MW_LAYER_DOMAIN_EXPORT_PATH(cwrt/npfunctions.h)" \
          "bridge/npapi.h                       $$MW_LAYER_DOMAIN_EXPORT_PATH(cwrt/npapi.h)" \
          "bridge/npruntime.h                   $$MW_LAYER_DOMAIN_EXPORT_PATH(cwrt/npruntime.h)"

- in PluginViewSymbian.cpp you invoke notSupported() function in a couple of cases. That function is not available.

- in PluginViewSymbian.cpp, the platformWindow() function was changed to platformPageClient(), just this weekend

- lastly, I had problems applying the patch, as submitted. I get an error, something like "The chunk size did not match the number of added/removed lines"
------- Comment #9 From 2009-09-23 09:51:41 PST -------
Created an attachment (id=40001) [details]
Patch with review comments incorporated
------- Comment #10 From 2009-09-23 10:02:50 PST -------
Patch addresses comments given by Simon and Nobert.

The reason for defining a new interface "npinterface.h" instead of npapi.h for Symbian is because in symbian,load by function name does not work and ordinal number has to be used, which is not generic. Current architecture suggested in patch uses Qt plugin framework way for discovering and loading of plugins.
------- Comment #11 From 2009-09-23 15:32:59 PST -------
Rohini, many thanks for the updates.

There is one issue with WebCore.pro:
The export macro (BLD_INF_RULES.prj_exports += ...) is currently under symbian && ENABLE_NETSCAPE_PLUGIN_API=1 (way down, at lines 2400ff). That is, the header files would not be exported in case of NPAPI disabled. 

I know that there is client code for the API that doesn't check the condition and would pickup the wrong npapi.h in that case (there is a name conflict and we took care that the exported npapi.h gets included first). Even though with NPAPI disabled, applications would not link, but they would at least give a sensible error message, given that the right npapi.h was used. If the wrong npapi.h was picked, it is almost impossible for an apps developer to trace the problem.

Anyway, in short, to solve this issue, I suggest to move the export macro to the beginning of WebCore.pro, into the symbian block that is not conditional to NPAPI being enabled (line 23ff).
------- Comment #12 From 2009-09-23 18:31:55 PST -------
There are a couple more compilation problems with PluginPackageSymbian.cpp in the patch, for both armv5 and winscw targets:

- The calls to platformPageClient (lines 403 and 437) are invalid:

"\webkit.org_trunk\WebCore\plugins\symbian\pluginviewsymbian.cpp", line 403: Error:  #393: pointer to incomplete class type is not allowed
      QRect qrect = m_parentFrame->view()->hostWindow()->platformPageClient()->rect();
                    ^
"\webkit.org_trunk\WebCore\plugins\symbian\pluginviewsymbian.cpp", line 437: Error:  #289: no instance of constructor "WebCore::PluginContainerSymbian::PluginContainerSymbian" matches the argument list
            argument types are: (WebCore::PluginView *, PlatformPageClient)
          setPlatformWidget(new PluginContainerSymbian(this, m_parentFrame->view()->hostWindow()->platformPageClient()));


- Its compilation causes multiple inclusion errors at the linker:

Error: L6200E: Symbol WebCore::PluginView::userAgentStatic() multiply defined (by QtWebKit.in and QtWebKit.in).
Error: L6200E: Symbol WebCore::PluginView::init() multiply defined (by QtWebKit.in and QtWebKit.in).
Error: L6200E: Symbol WebCore::PluginView::~PluginView__deallocating() multiply defined (by QtWebKit.in and QtWebKit.in).
Error: L6200E: Symbol WebCore::PluginView::~PluginView() multiply defined (by QtWebKit.in and QtWebKit.in).
Error: L6200E: Symbol WebCore::PluginView::~PluginView__sub_object() multiply defined (by QtWebKit.in and QtWebKit.in).
Error: L6200E: Symbol thunk{-36} to WebCore::PluginView::~PluginView__deallocating() multiply defined (by QtWebKit.in and QtWebKit.in).
Error: L6200E: Symbol thunk{-36} to WebCore::PluginView::~PluginView() multiply defined (by QtWebKit.in and QtWebKit.in).
Error: L6200E: Symbol thunk{-40} to WebCore::PluginView::~PluginView__deallocating() multiply defined (by QtWebKit.in and QtWebKit.in).
Error: L6200E: Symbol thunk{-40} to WebCore::PluginView::~PluginView() multiply defined (by QtWebKit.in and QtWebKit.in).
------- Comment #13 From 2009-09-24 05:12:25 PST -------
(From update of attachment 40001 [details])
Shall submit updated patch with build issues fixed
------- Comment #14 From 2009-10-01 02:46:28 PST -------
Created an attachment (id=40429) [details]
Patch with changes incorporated - Compilation issues solved

Changes done for compilation issues to be resolved.

1) Made platformPageClient() method call changes in PluginPackageSymbian.cpp
2) Modified PluginViewSymbian.cpp to incorporate latest PluginView.cpp changes and hence alter few methods of the file accordingly
------- Comment #15 From 2009-10-01 19:35:28 PST -------
I applied the patch and did not see the build issues any more that I reported before. The patch looks fine to me.
------- Comment #16 From 2009-10-07 00:22:04 PST -------
(From update of attachment 40429 [details])
As discussed with Yael on IRC, we need another iteration for this patch. It should be possible to replace the custom event structures with a typedef QEvent NPEvent; We may just need to find an alternate solution for the painting.

Ideally npinterface.h will only contain the interface declaration that allows us to avoid using ordinals for library loading.
------- Comment #17 From 2009-10-08 14:23:06 PST -------
(In reply to comment #16)
> (From update of attachment 40429 [details] [details])
> As discussed with Yael on IRC, we need another iteration for this patch. It
> should be possible to replace the custom event structures with a typedef QEvent
> NPEvent; We may just need to find an alternate solution for the painting.
> 
> Ideally npinterface.h will only contain the interface declaration that allows
> us to avoid using ordinals for library loading.

Simon, I will change npinterface.h to include only 

class NPInterface {
public:
    virtual NPError NP_Initialize(NPNetscapeFuncs *aNPNFuncs, NPPluginFuncs *aNPPFuncs) = 0;
    virtual void NP_Shutdown() = 0;
    virtual char* NP_GetMIMEDescription() = 0;
    virtual void NP_Paint(QPainter& painter, const QRect& rect) = 0;
};

The only problem is that if a plugin does not want to support windowless mode, it is still required to have an empty implementation of NP_Paint().
------- Comment #18 From 2009-10-08 19:10:34 PST -------
(In reply to comment #17)
Never mind, this is not going to work.
NPInterface is a library interface, but we need to call a specific plugin to draw itself, not the library.

I will continue with the draw event approach instead. It is actually very similar to the events approach used for Cocoa FW.
------- Comment #19 From 2009-10-09 01:14:04 PST -------
(In reply to comment #18)
> (In reply to comment #17)
> Never mind, this is not going to work.
> NPInterface is a library interface, but we need to call a specific plugin to
> draw itself, not the library.
> 
> I will continue with the draw event approach instead. It is actually very
> similar to the events approach used for Cocoa FW.

Ok. If it's just one event that we need, then let's create a custom Qt event.
------- Comment #20 From 2009-10-09 04:26:27 PST -------
Regarding painting, we should follow the same approach as both the QuickDraw and CoreGraphics drawing models on Mac use: to store the port-specific info in the NPWindow's window-member (see NP_CGContext, NP_GLContext and NP_Port).

Our struct would then have a QPainter* which is updated on each paint event by a call to NPP_SetWindow before the delivering the paint event to the plugin though the normal NPP_HandleEvent
------- Comment #21 From 2009-10-09 05:53:11 PST -------
(In reply to comment #20)
> Regarding painting, we should follow the same approach as both the QuickDraw
> and CoreGraphics drawing models on Mac use: to store the port-specific info in
> the NPWindow's window-member (see NP_CGContext, NP_GLContext and NP_Port).
> 
> Our struct would then have a QPainter* which is updated on each paint event by
> a call to NPP_SetWindow before the delivering the paint event to the plugin
> though the normal NPP_HandleEvent

I think this is what I am doing, I hope to upload a patch already today.
------- Comment #22 From 2009-10-11 16:27:33 PST -------
Created an attachment (id=41006) [details]
Patch

Reworked the events to be Qt events, except for painting.
------- Comment #23 From 2009-10-12 17:53:29 PST -------
Created an attachment (id=41074) [details]
Patch

Verified that no changes are needed to config.h.
------- Comment #24 From 2009-10-12 23:17:52 PST -------
(From update of attachment 41074 [details])

> -#if !PLATFORM(WIN_OS)
> +#if PLATFORM(SYMBIAN) || !PLATFORM(WIN_OS)

Why is this needed?

PLATFORM(WIN_OS) should not be defined when compiling for symbian, and looking
at wtf/Platform.h I can see that WTF_PLATFORM_WIN_OS is explicitly undefined
when __SYMBIAN32__ is defined.

> +#if COMPILER(WINSCW)
> +#undef XP_WIN
> +#endif
> +

Why is this needed? XP_WIN should not have been defined by bridge/npapi.h in the first place
when compiling with WINSCW for Symbian.

> +#if PLATFORM(SYMBIAN)
> +        NPInterface* npInterface() { return m_npInterface;}
> +#endif // PLATFORM(SYMBIAN)

There's a const missing after the () :), and a space after the semicolon.

> +class DrawEvent : public QEvent {
> +public:
> +    DrawEvent() : QEvent(QEvent::Type(QEvent::User + NPQtEventPaint)) {}
> +    void setPainter(QPainter* p) { m_painter = p; }
> +    QPainter* painter() { return m_painter; }
> +private:
> +    QPainter* m_painter;
> +};

Why is this still needed? Can't we call setNPWindow() before paint for example, set
ws_info to the QPainter pointer and then send a regular QPaintEvent() ?


> +    class PluginContainerSymbian : public QWidget {

It would be nice to merge this code with PluginContainerQt and make
its inheritance from QX11EmbedContainer conditional at compile-time.

I don't mind if this is done in a follow-up patch / separate bug though.


> +    QVector<QRect> rects = r->rects();
> +    for (int i = 1; i < rects.size(); ++i) {

Shouldn't this begin iteration at i = 0?

> +        const QRect& qRect = rects.at(i);
> +        IntRect r(qRect);

Is the explicit conversion necessary? Shouldn't

m_invalidRects.append(rects.at(i));

compile, too, due to IntRect's implicit QRect constructor?


> +    if (m_isWindowed) {
> +        QWebPageClient* client = m_parentFrame->view()->hostWindow()->platformPageClient();
> +        // FIXME this will not work for QGraphicsView.
> +        // But we cannot use winId because it will create a window and on S60,
> +        // QWidgets should not create a window. 
> +        Q_ASSERT(qobject_cast<QWidget*>(client->pluginParent()));
> +        setPlatformWidget(new PluginContainerSymbian(this,
> +            qobject_cast<QWidget*>(client->pluginParent())));
> +        m_npWindow.type = NPWindowTypeWindow;
> +        m_npWindow.window = (void*)platformPluginWidget();

I guess we should either not support windowed plugins when embedding into the graphics view
or we should support that the plugin can be a QGraphicsWidget that we embed. Just a thought :)

I still think supporting only windowless plugins is the best option.

> +    m_npInterface = qobject_cast<NPInterface *>(plugin);

Codingstyle nitpick :)

I recommend to run the newly added files through WebKitTools/Scripts/check-webkit-style


> +unsigned PluginPackage::hash() const
> +{ 
> +    unsigned hashCodes[2] = {
> +        m_path.impl()->hash(),
> +        m_lastModified
> +    };
> +
> +    return StringImpl::computeHash(reinterpret_cast<UChar*>(hashCodes), 2 * sizeof(unsigned) / sizeof(UChar));
> +}
> +
> +bool PluginPackage::equal(const PluginPackage& a, const PluginPackage& b)
> +{
> +    return a.m_description == b.m_description;
> +}

I guess these two functions weren't meant to be here but you intented to use ENABLE_PLUGIN_SIMPLE_HASH instead?

>  {
> -    uint16   event;
> -    uint32   wParam;
> -    uint32   lParam;
> -} NPEvent;
> +        uint16   event;
> +        uint32   wParam;
> +        uint32   lParam;
> +    } NPEvent;

Is this hunk needed? :)

> -#elif defined(XP_WIN)
> +#elif !defined(XP_SYMBIAN) && defined(XP_WIN)

This shouldn't be needed either, if we make sure that XP_WIN is not defined for Symbian.
------- Comment #25 From 2009-10-13 14:41:05 PST -------
Created an attachment (id=41128) [details]
Patch

Addressed most of the issues in Simon's comment #24.
Still left to do in follow up work:
- Add support for windowed plugins in QGraphicsView (Removing support for windowed plugin is not my decision to make :-)
- Remove the hash functions after Laszlo's patch in 30279 is committed
- Merge PluginContainerSymbian with PluginContainerQt
------- Comment #26 From 2009-10-14 07:52:40 PST -------
Created an attachment (id=41161) [details]
Updated patch after hash changes landed

What is left now is 
- Add support for windowed plugins in QGraphicsView
- Merge PluginContainerSymbian with PluginContainerQt
I think this is somewhat related, because I would want to wait for Girish to finish his work on QGraphicsView before getting into either one of the above issues.
------- Comment #27 From 2009-10-14 08:43:54 PST -------
(From update of attachment 41161 [details])

> +class NPInterface {
> +public:
> +    virtual NPError NP_Initialize(NPNetscapeFuncs *aNPNFuncs, NPPluginFuncs *aNPPFuncs) = 0;

Please fix the coding style (* placement) when landing.

> +{
> +    if (platformPluginWidget())
> +        delete platformPluginWidget();

This null pointer check is not needed.
------- Comment #28 From 2009-10-14 10:50:11 PST -------
Committed r49574: <http://trac.webkit.org/changeset/49574>
------- Comment #29 From 2009-10-18 08:25:34 PST -------
(In reply to comment #28)
> Committed r49574: <http://trac.webkit.org/changeset/49574>

The ChangeLog says "Reviewed by" following no name. Just have a look here: http://trac.webkit.org/changeset/49574

That would probably be good to fix.