Summary: | [Qt] QtWebKit doesn't support NPAPI for DirectFB | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Anders Bakken <agbakken> | ||||||||||||||||||||||||||||||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||||||||||||||||||||||||
Severity: | Enhancement | CC: | andersca, benjamin, hausmann, jturcotte, kenneth, laszlo.gombos, mrobinson, noam, ojan, ossy, tonikitoo, vestbo, webkit.review.bot | ||||||||||||||||||||||||||||||||||
Priority: | P5 | Keywords: | Qt, QtTriaged | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Anders Bakken
2010-03-16 09:41:33 PDT
I will contribute code for this soon. Created attachment 51100 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
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.
Created attachment 51106 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
Created attachment 51107 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs Bugs relating to the Qt port of WebKit should have the Qt keyword. See http://trac.webkit.org/wiki/QtWebKitBugs This patch extends the NPAPI spec by a new platform. I suggest to discuss this addition on the webkit development mailing list first. Created attachment 56890 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
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.
Created attachment 56908 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
Attachment 56908 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2343151 Created attachment 56942 [details]
Patch that enables Windowless NPAPI for Qt/QWS with DirectFB
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.
Created attachment 58046 [details]
Patch
Created attachment 63350 [details]
Patch
Attachment 63350 [details] did not build on qt: Build output: http://queues.webkit.org/results/3618476 Created attachment 63351 [details]
Patch
Created attachment 63382 [details]
Patch
It saves reviewers time when you make it explicity what changes from an old to a newer version of your patch. Created attachment 63384 [details]
Patch
Created attachment 63385 [details]
Patch
Created attachment 63608 [details]
Patch
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
Created attachment 63633 [details]
Patch
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.
Created attachment 63639 [details]
Patch
(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. (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. Created attachment 73107 [details]
Patch
Attachment 73107 [details] did not build on qt: Build output: http://queues.webkit.org/results/5285023 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) [CCing Torarne for the PluginViewQt part and Andersca for the npapi changes] (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 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 QtWebKit is gone now. |