Bug 78867 – [Qt] navigationType is missing in new API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Misha]

In QtWebPagePolicyClient::decidePolicyForNavigationAction() navigationType argument is dropped.

It looks like an oversight. 

I'm attaching proposed patch. Not for review yet...

[Misha]

Created attachment 127498 [details]
proposed patch

[Simon Hausmann]

Totally agreed, that's an oversight. Darn, we forgot that part yesterday in the session, we only briefly looked at it. But yeah, I think it's clear that we need it.

[Simon Hausmann]

Comment on attachment 127498 [details]
proposed patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:101
> +    enum NavigationType {
> +        NavigationTypeLinkClicked,
> +        NavigationTypeFormSubmitted,
> +        NavigationTypeBackForward,
> +        NavigationTypeReload,
> +        NavigationTypeFormResubmitted,
> +        NavigationTypeOther
> +    };

I think the patch is fine. These names are the same that we used in qwebpage.h, I think they're okay. We could iterate over names like

    FormSubmissionNavigation
    ReloadNavigation
    LinkClickNavigation

to shorten them a bit.

Or go all the way and shorten them with a prefix, like ApplicationAttribute or WindowAttribute (Qt::WA_Foobar):

    NT_LinkClicked
    NT_FormSubmitted

but I don't like that very much.

TBH I kind of like the FooNavigation order a bit more than NavigationTypeFoo, but that's just a guts feeling at the moment. Looking forward to comments from others :)

> Source/WebKit2/UIProcess/API/qt/qwebnavigationrequest_p.h:38
> +    Q_PROPERTY(QQuickWebView::NavigationType navigationType READ navigationType FINAL)

I guess CONSTANT is missing here, too?

[Misha]

Created attachment 127826 [details]
Patch for review

Corrected enum names

[Misha]

Created attachment 127833 [details]
Corrected patch

Fixed typoes

[Simon Hausmann]

Comment on attachment 127833 [details]
Corrected patch

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

> Source/WebKit2/UIProcess/API/qt/qwebnavigationrequest_p.h:52
> +    QQuickWebView::NavigationType navigationType();

Missing const here? :)

[Simon Hausmann]

I like the names (I'm obviously biased :).

What do the others think?

[Jocelyn Turcotte]

It looks good I think.

[Misha]

Created attachment 128307 [details]
added missed const

[Simon Hausmann]

Comment on attachment 128307 [details]
added missed const

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

r=me

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:101
> +    enum NavigationType {
> +        LinkClickedNavigation,
> +        FormSubmittedNavigation,
> +        BackForwardNavigation,
> +        ReloadNavigation,
> +        FormResubmittedNavigation,
> +        OtherNavigation
> +    };

I realize that one disadvantage of naming things this way around is that it's not as auto-completion friendly as when the enum values being with a common prefix. Anyway, this one is consistent with the other enums we have right now as well as other enums in QtQuick, such as for example FlickableDirection { ... VerticalFlick }, BoundsBehaviour { ... StopAtBounds }.

[WebKit Review Bot]

Comment on attachment 128307 [details]
added missed const

Clearing flags on attachment: 128307

Committed r108614: <http://trac.webkit.org/changeset/108614>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because an API test fails because of this patch:
FAIL!  : tst_publicapi::publicAPI() 'expectedAPI.contains(actual)' returned FALSE. (QQuickWebView.LinkClickedNavigation --> NavigationType)
   Loc: [/ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/publicapi/tst_publicapi.cpp(162)]

[Simon Hausmann]

Committed r108640: <http://trac.webkit.org/changeset/108640>