Bug 78867 - [Qt] navigationType is missing in new API
Summary: [Qt] navigationType is missing in new API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Blocker
Assignee: Misha
URL:
Keywords: Qt
Depends on:
Blocks: 78821
  Show dependency treegraph
 
Reported: 2012-02-16 19:17 PST by Misha
Modified: 2012-02-23 13:01 PST (History)
11 users (show)

See Also:


Attachments
proposed patch (10.11 KB, patch)
2012-02-16 19:18 PST, Misha
no flags Details | Formatted Diff | Diff
Patch for review (10.62 KB, patch)
2012-02-20 09:20 PST, Misha
no flags Details | Formatted Diff | Diff
Corrected patch (10.59 KB, patch)
2012-02-20 10:36 PST, Misha
no flags Details | Formatted Diff | Diff
added missed const (10.59 KB, patch)
2012-02-22 15:46 PST, Misha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Misha 2012-02-16 19:17:14 PST
In QtWebPagePolicyClient::decidePolicyForNavigationAction() navigationType argument is dropped.

It looks like an oversight. 

I'm attaching proposed patch. Not for review yet...
Comment 1 Misha 2012-02-16 19:18:25 PST
Created attachment 127498 [details]
proposed patch
Comment 2 Simon Hausmann 2012-02-16 23:06:31 PST
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.
Comment 3 Simon Hausmann 2012-02-16 23:13:02 PST
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?
Comment 4 Misha 2012-02-20 09:20:32 PST
Created attachment 127826 [details]
Patch for review

Corrected enum names
Comment 5 Misha 2012-02-20 10:36:25 PST
Created attachment 127833 [details]
Corrected patch

Fixed typoes
Comment 6 Simon Hausmann 2012-02-22 11:54:27 PST
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? :)
Comment 7 Simon Hausmann 2012-02-22 11:55:03 PST
I like the names (I'm obviously biased :).

What do the others think?
Comment 8 Jocelyn Turcotte 2012-02-22 12:42:54 PST
It looks good I think.
Comment 9 Misha 2012-02-22 15:46:20 PST
Created attachment 128307 [details]
added missed const
Comment 10 Simon Hausmann 2012-02-22 22:31:21 PST
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 }.
Comment 11 WebKit Review Bot 2012-02-22 23:21:48 PST
Comment on attachment 128307 [details]
added missed const

Clearing flags on attachment: 128307

Committed r108614: <http://trac.webkit.org/changeset/108614>
Comment 12 WebKit Review Bot 2012-02-22 23:21:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Csaba Osztrogonác 2012-02-23 07:18:54 PST
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)]
Comment 14 Simon Hausmann 2012-02-23 13:01:16 PST
Committed r108640: <http://trac.webkit.org/changeset/108640>