Bug 49750 – [Qt] Compile with QT_NO_GRAPHICSVIEW

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Sam Magnuson]

Compile when QT_NO_GRAPHICSVIEW is defined. I want to eliminate all that code and don't use QGraphicsWebView.

[Sam Magnuson]

Created attachment 74261 [details]
Patch

[Laszlo Gombos]

Comment on attachment 74261 [details]
Patch

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

The patch does not seem to apply r- for that (file WebKit/qt/ChangeLog).

How have you tested the patch ?

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:119
> +#if !defined(QT_NO_GRAPHICSVIEW)

Is this needed if USE(ACCELERATED_COMPOSITING) or USE(TEXTURE_MAPPER) is false ?

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:259
> +#if !defined(QT_NO_GRAPHICSVIEW)

Ditto.

[Sam Magnuson]

Created attachment 78614 [details]
Patch

[Laszlo Gombos]

Created attachment 79050 [details]
Rebaseline for cq

[WebKit Commit Bot]

Comment on attachment 79050 [details]
Rebaseline for cq

Clearing flags on attachment: 79050

Committed r75870: <http://trac.webkit.org/changeset/75870>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Laszlo Gombos]

Created attachment 79088 [details]
Hook up QT_NO_GRAPHICSVIEW with qt_minimal configuration.

Reopen to fix a few remaining issues building with QT_NO_GRAPHICSVIEW. In addition enable building without accelerated compositing.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1

WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:271:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 22 files


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

[Laszlo Gombos]

Created attachment 79089 [details]
fix style

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Laszlo Gombos]

Created attachment 79124 [details]
remove changes from GraphicsLayerQt* to fix build

[Konstantin Tokarev]

> Source/JavaScriptCore/wtf/Platform.h:1082
> +#if (PLATFORM(MAC) && !defined(BUILDING_ON_TIGER)) || PLATFORM(IOS) || PLATFORM(QT) && (!defined(QT_NO_GRAPHICSVIEW) || USE(TEXTURE_MAPPER)) || (PLATFORM(WIN) && !OS(WINCE) &&!defined(WIN_CAIRO))

QT_NO_GRAPHICSVIEW check requires inclusion of qconfig.h, if shrinked Qt configuration is used

> WebKit.pri:142
>  }

I'm building with "Tools/Scripts/build-webkit --qt --minimal" on Linux, however this defines does not appear in g++ command line. But I'm using largely shrinked Qt (embedded) configuration

> WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:20
> +#ifndef QT_NO_GRAPHICSVIEW

This line should go after line 21 to get QT_NO_GRAPHICSVIEW defined if shrinked Qt configuration is used

[Konstantin Tokarev]

I also needed to move moc_qgraphicswebview.cpp into ifdef guard to build:

diff --git a/Source/WebKit/qt/Api/qgraphicswebview.cpp b/Source/WebKit/qt/Api/qgraphicswebview.cpp
index b6ac31b..19feda3 100644
--- a/Source/WebKit/qt/Api/qgraphicswebview.cpp
+++ b/Source/WebKit/qt/Api/qgraphicswebview.cpp
@@ -1145,6 +1145,7 @@ void QGraphicsWebView::inputMethodEvent(QInputMethodEvent* ev)
     \sa QWebPage::linkDelegationPolicy()
 */
 
+#include "moc_qgraphicswebview.cpp"
+
 #endif // QT_NO_GRAPHICSVIEW
 
-#include "moc_qgraphicswebview.cpp"

[Eric Seidel (no email)]

Comment on attachment 78614 [details]
Patch

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 78614 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Laszlo Gombos]

Created attachment 81391 [details]
rebaseline + minor fixes as suggested

[Konstantin Tokarev]

I think it's better to add 

#if PLATFORM(QT)
#include <QtCore/qconfig.h>
#endif

to Platform.h somewhere before condition with QT_NO_GRAPHICSVIEW is added (useful if custom Qt configuration is used)

[Alexis Menard (darktears)]

Comment on attachment 81391 [details]
rebaseline + minor fixes as suggested

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

> Source/WebKit/qt/Api/qwebview.cpp:38
> +

Why this include?

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:-20
> -#include "../util.h"

Could you instead edit the tests.pro and remove the subdir completely? If the config is minimal then don't add the dir, It's a bit more elegant to me.

> Tools/QtTestBrowser/launcherwindow.h:98
> +#endif

It needs a rebase the code changed a bit. No big deal though.

[Laszlo Gombos]

(In reply to comment #18)
> (From update of attachment 81391 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81391&action=review
> 
> > Source/WebKit/qt/Api/qwebview.cpp:38
> > +
> 
> Why this include?

I had an explanation in the ChangeLog:

Api/qwebview.cpp: Include PassOwnPtr.h for OwnPtr::release() in QWebViewPrivate::detachCurrentPage.

> 
> > Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:-20
> > -#include "../util.h"
> 
> Could you instead edit the tests.pro and remove the subdir completely? If the config is minimal then don't add the dir, It's a bit more elegant to me.

Guarding entire files is preferred over configuration in the build system (e.g. pro files) for the project. This is the consensus based on some previous webkit dev discussions and not just my personal opinion (although I agree with it).

Also I would not disable all the autotest for the minimal config as I was hoping the (minimal) buildbot could actually run some of the autotests.

> 
> > Tools/QtTestBrowser/launcherwindow.h:98
> > +#endif
> 
> It needs a rebase the code changed a bit. No big deal though.

I will wait for more comments and will rebase.

[Konstantin Tokarev]

What happened to this patch?