Bug 36171 - [Qt] QtWebKit doesn't support NPAPI for DirectFB
Summary: [Qt] QtWebKit doesn't support NPAPI for DirectFB
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P5 Enhancement
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-03-16 09:41 PDT by Anders Bakken
Modified: 2013-12-09 10:24 PST (History)
13 users (show)

See Also:


Attachments
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB (42.36 KB, patch)
2010-03-18 14:58 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB (84.78 KB, patch)
2010-03-18 16:01 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB (84.78 KB, patch)
2010-03-18 16:03 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB (43.07 KB, patch)
2010-05-24 08:48 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB (43.11 KB, patch)
2010-05-24 11:33 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB (45.78 KB, patch)
2010-05-24 16:22 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (43.08 KB, patch)
2010-06-07 10:50 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (43.86 KB, patch)
2010-08-03 10:40 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (43.18 KB, patch)
2010-08-03 10:48 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (44.29 KB, patch)
2010-08-03 15:04 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (44.29 KB, patch)
2010-08-03 15:27 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (47.60 KB, patch)
2010-08-03 15:29 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (47.77 KB, patch)
2010-08-05 11:06 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (17.48 KB, patch)
2010-08-05 14:13 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (47.40 KB, patch)
2010-08-05 14:43 PDT, Anders Bakken
no flags Details | Formatted Diff | Diff
Patch (55.30 KB, patch)
2010-11-05 13:25 PDT, Anders Bakken
tonikitoo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Bakken 2010-03-16 09:41:33 PDT
There are Flash plugins available for DirectFB that can be supported without too much effort in QtWebKit.
Comment 1 Anders Bakken 2010-03-16 18:06:35 PDT
I will contribute code for this soon.
Comment 2 Anders Bakken 2010-03-18 14:58:55 PDT
Created attachment 51100 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
Comment 3 WebKit Review Bot 2010-03-18 15:05:30 PDT
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.
Comment 4 Anders Bakken 2010-03-18 16:01:26 PDT
Created attachment 51106 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
Comment 5 Anders Bakken 2010-03-18 16:03:00 PDT
Created attachment 51107 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
Comment 6 Tor Arne Vestbø 2010-03-22 06:36:26 PDT
Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs
Comment 7 Tor Arne Vestbø 2010-03-22 06:36:57 PDT
Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs
Comment 8 Simon Hausmann 2010-03-28 14:24:16 PDT
This patch extends the NPAPI spec by a new platform. I suggest to discuss this addition on the webkit development mailing list first.
Comment 9 Anders Bakken 2010-05-24 08:48:12 PDT
Created attachment 56890 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
Comment 10 WebKit Review Bot 2010-05-24 08:52:58 PDT
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.
Comment 11 Anders Bakken 2010-05-24 11:33:59 PDT
Created attachment 56908 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
Comment 12 Early Warning System Bot 2010-05-24 15:04:21 PDT
Attachment 56908 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2343151
Comment 13 Anders Bakken 2010-05-24 16:22:25 PDT
Created attachment 56942 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
Comment 14 WebKit Review Bot 2010-05-24 16:25:14 PDT
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.
Comment 15 Anders Bakken 2010-06-07 10:50:15 PDT
Created attachment 58046 [details]
Patch
Comment 16 Anders Bakken 2010-08-03 10:40:48 PDT
Created attachment 63350 [details]
Patch
Comment 17 Early Warning System Bot 2010-08-03 10:48:03 PDT
Attachment 63350 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3618476
Comment 18 Anders Bakken 2010-08-03 10:48:46 PDT
Created attachment 63351 [details]
Patch
Comment 19 Anders Bakken 2010-08-03 15:04:48 PDT
Created attachment 63382 [details]
Patch
Comment 20 Antonio Gomes 2010-08-03 15:07:13 PDT
It saves reviewers time when you make it explicity what changes from an old to a newer version of your patch.
Comment 21 Anders Bakken 2010-08-03 15:27:01 PDT
Created attachment 63384 [details]
Patch
Comment 22 Anders Bakken 2010-08-03 15:29:57 PDT
Created attachment 63385 [details]
Patch
Comment 23 Anders Bakken 2010-08-05 11:06:39 PDT
Created attachment 63608 [details]
Patch
Comment 24 Kenneth Rohde Christiansen 2010-08-05 11:20:31 PDT
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
Comment 25 Anders Bakken 2010-08-05 14:13:41 PDT
Created attachment 63633 [details]
Patch
Comment 26 Kenneth Rohde Christiansen 2010-08-05 14:22:55 PDT
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.
Comment 27 Anders Bakken 2010-08-05 14:43:37 PDT
Created attachment 63639 [details]
Patch
Comment 28 Anders Bakken 2010-08-05 14:54:45 PDT
(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.
Comment 29 Anders Bakken 2010-08-05 14:56:06 PDT
(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.
Comment 30 Anders Bakken 2010-11-05 13:25:23 PDT
Created attachment 73107 [details]
Patch
Comment 31 Early Warning System Bot 2010-11-05 14:05:22 PDT
Attachment 73107 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5285023
Comment 32 Antonio Gomes 2010-11-06 06:46:59 PDT
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)
Comment 33 Antonio Gomes 2010-11-06 06:48:49 PDT
[CCing Torarne for the PluginViewQt part and Andersca for the npapi changes]
Comment 34 Tor Arne Vestbø 2010-11-06 19:15:34 PDT
(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.
Comment 35 Tor Arne Vestbø 2010-11-10 05:41:05 PST
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
Comment 36 Martin Robinson 2013-12-09 10:24:40 PST
QtWebKit is gone now.