Bug 36171 – [Qt] QtWebKit doesn't support NPAPI for DirectFB

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Anders Bakken]

There are Flash plugins available for DirectFB that can be supported without too much effort in QtWebKit.

[Anders Bakken]

I will contribute code for this soon.

[Anders Bakken]

Created attachment 51100 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB

[WebKit Review Bot]

Attachment 51100 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bridge/npapi.h:596:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Bakken]

Created attachment 51106 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB

[Anders Bakken]

Created attachment 51107 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB

[Tor Arne Vestbø]

Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs

[Tor Arne Vestbø]

Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs

[Simon Hausmann]

This patch extends the NPAPI spec by a new platform. I suggest to discuss this addition on the webkit development mailing list first.

[Anders Bakken]

Created attachment 56890 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB

[WebKit Review Bot]

Attachment 56890 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bridge/npapi.h:450:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Bakken]

Created attachment 56908 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB

[Early Warning System Bot]

Attachment 56908 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2343151

[Anders Bakken]

Created attachment 56942 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB

[WebKit Review Bot]

Attachment 56942 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/plugins/qt/PluginViewQt.cpp:72:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bridge/npapi.h:450:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Bakken]

Created attachment 58046 [details]
Patch

[Anders Bakken]

Created attachment 63350 [details]
Patch

[Early Warning System Bot]

Attachment 63350 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3618476

[Anders Bakken]

Created attachment 63351 [details]
Patch

[Anders Bakken]

Created attachment 63382 [details]
Patch

[Antonio Gomes]

It saves reviewers time when you make it explicity what changes from an old to a newer version of your patch.

[Anders Bakken]

Created attachment 63384 [details]
Patch

[Anders Bakken]

Created attachment 63385 [details]
Patch

[Anders Bakken]

Created attachment 63608 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 63608 [details]
Patch

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

[Anders Bakken]

Created attachment 63633 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 63633 [details]
Patch

This seems very isolated apart from the XP_UNIX -> XP_X11 change which I'm not sure about, so I tending to give you a r=me.

Could you please answer my question regarding this before committing?

Ah, unfortunately I will have to give you an r-, due to the patch missing files such as the PluginViewQtDFB.cpp etc.

[Anders Bakken]

Created attachment 63639 [details]
Patch

[Anders Bakken]

(In reply to comment #24)
> (From update of attachment 63608 [details])
> 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

Hi Kenneth

The XP_X11 was a complete oversight on my part. XP_X11 doesn't really exist. XP_UNIX == X11. Mac is XP_MACOSX.

npapi.h in WebKit has diverged a fair bit from the mozilla version. I believe the WebKit occasionally is made functionally equivalent to the Mozilla one by hand.

[Anders Bakken]

(In reply to comment #26)
> (From update of attachment 63633 [details])
> This seems very isolated apart from the XP_UNIX -> XP_X11 change which I'm not sure about, so I tending to give you a r=me.
> 
> Could you please answer my question regarding this before committing?
> 
> Ah, unfortunately I will have to give you an r-, due to the patch missing files such as the PluginViewQtDFB.cpp etc.

Sorry about that. I believe I've addressed all the style issues in the last commit. I didn't see your comment before I committed the second last.

[Anders Bakken]

Created attachment 73107 [details]
Patch

[Early Warning System Bot]

Attachment 73107 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5285023

[Antonio Gomes]

Comment on attachment 73107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73107&action=review

Basically, I would like to see all X11 related code guard by x11 defines.

> WebCore/bridge/npapi.h:92
> +#if defined(XP_UNIX) && !defined(XP_DFB)

I'd prefer if you did:

#if defined(XP_UNIX) && defined(X_PROTOCOL)

So you'd be including X-related stuff when it is X.

> WebCore/bridge/npapi.h:246
> +#if defined(XP_UNIX) && !defined(XP_DFB)

Ditto.

> WebCore/bridge/npruntime_internal.h:32
> +#if defined XP_UNIX && !defined(XP_DFB)

Ditto

> WebCore/plugins/PluginView.cpp:373
> +#if defined(XP_X11) && !defined(XP_DFB)

here you could do:

#if defined(XP_X11_ && PLATFORM(X11)

[Antonio Gomes]

[CCing Torarne for the PluginViewQt part and Andersca for the npapi changes]

[Tor Arne Vestbø]

(In reply to comment #33)
> [CCing Torarne for the PluginViewQt part and Andersca for the npapi changes]

I will comment on this next week, please don't commit as is.

[Tor Arne Vestbø]

Comment on attachment 73107 [details]
Patch

I don't like this patch, for a few reasons:

- It mixes the new XP_DFB with XP_UNIX. As far as I know the XP-defines are mutually exclusive. XP_UNIX is really XP_X11, and should be treated that way. It should also be defined in npapi.h, and the npapi.h extension should be brought up to a wider audience to make sure it has accept, for example at the plugin-futures list [1].

- It's Qt-specific. If we want to add directfb support we should do it in a way that can be shared between ports, PluginViewDirectFB, where the platform bits are abstracted or #ifdefed. See PluginViewMac for an example.  We already have a lot of duplicated code in PluginViewQt and PluginViewGTK, which are both largely implementing what should be PluginViewX11. Let's not make the same mistake with this patch.
 

[1] https://mail.mozilla.org/listinfo/plugin-futures

[Martin Robinson]

QtWebKit is gone now.