Bug 29302

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;'

Yael, does this mean that you will be back working on windowless plugins? :-)

Comment on attachment 39649 [details] Enabling NPAPI plugin support for S60 r- for now We need to discuss the design some more, and there's style issues such as the ones Kenneth mentioned.

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.

Created attachment 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 :-)

> 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 on attachment 39695 [details] Patch with review comments incorporated > 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?

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"

Created attachment 40001 [details] Patch with review comments incorporated

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.

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).

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 on attachment 40001 [details] Patch with review comments incorporated Shall submit updated patch with build issues fixed

Created attachment 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

I applied the patch and did not see the build issues any more that I reported before. The patch looks fine to me.

Comment on attachment 40429 [details] Patch with changes incorporated - Compilation issues solved 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.

(In reply to comment #16) > (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. 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().

(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.

(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.

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

(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.

Created attachment 41006 [details] Patch Reworked the events to be Qt events, except for painting.

Created attachment 41074 [details] Patch Verified that no changes are needed to config.h.

Comment on attachment 41074 [details] Patch > -#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.

Created attachment 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

Created attachment 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 on attachment 41161 [details] Updated patch after hash changes landed > +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.

Committed r49574: <http://trac.webkit.org/changeset/49574>

(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.