[webkit-reviews] review denied: [Bug 52469] [Qt] Made QtScript dependency optional : [Attachment 79062] Made QtScript dependency optional

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Konstantin Tokarev
<annulen at yandex.ru>'s request for review:
Bug 52469: [Qt] Made QtScript dependency optional
https://bugs.webkit.org/show_bug.cgi?id=52469

Attachment 79062: Made QtScript dependency optional
https://bugs.webkit.org/attachment.cgi?id=79062&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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.