[Qt] QTouchWebView missing navigationPolicyForUrl

• Component: WebKit2
• Status: RESOLVED INVALID

[Gopal Raghavan]

This feature is required to handle some custom navigation policies in TouchWebView.

[Gopal Raghavan]

Created attachment 110046 [details]
implementation of navigation policy

Implemented navigationPolicyForUrl. This is similar to DesktopWebView implementation. It provides the flexibiltiy to specify custom navigation policies for TouchWebView.

Regarding testcase. Looks like qmltests are disabled for qt5. I also need some suggestion on handling mouseClick's.
Thanks,

[Chang Shu]

Comment on attachment 110046 [details]
implementation of navigation policy

View in context: https://bugs.webkit.org/attachment.cgi?id=110046&action=review

> Source/WebKit2/UIProcess/API/qt/qtouchwebview.h:47
> +    enum NavigationPolicy {
> +         UsePolicy,
> +         DownloadPolicy,
> +         IgnorePolicy
> +     };
> +

I am a bit confused why we have to define a new set of enums here and do conversions to the enums defined in the base class.

[Gopal Raghavan]

The PolicyAction enums defined in PolicyInterface.h is not available to qml. The current implementation enables the following in qml:

TouchWebView {
     id: webView
     ......
     .....
    function navigationPolicyForUrl(url, button, modifiers) {
        // link delegation
        if (button == Qt.LeftButton && pageLoaded) {
            return webView.TouchWebView.IgnorePolicy
        }
        return webView.TouchWebView.UsePolicy
    }
}

[Viatcheslav Ostapenko]

Comment on attachment 110046 [details]
implementation of navigation policy

View in context: https://bugs.webkit.org/attachment.cgi?id=110046&action=review

> Source/WebKit2/UIProcess/qt/qtouchwebpageproxy.h:43
> +    QTouchWebPageProxy(QTouchWebViewPrivate*, TouchViewInterface*, ViewportInteractionEngine*);

Why do you need QTouchWebViewPrivate class here?
Why PolicyInterface is not good?
Also, I would change order of constructor parameters to make it the same as in QtWebPageProxy.

[Gopal Raghavan]

Created attachment 110739 [details]
updated patch with reviewer comments

Good point. Fixed the interface.
Thanks

[Chang Shu]

I am still not convinced that we have to manipulate the lower level C++ code which throws away the class inheritance just to satisfy a drawback in the higher level qml binding. What prevents PolicyInterface.h being available to qml?

(In reply to comment #3)
> The PolicyAction enums defined in PolicyInterface.h is not available to qml. The current implementation enables the following in qml:
> 
> TouchWebView {
>      id: webView
>      ......
>      .....
>     function navigationPolicyForUrl(url, button, modifiers) {
>         // link delegation
>         if (button == Qt.LeftButton && pageLoaded) {
>             return webView.TouchWebView.IgnorePolicy
>         }
>         return webView.TouchWebView.UsePolicy
>     }
> }

[Gopal Raghavan]

Ideally, if QTouchWebView could be derived out of PolicyInterface we should be all set. Although, PolicyInterface looks like an api it is not publicly available within UIProcess/API/qt. Therefore QTouchWebViewPrivate inherits it and not QTouchWebView. Because of this restriction, we need the translation of enums using toPolicyAction.

Hope this clarifies the situation.
Thanks

[Chang Shu]

(In reply to comment #7)
> Ideally, if QTouchWebView could be derived out of PolicyInterface we should be all set. Although, PolicyInterface looks like an api it is not publicly available within UIProcess/API/qt. Therefore QTouchWebViewPrivate inherits it and not QTouchWebView. Because of this restriction, we need the translation of enums using toPolicyAction.
> 
> Hope this clarifies the situation.
> Thanks

Thanks. It's pretty clear now the problem is not qml itself but the hidden PolicyInterface. I think we should make it public. Any objections, Kenneth and Kling?

[Chang Shu]

Comment on attachment 110739 [details]
updated patch with reviewer comments

View in context: https://bugs.webkit.org/attachment.cgi?id=110739&action=review

> Source/WebKit2/UIProcess/API/qt/tests/tests.pro:3
> +lessThan(QT_MAJOR_VERSION, 6): SUBDIRS += qmltests

Would this possibly break the buildbot? If not, we can simply remove the condition. Or it should be the other way around? I will add Ossy for commenting this.

[Csaba Osztrogonác]

I will update Qt5 on the bot tomorrow evening. Please wait a little bit with landing.

[Gopal Raghavan]

Created attachment 110864 [details]
updated patch

That change to tests.pro is unintentional. I didn't mean to include it in this patch. 
Here is an updated patch.

[Chang Shu]

We probably should hold off the implementation until the proposed API change is made.
https://lists.webkit.org/pipermail/webkit-qt/2011-October/001984.html

[Chang Shu]

(In reply to comment #11)
> Created an attachment (id=110864) [details]
> updated patch
> 
> That change to tests.pro is unintentional. I didn't mean to include it in this patch. 
> Here is an updated patch.

Why not include this change (a modified version) otherwise the qmltests are not built nor run at all?

[Gopal Raghavan]

qmltests is a separate issue. Also, since this patch doesn't seem to be moving forward, I have attached a separate patch to enable qmltests here https://bugs.webkit.org/show_bug.cgi?id=70037

[Chang Shu]

(In reply to comment #14)
> qmltests is a separate issue. Also, since this patch doesn't seem to be moving forward, I have attached a separate patch to enable qmltests here https://bugs.webkit.org/show_bug.cgi?id=70037

Sounds good. While I was compiling the qmltests code, I had to "make install" webkit first. I am not sure if this will be an issue on the bot. Anyway, we can ask Ossy's help to review the patch.

[Kenneth Rohde Christiansen]

Comment on attachment 110864 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110864&action=review

We want to do this in a different way

> Source/WebKit2/ChangeLog:8
> +        Implemented navigationPolicyForUrl. This is similar to DesktopWebView implementation. It provides the flexibiltiy to specify custom navigation policies for TouchWebView.

Please break that line

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/TouchWebView/tst_navigationPolicyForUrl.qml:16
> +        function navigationPolicyForUrl(url, button, modifiers) {
> +            if (button == Qt.MiddleButton && modifiers & Qt.ControlModifier) {
> +                otherWebView.page.load(url)
> +                return TouchWebView.IgnorePolicy
> +            }
> +            return TouchWebView.UsePolicy
> +        }

We decided to do this differently. Simon had some better ideas. Please check the ML

[Csaba Osztrogonác]

(In reply to comment #9)
> (From update of attachment 110739 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110739&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/tests/tests.pro:3
> > +lessThan(QT_MAJOR_VERSION, 6): SUBDIRS += qmltests
> 
> Would this possibly break the buildbot? If not, we can simply remove the condition. Or it should be the other way around? I will add Ossy for commenting this.

I checked, it builds fine on the bot.

[Jesus Sanchez-Palencia]

Should we close this now that all we have is a single QQuickWebView?! :)

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.