[Webkit-unassigned] [Bug 29302] NPAPI plugin support feature on Webkit for S60 platform

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 12 23:17:53 PDT 2009


https://bugs.webkit.org/show_bug.cgi?id=29302


Simon Hausmann <hausmann at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41074|review?                     |review-
               Flag|                            |




--- Comment #24 from Simon Hausmann <hausmann at webkit.org>  2009-10-12 23:17:52 PDT ---
(From update of attachment 41074)

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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list