Bug 78867

Created attachment 127498 [details] proposed patch

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 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?

Created attachment 127826 [details] Patch for review Corrected enum names

Created attachment 127833 [details] Corrected patch Fixed typoes

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? :)

I like the names (I'm obviously biased :). What do the others think?

It looks good I think.

Created attachment 128307 [details] added missed const

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 on attachment 128307 [details] added missed const Clearing flags on attachment: 128307 Committed r108614: <http://trac.webkit.org/changeset/108614>

All reviewed patches have been landed. Closing bug.

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)]

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