Bug 77111 - [Qt][WK2] Add the componentComplete method to WebView
: [Qt][WK2] Add the componentComplete method to WebView
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: All All
: P3 Normal
Assigned To: Andras Becsi
: Qt
Depends on: 77305 77632
Blocks: 76275
  Show dependency treegraph
 
Reported: 2012-01-26 09:55 PST by Andras Becsi
Modified: 2012-02-07 05:11 PST (History)
10 users (show)

See Also:


Attachments
proposed patch (6.42 KB, patch)
2012-01-26 10:00 PST, Andras Becsi
no flags Details | Formatted Diff | Diff
proposed patch (6.14 KB, patch)
2012-01-26 11:33 PST, Andras Becsi
no flags Details | Formatted Diff | Diff
proposed patch (17.52 KB, patch)
2012-02-03 11:44 PST, Andras Becsi
no flags Details | Formatted Diff | Diff
proposed patch (17.52 KB, patch)
2012-02-03 11:47 PST, Andras Becsi
no flags Details | Formatted Diff | Diff
proposed patch (17.13 KB, patch)
2012-02-06 07:50 PST, Andras Becsi
no flags Details | Formatted Diff | Diff
updated patch (17.67 KB, patch)
2012-02-07 04:39 PST, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 2012-01-26 09:55:04 PST
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.
Comment 1 Andras Becsi 2012-01-26 10:00:10 PST
Created attachment 124133 [details]
proposed patch
Comment 2 Kenneth Rohde Christiansen 2012-01-26 11:03:04 PST
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
Comment 3 Andras Becsi 2012-01-26 11:33:02 PST
Created attachment 124154 [details]
proposed patch
Comment 4 Andras Becsi 2012-01-26 11:36:03 PST
(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 5 Andras Becsi 2012-01-26 11:48:03 PST
Comment on attachment 124154 [details]
proposed patch

Clearing flags on attachment: 124154

Committed r106022: <http://trac.webkit.org/changeset/106022>
Comment 6 Andras Becsi 2012-01-26 11:48:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Csaba Osztrogonác 2012-01-29 15:53:29 PST
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
Comment 8 Csaba Osztrogonác 2012-01-29 16:04:28 PST
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
Comment 9 Andras Becsi 2012-02-03 11:44:05 PST
Created attachment 125375 [details]
proposed patch
Comment 10 Andras Becsi 2012-02-03 11:47:22 PST
Created attachment 125377 [details]
proposed patch
Comment 11 Simon Hausmann 2012-02-06 05:53:50 PST
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 :)
Comment 12 Andras Becsi 2012-02-06 07:50:13 PST
Created attachment 125646 [details]
proposed patch
Comment 13 Andras Becsi 2012-02-06 07:59:16 PST
(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.
Comment 14 Andras Becsi 2012-02-07 04:39:00 PST
Created attachment 125815 [details]
updated patch
Comment 15 Andras Becsi 2012-02-07 04:45:37 PST
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.
Comment 16 Andras Becsi 2012-02-07 05:10:46 PST
Committed r106930: <http://trac.webkit.org/changeset/106930>
Comment 17 Andras Becsi 2012-02-07 05:11:28 PST
Comment on attachment 125815 [details]
updated patch

Clearing flags from attachment.