Bug 80164 – [Qt] QWebNavigationRequest 'action' property should have an enum type instead of int

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

[Caio Marcelo de Oliveira Filho]

[Qt] QWebNavigationRequest 'action' property should have an enum type instead of int

[Caio Marcelo de Oliveira Filho]

Created attachment 129909 [details]
Patch

[Rafael Brandao]

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

[Caio Marcelo de Oliveira Filho]

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.

[Tor Arne Vestbø]

Comment on attachment 129909 [details]
Patch

after discussion on IRC

[Caio Marcelo de Oliveira Filho]

Created attachment 130167 [details]
Patch

[Simon Hausmann]

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)

[Caio Marcelo de Oliveira Filho]

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".

[Caio Marcelo de Oliveira Filho]

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