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

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Rohini Ananth
<rohini.ananth 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 39695: Patch with review comments incorporated
https://bugs.webkit.org/attachment.cgi?id=39695&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
> 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?