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


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2012-01-26 10:00:10 PST -------
Created an attachment (id=124133) [details]
proposed patch
------- Comment #2 From 2012-01-26 11:03:04 PST -------
(From update of attachment 124133 [details])
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 From 2012-01-26 11:33:02 PST -------
Created an attachment (id=124154) [details]
proposed patch
------- Comment #4 From 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 From 2012-01-26 11:48:03 PST -------
(From update of attachment 124154 [details])
Clearing flags on attachment: 124154

Committed r106022: <http://trac.webkit.org/changeset/106022>
------- Comment #6 From 2012-01-26 11:48:10 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #7 From 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 From 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 From 2012-02-03 11:44:05 PST -------
Created an attachment (id=125375) [details]
proposed patch
------- Comment #10 From 2012-02-03 11:47:22 PST -------
Created an attachment (id=125377) [details]
proposed patch
------- Comment #11 From 2012-02-06 05:53:50 PST -------
(From update of attachment 125377 [details])
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 From 2012-02-06 07:50:13 PST -------
Created an attachment (id=125646) [details]
proposed patch
------- Comment #13 From 2012-02-06 07:59:16 PST -------
(In reply to comment #11)
> (From update of attachment 125377 [details] [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 From 2012-02-07 04:39:00 PST -------
Created an attachment (id=125815) [details]
updated patch
------- Comment #15 From 2012-02-07 04:45:37 PST -------
(From update of attachment 125815 [details])
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 From 2012-02-07 05:10:46 PST -------
Committed r106930: <http://trac.webkit.org/changeset/106930>
------- Comment #17 From 2012-02-07 05:11:28 PST -------
(From update of attachment 125815 [details])
Clearing flags from attachment.