Bug 77111

Created attachment 124133 [details] proposed patch

Comment on attachment 124133 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=124133&action=review > Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:47 > if (!m_webViewPrivate->interactionEngine) > - return QVariant(); > + return QVariant(0); Default scale is 1.0 > Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:56 > QVariant QWebViewportInfo::devicePixelRatio() const > { > if (!m_webViewPrivate->interactionEngine) > - return QVariant(); > + return QVariant(0); > This can actually be queried from JS, so it should be possible to return the right value here > Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:64 > if (!m_webViewPrivate->interactionEngine) > - return QVariant(); > + return QVariant(0); > default would be 1.0 > Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:88 > if (!m_webViewPrivate->interactionEngine) > - return QVariant(); > + return QVariant(false); > default is false for desktop as we dont support it, but the page actually allow it by default

Created attachment 124154 [details] proposed patch

(In reply to comment #2) Thanks, Kenneth, for the quick review. Setting the values to the default values of QtViewportInteractionEngine::Constraints seems to be better.

Comment on attachment 124154 [details] proposed patch Clearing flags on attachment: 124154 Committed r106022: <http://trac.webkit.org/changeset/106022>

All reviewed patches have been landed. Closing bug.

Sorry, but I had to roll it out, because it broke the Qt-WK2 API tests. before this patch: TOTALS: 112 passed, 0 failed, 0 skipped - http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/19228/steps/API%20tests/logs/stdio after this patch: TOTALS: 6 passed, 1 failed, 0 skipped - http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/19229/steps/API%20tests/logs/stdio

Additionally it made multi touch tests flakey crash: http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r106024%20%2819230%29/results.html

Created attachment 125375 [details] proposed patch

Created attachment 125377 [details] proposed patch

Comment on attachment 125377 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=125377&action=review > Source/WebKit2/ChangeLog:11 > + Suspend the page and delay the dispatch of load succes on startup until succes -> success > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:744 > +static QDeclarativeComponent* s_webViewComponent = 0; > + > +QQuickWebView* QQuickWebViewExperimental::createWebView() > +{ > + ASSERT(s_webViewComponent); > + > + QObject* viewInstance = s_webViewComponent->create(); > + ASSERT(viewInstance); > + > + return qobject_cast<QQuickWebView*>(viewInstance); > +} > + > +void QQuickWebViewExperimental::prepareWebViewComponent(const QString& qmlImportPath, QObject* parent) > +{ > + ASSERT(!s_webViewComponent); > + ASSERT(!qmlImportPath.isEmpty()); > + > + static QDeclarativeEngine* engine = new QDeclarativeEngine(parent); > + engine->addImportPath(qmlImportPath); > + > + s_webViewComponent = new QDeclarativeComponent(engine, parent); > + > + s_webViewComponent->setData(QByteArrayLiteral("import QtQuick 2.0\n" > + "import QtWebKit 3.0\n" > + "WebView {}") > + , QUrl()); > +} I'm not a fan of global pointers that can become dangling (for example if the parent dies). I understand that it's an optimization to create the QDeclarativeComponent only once, but if one single unit test is the only place where it is called, then I'd prefer the code to be moved there (and therefore get rid of the global variable in the library here). > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:71 > + virtual void onComponentComplete() { } Why is this method re-implemented but empty in the body? > Source/WebKit2/UIProcess/API/qt/qwebviewportinfo.cpp:81 > + return QVariant(1.8); Shouldn't those constants come from somewhere else? For example via return QtViewportInteractionEngine::Constraints().maximumScale; maybe? > Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp:65 > + QQuickWebViewExperimental::prepareWebViewComponent(QLatin1Literal(IMPORT_DIR), this); I think technically we should assume paths to be encoded in utf-8, not latin1. You could just do a QString::fromUtf8 - this isn't performance critical :)

Created attachment 125646 [details] proposed patch

(In reply to comment #11) > (From update of attachment 125377 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125377&action=review > > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:71 > > + virtual void onComponentComplete() { } > > Why is this method re-implemented but empty in the body? > Since the split of the private class into flickable/legacy we do not have a boolean any more but have virtual functions. For the legacy webview we do nothing specific in componentComplete so the base method has an empty body but for the flickable case we reimplement it and do the initialization of the flickProvider. This is in accordance with the other functions in QQuickWebViewPrivate which have an empty body.

Created attachment 125815 [details] updated patch

Comment on attachment 125815 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=125815&action=review > Source/WebKit2/ChangeLog:14 > + the initialization depends on the componentCoplete method being called. D'oh, I m going to fix this typo before landing.

Committed r106930: <http://trac.webkit.org/changeset/106930>

Comment on attachment 125815 [details] updated patch Clearing flags from attachment.