[webkit-reviews] review denied: [Bug 36171] [QT] QtWebKit doesn't support NPAPI for DirectFB : [Attachment 63608] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Anders Bakken
<agbakken at gmail.com>'s request for review:
Bug 36171: [QT] QtWebKit doesn't support NPAPI for DirectFB
https://bugs.webkit.org/show_bug.cgi?id=36171

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
WebCore/bridge/npapi.h:92
 +  #if defined(XP_UNIX) && !defined(XP_DFB)
Doesn't this have to be upstreamed to npapi.h etc used by Mozilla? I think
these headers are copied from mozilla, though I might be wrong.

WebCore/plugins/PluginView.cpp:366
 +  #if defined(XP_X11) && !defined(XP_DFB)
You are chaning a XP_UNIX to XP_X11, are you sure that is right? I guess
XP_UNIX is true on mac.

WebCore/plugins/qt/PluginViewQtDFB.cpp:27
 +  #include "PluginViewQtDFB.h"
I guess PluginViewDFBQt would have made more sense

WebCore/plugins/qt/PluginViewQtDFB.cpp:48
 +  extern Q_GUI_EXPORT IDirectFBSurface* qt_directfb_surface_for_pixmap(const
QPixmap &pixmap);
& is wrongly aligned

WebCore/plugins/qt/PluginViewQtDFB.cpp:51
 +  namespace WebCore {
Add a newline after this

WebCore/plugins/qt/PluginViewQtDFB.cpp:52
 +  void PluginViewQtDFB::clearWindowInfo(PluginViewQtDFB::DFBWindowInfo
*windowInfo)
* is wrongly algined

WebCore/plugins/qt/PluginViewQtDFB.cpp:63
 +					     const DFBWindowInfo& windowInfo)
put the function definition on one line

WebCore/plugins/qt/PluginViewQtDFB.cpp:64
 +	: m_pluginView(pluginView), m_dfbSurface(0),
m_installedEventFilter(false), m_dfbWindowInfo(windowInfo)
Put these on separate lines starting with ,

WebCore/plugins/qt/PluginViewQtDFB.cpp:106
 +  void PluginViewQtDFB::paint(GraphicsContext* context, const IntRect& rect)
a bit of newlines in this method would be nice, grouping the related things
together

WebCore/plugins/qt/PluginViewQtDFB.cpp:127
 +  bool PluginViewQtDFB::dispatchEvent(const KeyboardEvent* event)
Here as well. This code is hard reading

WebCore/plugins/qt/PluginViewQtDFB.cpp:145
 +	case Qt::Key_Backspace:
Can't this be moved to another method? It can always be inline

WebCore/plugins/qt/PluginViewQtDFB.cpp:724
 +	case FocusIn: window.type = DWET_GOTFOCUS; break;
dont put  this on one line

WebCore/plugins/qt/PluginViewQtDFB.cpp:731
 +  bool PluginViewQtDFB::eventFilter(QObject* , QEvent* e)
We normally abbriviate event with 'ev'

WebCore/plugins/qt/PluginViewQtDFB.h:40
 +  class KeyboardEvent;
Add a newline after this