Bug 52469 – [Qt] Made QtScript dependency optional

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Konstantin Tokarev]

This patch fixes compilation when QtScript module is disabled.

[Konstantin Tokarev]

Created attachment 78982 [details]
Made QtScript dependency optional

[WebKit Review Bot]

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

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

Source/WebCore/bridge/qt/qt_runtime.h:244:  Should have a space between // and comment  [whitespace/comments] [4]
WebKit/qt/Api/qwebelement.cpp:806:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/bridge/qt/qt_instance.cpp:401:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/bridge/qt/qt_class.cpp:228:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/bridge/qt/qt_class.h:61:  Should have a space between // and comment  [whitespace/comments] [4]
WebKit/qt/Api/qwebframe.cpp:633:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/bridge/qt/qt_instance.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bridge/qt/qt_instance.h:99:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 8 in 16 files


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

[Konstantin Tokarev]

Created attachment 79062 [details]
Made QtScript dependency optional

[Simon Hausmann]

I don't really see us moving in that direction.

Right now the only dependency on QtScript we have is that $$prefix/include/QtScript is present. The library does not even have to be compiled!

In the future we want to make QtScript a real link-time dependency.

[Konstantin Tokarev]

(In reply to comment #4)
> I don't really see us moving in that direction.
> 
> Right now the only dependency on QtScript we have is that $$prefix/include/QtScript is present. The library does not even have to be compiled!

I realize that, but $$prefix/include/QtScript is not normally installed when script module is not enabled. I've asked for advice on [e-mail], and Benjamin Poulain adviced me to disable bridge if QtScript is missing

[Simon Hausmann]

Comment on attachment 79062 [details]
Made QtScript dependency optional

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

Ok, apart from the other notes I made in the review, I think for this patch to go in we should only disable the parts of the code that actually rely on QScriptEngine::ValueOwnership.

One way of doing that would be to disable the QObject bindings, another way would be to introduce a QtWebKit
internal enum that mirrors QScriptEngine::ValueOwnership and when QT_NO_SCRIPT is define only the
addToJavaScriptWindowObject method overload that takes the QScriptEngine parameter is excluded from
the build. The other overload would remain intact.

Either way when QT_NO_SCRIPT is define, I think QWebFrame::evaluateJavaScript should remain functional.

So let me revert my earlier statement. What you're trying to do makes sense, but I think the patch needs a bit of refinement :)

> Source/WebCore/bindings/js/ScriptControllerQt.cpp:54
> +#ifndef QT_NO_SCRIPT
>      if (widget->isPluginView()) {
>          PluginView* pluginView = static_cast<PluginView*>(widget);
>          return pluginView->bindingInstance();

This code - the one to return the binding instance of the PluginView is needed for Netscape plugin scripting to work.
The QT_NO_SCRIPT #ifdef has to be placed below that.

> Source/WebCore/platform/qt/PlatformMouseEventQt.cpp:39
> +#ifndef QT_NO_GRAPHICSVIEW

This should be handled in a different patch.

[Konstantin Tokarev]

(In reply to comment #6)
> One way of doing that would be to disable the QObject bindings, another way would be to introduce a QtWebKit internal enum that mirrors ScriptEngine::ValueOwnership and when QT_NO_SCRIPT is define only the
addToJavaScriptWindowObject method overload that takes the QScriptEngine parameter is excluded from the build. The other overload would remain intact.

Which way is more preferable?

[Simon Hausmann]

(In reply to comment #7)
> (In reply to comment #6)
> > One way of doing that would be to disable the QObject bindings, another way would be to introduce a QtWebKit internal enum that mirrors ScriptEngine::ValueOwnership and when QT_NO_SCRIPT is define only the
> addToJavaScriptWindowObject method overload that takes the QScriptEngine parameter is excluded from the build. The other overload would remain intact.
> 
> Which way is more preferable?

Hmm, I prefer the latter, just disabling the overload.

[Konstantin Tokarev]

> Hmm, I prefer the latter, just disabling the overload.

It requires addition of new public header which should be included instead of <QtScript/qscriptengine.h>, e.g., "scriptvalueownership.h". Where should it be placed (Source/WebCore/bridge/qt, /home/kostya/projects/git/WebKit/Source/WebKit/qt, or somewhere else)?

Also, do you have something against separate option for disabling QObject bindings?

[Benjamin Poulain]

*** Bug 49748 has been marked as a duplicate of this bug. ***

[Benjamin Poulain]

*** Bug 34519 has been marked as a duplicate of this bug. ***

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.