Summary: | [Qt] navigationType is missing in new API | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Misha <mtutunik> | ||||||||||
Component: | New Bugs | Assignee: | Misha <mtutunik> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Blocker | CC: | cmarcelo, hausmann, jturcotte, kenneth, laszlo.gombos, maheshk, ossy, pierre.rossi, vestbo, webkit.review.bot, yael | ||||||||||
Priority: | P3 | Keywords: | Qt | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 78821 | ||||||||||||
Attachments: |
|
Description
Misha
2012-02-16 19:17:14 PST
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> |