Bug 77111 – [Qt][WK2] Add the componentComplete method to WebView

• Component: New Bugs
• Status: RESOLVED FIXED

[Andras Becsi]

Do the final steps of touch/desktop initialization when the QML engine has completed the construction of the WebView.
This is needed in order to be able to request the QDeclarativeEngine associated with the WebView object in the touch initialization with the qmlEngine function.

[Andras Becsi]

Created attachment 124133 [details]
proposed patch

[Kenneth Rohde Christiansen]

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

[Andras Becsi]

Created attachment 124154 [details]
proposed patch

[Andras Becsi]

(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.

[Andras Becsi]

Comment on attachment 124154 [details]
proposed patch

Clearing flags on attachment: 124154

Committed r106022: <http://trac.webkit.org/changeset/106022>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Andras Becsi]

Created attachment 125375 [details]
proposed patch

[Andras Becsi]

Created attachment 125377 [details]
proposed patch

[Simon Hausmann]

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 :)

[Andras Becsi]

Created attachment 125646 [details]
proposed patch

[Andras Becsi]

(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.

[Andras Becsi]

Created attachment 125815 [details]
updated patch

[Andras Becsi]

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.

[Andras Becsi]

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

[Andras Becsi]

Comment on attachment 125815 [details]
updated patch

Clearing flags from attachment.