Bug 80164

Created attachment 129909 [details] Patch

Comment on attachment 129909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129909&action=review > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:86 > + RequestActionCount, Shouldn't this be named ActionCountRequest instead (to follow other names pattern)?

Comment on attachment 129909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129909&action=review >> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:86 >> + RequestActionCount, > > Shouldn't this be named ActionCountRequest instead (to follow other names pattern)? It is the Count of RequestActions, it is not an action to be performed itself.

Comment on attachment 129909 [details] Patch after discussion on IRC

Created attachment 130167 [details] Patch

Comment on attachment 130167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130167&action=review > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:86 > + IgnoreRequest = 0xFF Why not simply leave it as its original value and use DownloadRequest = IgnoreRequest + 1? It seems strange to have a gap in the _existing_ enums. or enum NavigationRequestAction { AcceptRequest, IgnoreRequest, LastRequestAction = IgnoreRequest /* Mark internal in docs! */ } DownloadRequest = LastRequest I recall seeing the pattern like this in Qt. (see qnamespace.h)

Comment on attachment 130167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130167&action=review >> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:86 >> + IgnoreRequest = 0xFF > > Why not simply leave it as its original value and use DownloadRequest = IgnoreRequest + 1? > It seems strange to have a gap in the _existing_ enums. > > or > > enum NavigationRequestAction { > AcceptRequest, > IgnoreRequest, > LastRequestAction = IgnoreRequest /* Mark internal in docs! */ > } > > DownloadRequest = LastRequest > > I recall seeing the pattern like this in Qt. (see qnamespace.h) In Qt we use this pattern for ItemDataRole, people create their roles with values >= UserRole. The difference is that functions taking roles as parameter actually take int (see QAbstractItemModel methods). If we want to use the enum type as the property type and as storage for the property (member m_action is of the enum type), we are saying we accept only values inside enum range. It may work in GCC if the value is out of range, but GCC still is unhappy when we construct switch() statements using the type. Casting to int in the switch() alleviates this second problem, but with no true guarantee. Tor-Arne explicitly preferred we do not "clutter" the QML API with the "LastRequestAction" or "RequestActionCount".

Committed r110047: <http://trac.webkit.org/changeset/110047>