[webkit-reviews] review denied: [Bug 29302] NPAPI plugin support feature on Webkit for S60 platform : [Attachment 41074] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Yael <yael.aharon at nokia.com>'s
request for review:
Bug 29302: NPAPI plugin support feature on Webkit for S60 platform
https://bugs.webkit.org/show_bug.cgi?id=29302

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

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